RevertInterface::revert() and ::import() do not handle config dependencies

For (b), there is already ConfigRevertInterface::import() that does the import of new configuration. You use ::revert() to revert an existing config item, and ::import() to import a new one.

For (a)... yeah. The class was set up to handle the needs of the Config Update UI (later the module was split into a Base module and the UI/Reports module being separate). So the idea was in the UI, you would be looking at one config item, look at the diffs, and then revert to the current config provided by the module, presumably/hopefully with only minor changes.

So, functionality to handle config dependencies could be added to this module. What we'd need to do is, I think, add a new interface with a new method that handles this, and then have the ConfigReverter class implement this new interface as well as ConfigRevertInterface that it already implements.

Note: I think a new method is better than changing the behavior of an established interface method -- the module has an API that is established, and changing how the existing methods behave, or adding new methods to an existing established interface could break compatibility/expectations in other modules that rely on the Base module. So it would need to be a new method on a new interface.

Thoughts?

It sounds like that Core method does more/different stuff than the existing ::revert() and ::import() methods. I am not against adding a new method to ConfigReverter() that wraps or duplicates it (on a new interface), but I don't think it's OK to change the behavior or remove our existing methods, since they are now an established API.

I think when I created this Reverter class and its methods, I looked into that Core method (vague memories) and decided it wasn't really what was needed for this purpose... not sure why now... will need to look again to remember why (maybe remember).

The core method seems designed for use in a context where it receives a full set of configuration, like that of a set of modules being installed, or of the site as a whole. It just handles the question of ordering dependencies, not collecting them.

That is, for our purposes, the prior problem is: how do we ensure we pass in all config that's needed?

Say I've just updated two modules, Module A that has a new field and Module B that has the corresponding new field storage. If I run an update operation just on Module A, it will still fail, since the field storage will be missing.

This is why in Configuration Synchronizer (which indirectly leverages core's config handling), I'm working with full sets of config updates.

OK, this is starting to trigger my memory... Yeah, I think you are right nedjo -- to use that Core method, Core would need to know more than just "I'm reverting or importing this one config item".

There may be a path forward... maybe it can look at (a) the other config/optional and config/install directories and (b) the current config and figure it all out though? Or maybe we can make a method that does this? The problem would be that if you updated a module, and were "reverting" to update a config item to a newly released version, you wouldn't really know whether it depended on another module's config also being the newly released version, would you?

Either way... I'm definitely open to adding this to the Config Update API, as a new method on a new interface, but included in the Reverter class. But as you say, it might be easier for you to handle it on your ConfigInstaller override... I guess for Config Update to handle it, we'd need to extend that class too?

Here's another idea: What if we just added a method that, given a set of configuration, did the sorting based on dependencies? So the idea would be that Features (or whatever) could call this method if it had a bunch of config to update, and then after finding out what order to do the updates, it would go ahead and revert/import individually in that order.

I guess this would be best off being on the ConfigLister class, not on Reverter, maybe? Because it seems like more of an "informational" method than a "do an action" method. And it could have an option to either check dependencies using current storage or install/optional storage.

Thoughts?

More accurate title

Linking to previous issue with some related discussion.

What if we added a ConfigReverter::revertOrImport(one config) and ConfigReverter::revertOrImportRecursive([list of configs])
revertOrImport() would revert a config if it exists, otherwise import it.
revertOrImportRecursive() would loop over the list of configs calling revertOrImportRecursive() on each dependency of each config, then revertOrImport() each config that hasn't been already. We have to add some static variable for revertOrImportRecursive() so that it only runs once for each config.

To do the "revert entire module" you would call revertOrImportRecursive() with the list of all the configs in the module.

That could be done... I am not sure this is needed, though?

a) I don't know that a revertOrImport() method is needed. The original issue report was saying there wasn't an import() method at all, but there is one already.

b) I also don't think this recursive approach is needed... just not seeing it. I think what was being asked for was just to make sure that if a list was passed in, that they were done in the right order (if both A and B are asked for, and A depends on B, do B before A). Right?

c) Also I think we shouldn't invent our own way of doing things. We should use Core's way that already exists?

Re #13

a) You need revertOrImport because the use-case is "revert an updated feature". The config entities being reverted could have new dependencies that need to be imported

b) I'm not sure if the recursive thing is needed. I guess it just depends how much "magic" this feature needs to have. If we want to use it to revert a feature and import all its new dependencies, then it probably needs enough magic to know all the dependencies dependencies, etc.

c) You're right, we should figure out how to use ConfigDependencyManager for gathering the dependencies

OK, well it seems that we have not agreed upon what the method needs to do.

Let's try to write an API documentation block for the function, with @param and @return and a good description, to define the API, and then make sure that everyone, including @mpotter who originally reported this issue, agrees on the functionality, and then write the function. Because I am not quite understanding what is needed, and I'm not sure it's just one function -- there may be two or three different use cases.

Since I am not clear on what is needed, can someone else take the first pass at writing the API docs? Thanks!

