Address conflict with other modules over config.installer class

+++ b/src/FeaturesServiceProvider.php
@@ -23,7 +23,10 @@ class FeaturesServiceProvider extends ServiceProviderBase {
+    }
   }

So we should at least log that we did not replaced the service. We could get rid of the problem by decorating an existing ConfigInstaller, which though would require a couple of more lines of code in features, so basically \Drupal\Core\Config\ConfigInstaller::checkConfigurationToInstall and all its dependent code. I guess for now that would be certainly too much for this issue.

Don't think it causes an API change in base Features, just in the config.installer replacement service.

In the hook_requirements can you not get the class from the current container like you do in the alter code? (using \Drupal::getContainer()) ?

But wait, going back to #9. Why can't we use a Trait and just replace the protected method? You shouldn't need to replicate the public method that calls it. In your child class, calling the public method defined in the parent class will still invoke the protected method in your child class. So I'm thinking there is still a cleaner way to do this.

Here is my test of using Traits to override protected functions: https://gist.github.com/mike-potter/c380ebc4f05469cfec5a8cf464995e6d

Oh, I see, you weren't talking about Traits, you were talking about Decorators.

Let me give this more thought. I'm still leary of just relying on module weight. Seems like there should be a way to do this that is cleaner and would allow other modules to still inherit the Features config installer behavior.

hi,

In this patch (2625310-14.patch) translation function used incorrectly. Use $this->t() instead of t() I have created a interdiff.

Going back to the approach in #9. I found that I could decorate the existing config installer service by still using a subclass rather than having to re-implement everything in a separate class. Tested this and it properly allows pre-existing config in feature modules to be installed.

It also removes the need for the proxyclass for our service I believe, but someone could correct me if I'm wrong on that.

Also, FYI, this behavior already gets tested by FeaturesCreateUITest. If you don't have the config installer overridden several of the asserts will fail. So test coverage for this is good.

Nedjo: So I tested this. I created another decorator for config.installer. If I set it's "decoration_priority" higher than 5 then Features config installer "wins". If I set it's "decoration_priority" lower than 5 then the new decorator wins.

So you are correct that only one can run at once, but that's at least the same as your proposal in #14, and I think this method is cleaner. I'll play around with whether I can get multiple decorators to both work at the same time with different methods.

Ah ha!!! It worked!

Nedjo: Here is what I did. I created another config.installer that *did* take the "inner" method:

  features.config.anotherinstaller:
    class: Drupal\features\FeaturesAnotherConfigInstaller
    public: false
    decorates: config.installer
    decoration_priority: 4
    arguments: ['@features.config.anotherinstaller.inner', '@config.factory', '@config.storage', '@config.typed', '@config.manager', '@event_dispatcher']

Then, in that new installer I modified a public function. It properly executed BOTH the "anotherinstaller" as well as the Features decorator. Here is a patch file that shows the additional installer that I created. I think you can use this as a model in your other module. As long as your decoration_priority is lower than Features then both should work. In fact, maybe I'll raise the priority of Features installer to 9.

Basically, Features Installer wants to have the highest priority and doesn't need the "inner" service because it's directly overriding a protected method, rather than the normal way decorators use the "inner" service to override public functions.

So here is a slightly revised patch from #19 that adds the inner service in case it needs to be called and just to better follow decorator convention.

In the strictest case we might need to pull in the public checkConfigurationToInstall() and then have it call $this->configInstaller->findPreExistingConfiguration(), but so far I don't find that to be needed. So for now I'm just going to leave the override of the protected class.

Having a decorator service "extend" the existing service rather than using a completely separate class that implements the same interface is used already in core (see BigPipe for example). So this looks like a pattern people are already using. Only thing special here is that it seems to work for protected functions. But I honestly haven't found any real Symfony documentation on this which would exclude this possibility from working.

I also set the decorator priority to 9 in this patch so it's clear to other decorators that they should use a lower number.

Shoot, that is what I get for last minute copy/paste. Try this.

I think this is still a good use for decorators. Features is "layering" a piece of functionality on top of the normal config installer to allow Features to be imported. This is likely a piece of functionality that any site using BOTH Features and some other module would need. In your Config Provider, I'd still expect to be able to install a features module with existing config.

I'd recommend actually trying it rather than trying to determine whether it will work or not theoretically. I believe in Configuration Provider you should be able to use the same approach I'm using here and just supply your protected function. You don't need to worry about which public functions it is called from. Then give it a priority lower than features. Add some print statements temporarily to the features getExistingConfig function to see if it still gets called when enabling a module and if your protected function also gets called. I think there is actually a good chance that it will just work.

Rather than closing this as "works as designed" I'd much rather commit #25. I find the use of a decorator here *much* cleaner and better than just overwriting the existing config installer in the alter function like we previously did. Even if #25 doesn't quite work for your specific case, it enables many other cases of projects that override the config installer.

I'd like to include #25 as part of RC1, so looking for final comments on whether this decorator approach is better than the existing alter method of overridding the normal service.

Here is an update that uses $this->configInstaller instead of $this in two places. Also removed the start/end comments.

Ahh, those were both protected methods, so can't use this->configInstaller with them. Which is still fine. It will call whatever protected method is in lower-priority decorators, or in the base class. Still works and still allows other decorators. So back to mostly the same as #25 with just the comment and code cleanup.

In IRC, dawehner's comment was:

that is all you need? Nice
that's kind of an interesting approach, this is for sure
but don't you still have to call to this->configInstaller as much as possible?

So, addressed his question with "No, those are protected methods, so this->configInstaller not needed". Thus, if #32 passes tests, I'm going to commit it unless somebody posts a strong objection.

Committed to 77534a3