Remodel configuration items in FeaturesManager::configCollection as objects

This would be worthwhile and would also cause API changes, so need to do this before beta.

This 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 ...

Some small updates

I guess for clarity it makes more sense to convert to using object property notation?

I 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.

Some more work with an additional todo, should this value object have an interface? Personally I think this should be avoided as long as possible.

In 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.

Identify methods to move from FeaturesManager to FeaturesConfigurationItem. Candidates:

::getFullName()
::getConfigType()

So 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.

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?

Yeah something like that, but sure, it should be in a followup.

That sound good to me, with, probably, accompanying ::get() and ::set() methods equivalent to EntityType::get() and EntityType::set() rather than a distinct getter/setter for each property.

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.

Alright, fixed the test.

This 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:

    $config_collection['example.config2'] = new ConfigurationItem('example.config2', [
      'dependencies' => [],
    ], '', '', '', [], '', 'package2', TRUE, 'my_feature');

into something more like this:

$config_collection['example.config2'] = new ConfigurationItem('example.config2')
  ->setData(['dependencies' => []])
  ->setPackage('package2')
  ->setProvidingFeature('my_feature')

Or something like that.

What about enabling both?

Ha, its great to have some form of tests!

+++ b/src/FeaturesManager.php
@@ -948,12 +953,10 @@ class FeaturesManager implements FeaturesManagerInterface {
+          $config_collection[$name] = (new ConfigurationItem($name, $data, [

Minor 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.

Oh yeah, they aren't needed at all, unless you want to put additional function calls inline.

OK, 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.

Oops, missed a couple that I forgot to push from my testing. Fixed in commit b72e60b.

OK, sorry, my bad. This still needs some work. I reverted the commits and will continue to test more and work on it.

@mpotter
Yeah I could totally imagine bits which aren't perfect :(

OK, here is a new patch for somebody else to also test and review. Included an interdiff of what I had to fix vs #20.

Note, 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.

Ooops. Try this.

Good observation, here is a test.