Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sealed TextFieldDisplayDriver in 2.0 makes upgrading difficult #16886

Open
brunoAltinet opened this issue Oct 15, 2024 · 24 comments · May be fixed by #16898
Open

Sealed TextFieldDisplayDriver in 2.0 makes upgrading difficult #16886

brunoAltinet opened this issue Oct 15, 2024 · 24 comments · May be fixed by #16898
Milestone

Comments

@brunoAltinet
Copy link
Contributor

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
Copy link
Member

Piedone commented Oct 15, 2024

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

@brunoAltinet
Copy link
Contributor Author

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
Copy link
Member

Piedone commented Oct 15, 2024

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
Copy link
Contributor Author

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
Copy link
Member

@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
Copy link
Contributor Author

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
Copy link
Member

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

@Piedone
Copy link
Member

Piedone commented Oct 15, 2024

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
Copy link
Contributor Author

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
Copy link
Contributor Author

@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
Copy link
Member

Piedone commented Oct 15, 2024

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
Copy link
Contributor Author

brunoAltinet commented Oct 17, 2024

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
Copy link
Member

Piedone commented Oct 17, 2024

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
Copy link
Member

Piedone commented Oct 22, 2024

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
Copy link
Contributor Author

brunoAltinet commented Oct 22, 2024 via email

@brunoAltinet
Copy link
Contributor Author

brunoAltinet commented Oct 23, 2024

@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
Copy link
Member

Piedone commented Oct 23, 2024

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.

@sebastienros sebastienros added this to the 2.x milestone Oct 24, 2024
Copy link
Contributor

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
Copy link
Member

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
Copy link
Member

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
Copy link
Member

Piedone commented Oct 24, 2024

As in, omit TypePartDefinition in the PR.

@brunoAltinet
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants