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

ENH Make all GridField components injectable. #10154

Conversation

GuySartorelli
Copy link
Member

Some components were already injectable, but all GridField components shipped in silverstripe should be injectable.
This makes it a LOT easier to make global project-specific changes to a given component.

Separate pull requests will need to be made to other silverstripe core modules to replace use of the new keyword to take advantage of this - I'll work on that as time permits if this PR gets approved.

Some components were already injectable, but all GridField components
shipped in silverstripe should be injectable.
This makes it a LOT easier to make global project-specific changes to a given component.
@GuySartorelli
Copy link
Member Author

GuySartorelli commented Nov 18, 2021

Another way to do this, now that I've gone and done it this naïve way, would be to make an AbstractGridFieldComponent class that implements GridFieldComponent and uses Injectable, and have all of the built-in components extend the new abstract class.
If maintainers prefer the abstract class way of doing this I'm happy to refactor the PR, just let me know.

Edit: That would have the advantage of making it easier to add enhancements that should cover all components in the future, so I may end up doing that anyway if I have time before this gets merged.

@michalkleiner
Copy link
Contributor

Thanks @GuySartorelli. I think I'd prefer the AbstractGridFieldComponent approach, but can you please open it as a separate PR and keep this until we make the final call?

@GuySartorelli
Copy link
Member Author

@michalkleiner Done - see #10204

@michalkleiner
Copy link
Contributor

Thanks @GuySartorelli!

Is the addition of the Injectable trait the only thing to live in the abstract class? Nothing else can be abstracted there?

@GuySartorelli
Copy link
Member Author

@michalkleiner That's all I was intentionally doing for the purposes of this pull request. I didn't really look further than that as to what might be common between all components - but given that the interface is literally empty I would hesitate to put anything into the abstract class without a fairly thorough analysis of not only the built-in components but also some common third-party components to be sure there won't be any unintended side-effects.

I definitely won't have time to have a proper look at the similarities between components any time soon in any case but would be happy to make changes/add stuff to the abstract class if there is something specific that someone can point out. But that might be best left for a future follow-up pull request IMO.

@emteknetnz
Copy link
Member

Alternative PR using an abstract class was merged, closing this one

@emteknetnz emteknetnz closed this Feb 1, 2022
@GuySartorelli GuySartorelli deleted the enh/make-gridfield-components-injectable branch February 1, 2022 22:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants