ENH Make all GridField components injectable (using abstract class) #10204
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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. The new AbstractGridFieldComponent also makes it easy to make similar wide-spread changes in the future.
GuySartorelli
force-pushed
the
enh/abstract-gridfield-component
branch
from
d82d9d8
to
3edc3e0
Compare
emteknetnz
reviewed
Jan 20, 2022
There was a problem hiding this comment.
Looks good to me, I think I prefer the abstract class approach. It would allow us to easily add any other traits in the future if we wanted e.g. Extensible, Configurable
If other are happy with this I'm happy to merge. I'll run this through wider silverstripe/installer CI to see if it catches anything
https://github.com/emteknetnz/silverstripe-installer/actions/runs/1721684404
emteknetnz
approved these changes
Feb 1, 2022
|
Cross repo run above using this PR at a patch file was green, happy with this |
GuySartorelli
added a commit
to GuySartorelli/silverstripe-framework
that referenced
this pull request
Feb 6, 2022
I figured I'd write this while these changes are fairly fresh in my mind, and save someone else the trouble. Obviously if there's some procedure ormpolicy for writing release notes that I'm not following here feel please let me know and feel free to close this PR. Refer to silverstripe/silverstripe-admin#1251 for preview PRs, silverstripe#10204 for the `GridFieldComponent` stuff, and silverstripe/silverstripe-elemental#913 for changes to elemental search indexing. There are some extra new lines in here that I can't remove from the GitHub editor - but I expect there will be other requested changes so I'll clean that up at the same time.
This was referenced Feb 6, 2022
Merged
Merged
GuySartorelli
added a commit
to GuySartorelli/silverstripe-framework
that referenced
this pull request
Jul 6, 2022
…ilverstripe#10204) * ENH Make all GridField components injectable. 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. The new AbstractGridFieldComponent also makes it easy to make similar wide-spread changes in the future. * DOCS Encourage injection for GridField and GridFieldComponents.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Alternative to #10154.
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.
The new
AbstractGridFieldComponentclass also makes it easier to make similar wide-spread changes to the components in the future.Separate pull requests will need to be made to other silverstripe core modules to replace use of the
newkeyword to take advantage of this - I'll work on that as time permits if this PR gets approved.