Sealed TextFieldDisplayDriver in 2.0 makes upgrading difficult

[brunoAltinet]

Describe the bug

I've reached out about this on Discord, but I feel this should be considered a bug and maybe better to discuss it here.

I would inherit/extend a TextFieldDisplayDriver, where i would use both
.IsNew property and .TypePartDefinition properties to modify the resulting shape. Now that TextFieldDisplayDriver is sealed, i have to move that logic somewhere else.
I considered moving it to IShapeTableProvider but i don't have either of the former properties. I would hate to copy/paste all of the TextFieldDriver logic just to alter some displays.
Doing it inside the template would prolly have me in the same problems.

Orchard Core version

2.0

To Reproduce

Try customizing behaviour of TextFieldDisplayDriver, usually done by inheritance.

Expected behavior

There should be a good way to extend Content Field behaviour. Either add the same context to Shape events or some other way. No idea. I support having these classes sealed as long as the same thing can be done in a different way.

Logs and screenshots

n/a

[Piedone]

Have you tried adding a second, custom driver (a part/field can have any number of drivers)?

[brunoAltinet]

I would have to have that driver affect the same shape without actually creating a new shape. What used to happen, when i didn't deregister the default one, is it would create 2 inputs. I am not sure how to have 2 drivers work on the same shape?

[Piedone]

You can display the same shape from your own driver too, with a different differentiator (to be able to target it from placement, see ShapeResult.Differentiator()), and hide the original one from placement.

[brunoAltinet]

From what i understand you, 2 drivers would create 2 shapes, i would ignore/hide 1 shape and show the other?
But that would mean i'd have to copy all of the logic from original driver (since i can't inherit it) and do my own customization.
I've looked through the code, I don't see how i can have same shape result processed by different drivers.

Shape Events (ondisplaying for instance) look like a better way of doing this, but as i said, no way to access the stuff i need, even though it could be provided (that context is still around when those events are getting fired).

[MikeAlhayek]

@brunoAltinet it would be nice to share your code so we can understand your logic on why you really need to inherit from a display driver. Maybe you are looking for a custom editor?

[brunoAltinet]

Sure:
It's a bit convoluted but there are a couple of use cases:

1. make a field read only after initial creation depending on user privilege
2. customize labels for the same field depending on parent content item type

see here:
https://github.com/altibiz/ugp-web/blob/273098622442cb5b5e2f3d693ee21aa7937e06c3/Members/Base/PartFieldSettings/PartTextFieldDriver.cs#L34

and here:

https://github.com/altibiz/ugp-web/blob/273098622442cb5b5e2f3d693ee21aa7937e06c3/Members/Base/DriverService.cs#L38

[MikeAlhayek]

@brunoAltinet can't you just add a new editor that wraps that logic instead of having to inherit and register yet another service?

[Piedone]

These are both better done from IShapeTableProvider:

• AdminAttribute.IsApplied() check: you can hide (place) shapes with any custom logic, see an example here.
• Customize label: in OnDisplaying you can pass new properties to the shape, including such a value, and then use a custom shape override to use that label. I don't recommend setting the corresponding ContentPartFieldSettings since that's a global config for the field, but that you can do there too (as well as from an IContentFieldHandler).

[brunoAltinet]

These are both better done from IShapeTableProvider:

• AdminAttribute.IsApplied() check: you can hide (place) shapes with any custom logic, see an example here.
• Customize label: in OnDisplaying you can pass new properties to the shape, including such a value, and then use a custom shape override to use that label. I don't recommend setting the corresponding ContentPartFieldSettings since that's a global config for the field, but that you can do there too (as well as from an IContentFieldHandler).

How do I know if the shape is for a new contentitem? And second, I'd need access to parent part settings, is that possible?
That was pretty straightforward from driver

[brunoAltinet]

@brunoAltinet can't you just add a new editor that wraps that logic instead of having to inherit and register yet another service?
Tried it but wrapping doesnt work, Prefix is not sert on a wrapped component.

[Piedone]

How do I know if the shape is for a new contentitem? And second, I'd need access to parent part settings, is that possible? That was pretty straightforward from driver

The examples you've linked to don't utilize IsNew. But you can also access that from an IContentDisplayHandler.BuildEditorAsync() implementation.

You have access to the whole content item, including the field, from content field shapes. If you inspect the context with the debugger in the even, you'll see what's available.

[brunoAltinet]

How do I know if the shape is for a new contentitem? And second, I'd need access to parent part settings, is that possible? That was pretty straightforward from driver

The examples you've linked to don't utilize IsNew. But you can also access that from an IContentDisplayHandler.BuildEditorAsync() implementation.

You have access to the whole content item, including the field, from content field shapes. If you inspect the context with the debugger in the even, you'll see what's available.

The Editor context is not handed over further down, and IsNew is used there. See here: https://github.com/altibiz/ugp-web/blob/273098622442cb5b5e2f3d693ee21aa7937e06c3/Members/Persons/PersonPart.cs#L51

I am not sure what you mean access that from an BuildEditorAsync implementation.
There is no isNew boolean on content field shapes, and there's no way of knowing (from what i can see) if the contentItem is a new one other than that flag on the driver, which is not propaggated further.

To iterate, i need access to parent part definition and if the content item is new at the same time to determine how the field is shown.
Example of request: Member email field should be shown as read-only to regular users after initial input.
Overriden driver had everything i needed. Now i need to either completely copy the code from that driver or do something else. Shape events does not have isNew keyword.

I suggest handing over isNew to shape events somehow, or even the whole editor context. I'll make an example PR.

[Piedone]