Updating status to reflect #15...

Hello! @mpotter / whoever... Do you still need this functionality?

If so, can you write a doc block/signature for the method or methods you would like to have, in an ideal world, that would provide you with the functionality you need?

And if not, maybe we should just close this as Outdated? I don't want to add functionality to the module that isn't needed for an actual use, just because it might theoretically be useful.

And if there's a related but not exactly the same need, let's make a new separate issue to cover that.

Thanks!

I was referred to this issue by @nedjo on #2592751: Optional configuration listed for import regardless of validity.

In my case, when I do the Features Import All operation, optional config gets installed even when it has dependencies that are not met.

A new method to address this functionality as well as the what's described above would have essentially the same footprint/docblock, except but would have a few changes to the functionality.

/**
 * Reverts configuration.
 *
 * @param string $collection
 *   The configuration collection.
 * @param array $config_to_create
 *   An array of configuration data to create, keyed by name.
 * @param bool $optional
 *   (optional) Indicates if the configuration is optional, defaults to FALSE.
 */

I propose the functionality of the method to be something like:

1. Gather dependencies: Look at specified $config_to_create and add dependencies which are not already installed or included in $config_to_create to a list of dependencies.
2. Analyze dependencies: For optional config, we'll want to remove items from the list which do not have their dependencies met and probably regather the list of dependencies so it will not include orphans (dependencies only applicable to an optional config that was unset). For non-optional config, we'd want to throw an exception if dependencies are not met.
3. Install dependencies: Install the dependencies in appropriate order.
4. Install config: Install the config in appropriate order.

Thoughts?

One note on the comment above, looking at the code for the modules, it wasn't obvious if Configuration Update Manager had or could easily obtain the distinction between required and optional config. Features has that information readily available (in the drush command at least) so I thought it'd be easiest to have the calling code dictate if the configuration was to be treated as optional.

Hm. I'm a bit confused by #18. My questions:

a) Is this method reverting or importing? The first line says reverting, but the rest of the comment sounds like importing.

b) I don't think any of the other methods in this module take as input a configuration collection... how would that work and what would it do?

c) I also don't understand what the $optional parameter means here. Which configuration is optional?

I think we just got bitten by this issue. We have a distribution in which several modules have optional config that now requires "Layout Manager" and "Field Layout", but we had accidentally missed adding dependencies for both modules to the install profile. We were finding that our optional config wasn't getting installed in new environments (we didn't initially realize it was a dependency issue), and Config Update let us work around that by allowing us to import the configs using "Import from Source". However, after we did that, attempting to access any node form configuration page for an affected node type (e.g. admin/structure/types/manage/article/form-display would result in this fatal error:

Symfony\Component\DependencyInjection\Exception\ServiceNotFoundException: You have requested a non-existent service "plugin.manager.core.layout". Did you mean one of these: "plugin.manager.condition", "plugin.manager.diff.layout", "plugin.manager.views.sort"? in Drupal\Component\DependencyInjection\Container->get() (line 153 of core/lib/Drupal/Component/DependencyInjection/Container.php).
The website encountered an unexpected error. Please try again later.

After stepping through, it looks like these form display modes were forcibly installed by Config Update Manager without checking deps.

We have since added the missing dependencies to the install profile and now the site works fine, but that's beside the point. Config Update should be enforcing dependencies on config.

The functionality that has been proposed so far for this issue would not resolve your problem, actually.

It seems like we have several possible problems related to dependencies and config:

a) If you are importing/reverting/deleting a bunch of configuration (such as using the Features module), you want to do the operations in the right order for dependencies. As a note, right now, this module only provides functions to import/delete/revert single config items, so this would be totally new functionality to either do a set together or maybe just sort a set so that you could call the single import/revert/delete in the right order.

b) If you are importing a single config item that has dependencies, this module doesn't check that the dependencies already exist, and doesn't try to also import the dependencies. As a note, I believe this is consistent with the core Config Manager module single import UI page.

c) If you are reverting a single config item that has more dependencies in the on-disk config that you are reverting to than you have in the version that is in the active config, this module doesn't detect that, so you could end up with active config that is missing dependencies. This is similar to (b).

d) If you delete a config item, this module doesn't check that nothing is dependent on the config, so you could end up with active config that is now missing a dependency.

Right now we are not handling any of these problems.

My inclination is that:

a) We should not handle this, unless we add an API for doing multiple imports/reverts/deletes as a set, which is not currently part of this module. Other modules that are using the existing API can do the sorting, and a person using Drush or the UI to import/revert/delete will need to do the operations in the right order. If there is demand for this sorting functionality or for multiple-operation functionality, then we can create a new issue to add this functionality. Adding some related issues that would require this functionality.

b/c) We should probably verify before importing/reverting config, that the dependencies listed in the config on disk exist. If not, we should throw an exception.

d) We should probably verify before deleting config, that nothing in the active config is dependent on it. If there is something depending on it, we should throw an exception.

Also updating issue summary in general.