Problem/Motivation
As a carry over from Configuration Packager, currently we model config in FeaturesManager::configCollection
as a structured array. A piece of configuration has a complex set of properties and FeaturesManager
includes several methods related to configuration items. It would be more consistent and appropriate to model configuration items as a custom object type.
Proposed resolution
Create a new FeaturesConfigurationItem
object. TBD: is there any existing core object we should consider extending?
Model existing array keys as properties.
Identify methods to move from FeaturesManager
to FeaturesConfigurationItem
. Candidates:
- ::getFullName()
- ::getConfigType()
Remaining tasks
Comment | File | Size | Author |
---|---|---|---|
#33 | 2595265-33.patch | 2.26 KB | dawehner |
| |||
#30 | 2595265-30.patch | 39.81 KB | mpotter |
| |||
#27 | interdiff.txt | 10.8 KB | mpotter |
#27 | 2595265-27.patch | 34.34 KB | mpotter |
| |||
#20 | interdiff.txt | 897 bytes | dawehner |
Comments
Comment #2
nedjoComment #3
mpotter CreditAttribution: mpotter commentedThis would be worthwhile and would also cause API changes, so need to do this before beta.
Comment #4
dawehnerThis is just a quick start so far:
* One fundamental question: should we deal with an immutable object?
* Should we keep an ArrayAccess for now or should everything be converted ...
Comment #5
dawehnerSome small updates
Comment #6
nedjoGreat start, thanks!
We make changes to items in the config collection in assignment plugins, so I'm assuming not.
I guess for clarity it makes more sense to convert to using object property notation?
Comment #7
dawehnerI mean on the longrun this is for sure, but I'm curious whether we should keep the array object in order to keep the issue limited.
Currently looking at some conversions.
Comment #8
dawehnerSome more work with an additional todo, should this value object have an interface? Personally I think this should be avoided as long as possible.
Comment #9
nedjoLooking great.
Can we now remove these methods and replace calls to them with calls to equivalent
ConfigurationItem
methods?If so, should we wait and do that as a followup patch?
Comment #10
dawehnerIn general I'm a huge fan in incremental approaches, even they introduce temporarily non optional approaches.
Happy to work on extracting those helper methods though, i'll do that in a local branch so splitting up will be easy.
Comment #11
dawehnerSo I was wondering whether there should be a
SimpleConfigurationItem
ConfigEntityConfigurationItem
to distinct the logic.
Another question which you could ask, should this object really have everything as a big list of constructor arguments or should there be an array of properties like on
\Drupal\Core\Entity\EntityType
itself.Comment #12
nedjoAgreed. Let's aim to do the minimum first step here.
To avoid some of the conditional logic built into methods like
FeaturesManager::getFullName()
? They would extend a common base class? Sounds like a worthwhile refinement, though maybe one we could leave for followup?That sound good to me, with, probably, accompanying
::get()
and::set()
methods equivalent toEntityType::get()
andEntityType::set()
rather than a distinct getter/setter for each property.Comment #13
dawehnerYeah something like that, but sure, it should be in a followup.
Not sure whether we need
get/set()
. Afaik this is there to accomplish the feature to add arbitrary entity metadata which I think features won't need?Currently on fixing the tests locally.
Comment #14
dawehnerAlright, fixed the test.
Comment #15
mpotter CreditAttribution: mpotter commentedThis is looking pretty good. My only concern is that giant constructor and the effect that has on creating new config objects. There is an awful lot of '', NULL, [] kind of magic arguments being passed which makes it very hard to read. It also makes it hard to maintain when new properties are added to this object.
I think a better approach might be to have the setter methods return the config object itself. Then make the constructor take the minimum (if any) arguments. Then you could have code that could convert the existing:
into something more like this:
Or something like that.
Comment #16
dawehnerWhat about enabling both?
Comment #18
dawehnerHa, its great to have some form of tests!
Comment #19
mpotter CreditAttribution: mpotter commentedMinor nit-pick, but do we actually need the extra () around the "new ConfigurationItem..." statement?
Otherwise, I really like this dual approach! Moving the extra properties into an array argument was also much better and more extensible for the future. I'll apply this patch and test it today.
Comment #20
dawehnerOh yeah, they aren't needed at all, unless you want to put additional function calls inline.
Comment #21
mpotter CreditAttribution: mpotter commentedOK, did some testing and this looks good, so committed it to c06283e.
Now that it is refactored we can add some additional unit tests for it.
Comment #22
mpotter CreditAttribution: mpotter commentedOops, missed a couple that I forgot to push from my testing. Fixed in commit b72e60b.
Comment #25
mpotter CreditAttribution: mpotter commentedOK, sorry, my bad. This still needs some work. I reverted the commits and will continue to test more and work on it.
Comment #26
dawehner@mpotter
Yeah I could totally imagine bits which aren't perfect :(
Comment #27
mpotter CreditAttribution: mpotter commentedOK, here is a new patch for somebody else to also test and review. Included an interdiff of what I had to fix vs #20.
Comment #28
mpotter CreditAttribution: mpotter commentedNote, that it might be good to refactor the $manager->getConfigType() function to also return a configuration item instead of array since it causes some inconsistent code that is hard to maintain.
Comment #29
nedjoConfigurationItem.php file is missing from the patch.
Comment #30
mpotter CreditAttribution: mpotter commentedOoops. Try this.
Comment #32
nedjo@dawehner, @mpotter: nice work!
Ran through its paces, no issues showed up. One more beta blocker down :)
Setting to "needs work" because after I committed this I noticed a missed change in
FeaturesManager::reset()
(should call$config->setPackage()
):Comment #33
dawehnerGood observation, here is a test.
Comment #35
nedjoApplied, thanks!
Comment #40
nedjo