If you implement IContentDisplayHandler.BuildEditorAsync(), which is an extension point (much like content part handlers) for use cases like yours, then you can access IsNew as well, since it receives BuildEditorContext.

[Piedone]

So, I don't think any change should be needed for your use case, let alone in general in the magnitude of #16898. So, please tell why you think it's still necessary.

[brunoAltinet]

I'm still working on this, intermittently, will report back tomorrow on how I find this solution.

…

________________________________ From: Zoltán Lehóczky ***@***.***> Sent: Wednesday, October 23, 2024 1:25:10 AM To: OrchardCMS/OrchardCore ***@***.***> Cc: Bruno Samardzic ***@***.***>; Mention ***@***.***> Subject: Re: [OrchardCMS/OrchardCore] Sealed TextFieldDisplayDriver in 2.0 makes upgrading difficult (Issue #16886) So, I don't think any change should be needed for your use case, let alone in general in the magnitude of #16898<#16898>. So, please tell why you think it's still necessary. — Reply to this email directly, view it on GitHub<#16886 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AA6ZALBMP2EGJJ3Y7BVQAY3Z43NFNAVCNFSM6AAAAABP6UTHAKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDIMZQGQ4TOMJUGY>. You are receiving this because you were mentioned.Message ID: ***@***.***>

[brunoAltinet]

@Piedone so i did manage to get it working via IContentDisplayEditor, even though the implementation to me seemed too involved compared to what I was trying to achieve. If anyone's interested, it's a pretty cool exercise on how to modify shapes before they get rendered.

I still think that the "IsNew" flag would be beneficial on the template level, it would make this handler redundant since that's the only info i can't get trough service container.

I also think that Field ViewModels should inherit from IShape or Shape, instead of having this Castle magic doing it's thing. It would save me from casting to IShape and it kinda gives the wrong impression that the Model in template is POCO, but that's nor here nor there.

I lost a lot of time to get this right, and I'm not a novice. I haven't found a good example on this overriding IContentDisplayHandler.

As for PR, I would omit the TypePartDefinition but would keep the IsNew flag for better developer experience if anyone wants to extend content field functionality. I'd also base those Field Viewmodels on Shape if you think that's ok. Let me know what you think.

link to related class and code: https://github.com/altibiz/ugp-web/blob/main/Members/ContentDisplayHandlerExt.cs

    internal class ContentDisplayHandlerExt : IContentDisplayHandler
    {
        private IHttpContextAccessor _httpCA;

        public ContentDisplayHandlerExt(IHttpContextAccessor httpCA)
        {
            _httpCA = httpCA;
        }

        public Task BuildDisplayAsync(ContentItem contentItem, BuildDisplayContext context)
        {
            return Task.CompletedTask;
        }

        public Task BuildEditorAsync(ContentItem contentItem, BuildEditorContext context)
        {
            HandleEditorShape(contentItem, context, context.Shape as Shape);
            return Task.CompletedTask;
        }

        private void HandleEditorShape(ContentItem contentItem, BuildEditorContext context, IShape theShape)
        {
            //Check if Shape has any items (shapes for fields) and process them
            foreach (var item in theShape.Items.Where(x=>x is EditTextFieldViewModel or EditTagTaxonomyFieldViewModel or EditTaxonomyFieldViewModel or EditDateFieldViewModel))
            {
                (item as IShape).SetFieldSettingsExt(context.IsNew, AdminAttribute.IsApplied(_httpCA.HttpContext));
            }

            //Traverse through any child shapes and handle their items
            foreach (var val in theShape.Properties.Where(x => x.Key != "Parent").Select(x => x.Value).OfType<IShape>())
            {
                HandleEditorShape(contentItem, context, val);
            }
        }

        public Task UpdateEditorAsync(ContentItem contentItem, UpdateEditorContext context)
        {
            return Task.CompletedTask;
        }
    }

[Piedone]

Thank you!

Please update your PR with:

As for PR, I would omit the TypePartDefinition but would keep the IsNew flag for better developer experience if anyone wants to extend content field functionality.

I'd also base those Field Viewmodels on Shape if you think that's ok.

I'm really not sure about that. Do we have similar cases? In any case, this should be a separate PR.

[github-actions]

We triaged this issue and set the milestone according to the priority we think is appropriate (see the docs on how we triage and prioritize issues).

This indicates when the core team may start working on it. However, if you'd like to contribute, we'd warmly welcome you to do that anytime. See our guide on contributions here.

[sebastienros]

I'd also base those Field Viewmodels on Shape

That could break the current drivers which use an overload to create a shape result based on a type that doesn't implement IShape and then create the proxy.

[sebastienros]

I would omit the TypePartDefinition but would keep the IsNew flag

Why? Is this not useful anywhere? Or is it because it can be retrieved in other way (it's not contextual to the curent view).

[Piedone]

As in, omit TypePartDefinition in the PR.

[brunoAltinet]

I would omit the TypePartDefinition but would keep the IsNew flag

Why? Is this not useful anywhere? Or is it because it can be retrieved in other way (it's not contextual to the curent view).

It can be retrieved from PartFieldDefintion.ContentTypePartDefinition

[brunoAltinet]

That could break the current drivers which use an overload to create a shape result based on a type that doesn't implement IShape and then create the proxy.

Hmm, not sure how it would brake those? Anyway, i'll do a PR and we can go from there. Castle is convenient but hinders discoverability. Btw, what about using default implementations on IShape? Would that be a breaking change?

[sebastienros]

Not sure this was mentioned, if you can't alter an existing driver, you can always unregister it and add your own instead. It being sealed would not be a problem for that.