Problem/Motivation

Currently we analyze the extension dependencies of configuration assigned to a package and add package dependencies on those extensions.

However - for default config in the config/install directory, at least - there is a dependency on the module that provides the configuration entity type, which typically is not included in the declared dependencies of a given configuration type.

For example, a contact form might be exported as follows:

langcode: en
status: true
dependencies: {  }
id: feedback
label: 'Website feedback'
recipients:
  - admin@example.com
reply: ''
weight: 0

The feature to which this item is assigned won't get a dependency on the contact module, and will generate an install error when installed (unless the contact module is already installed).

Proposed resolution

Where config of given types are assigned to a package, add dependencies on modules that provide the configuration types.

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#17 2653046-17.patch1.96 KBmpotter
#8 2653046-8.patch2.08 KBnedjo
#6 interdiff.txt2.67 KBnedjo
#6 2653046-6.patch4.47 KBnedjo
#5 2653046-5.patch2.35 KBnedjo

Comments

nedjo created an issue. See original summary.

nedjo’s picture

nedjo
he/him/his
Primary language English
commented
Issue summary: View changes
dawehner’s picture

dawehner
Primary language German
commented 
[23:05:17]  <dawehner>	I see no config dependency used in any config entity typ
[23:05:19]  <dawehner>	e
[23:05:33]  <dawehner>	I think its up to the module developer to take care about that when its part of a module
[23:05:54]  <dawehner>	or when its part of the entire site config, this is always taken care of, by having a full set of core.extension
[23:06:06]  <dawehner>	so IMHO it is the right thing to deal with that in features

, so a humble +1 here.

nedjo’s picture

nedjo
he/him/his
Primary language English
commented
Issue summary: View changes

Updating per discussion with dawehner.

nedjo’s picture

nedjo
he/him/his
Primary language English
commented
Priority: Normal » Major
Status: Active » Needs review
StatusFileSize
new 2653046-5.patch2.35 KB

Escalating to major since generated features can't be installed.

Approach in attached patch: when assigning a configuration item to a package, determine the provider of the item's configuration type and, if the provider is an installed module (and not, for example, 'core'), add it as a dependency.

nedjo’s picture

nedjo
he/him/his
Primary language English
commented
StatusFileSize
new 2653046-6.patch4.47 KB
new interdiff.txt2.67 KB

This patch adds testing, not yet for this issue, but for the existing dependency handling (add dependency on module that the config item depends on).

  • nedjo committed 69491eb on 8.x-3.x 
    Issue #2653046 by nedjo, dawehner: Missing dependency on module...
nedjo’s picture

nedjo
he/him/his
Primary language English
commented
StatusFileSize
new 2653046-8.patch2.08 KB

I went ahead and committed this because it's the last blocker to a much-needed alpha release.

Attached patch is a draft of a test. So far it's failing. In FeaturesManager::assignConfigPackage() this line

            if (isset($module_list[$provider])) {

is evaluating to FALSE when it should be TRUE. I'm attempting to get the test to return ['my_module' => NULL] for $module_list, see this part of the patch:

+    $this->moduleHandler->expects($this->any())
+      ->method('getModuleList')
+      ->willReturn(['my_module' => NULL]);

Help appreciated!

Status: Needs review » Needs work

The last submitted patch, 8: 2653046-8.patch, failed testing.

mpotter’s picture

mpotter commented

Not sure this is actually working. I exported some field_storage to a new feature and none of the module dependencies from the config were added to the feature itself. Will look into this a bit more.

mpotter’s picture

mpotter commented

From #3:

I think its up to the module developer to take care about that when its part of a module

No, I disagree with this. If Features is creating a module containing config, the dependencies from the config need to be added to the module. If the module developer wants to remove this for some reason they can. But many people using Features to organize their config are not module developers and just want to be able to enable the module they just created.

Trying to use Features in a new D8 distro that we are building for a client, this lack of dependency info became a huge issue and a lot of manual work.

So, what was the rationale for saying that Features should *not* add the dependencies to the exported module?

mpotter’s picture

mpotter commented

So, I think maybe this issue is talking about something different. The code in patch #5 is trying to add a dependency of a module that contains dependent config. And it's possible this is working.

But what isn't working is when the config item itself contains dependencies. For example, a taxonomy reference field_storage has a dependency on the "taxonomy" module. The "taxonomy" module is not providing any config, but is a dependency. If I generate a Feature Module A that contains this field_storage, it's important to add "taxonomy" as a dependency to this feature module. Otherwise during a site install Drupal can attempt to enable my Feature Module A *before* Taxonomy is enabled, causing UnMetDependencies errors.

mpotter’s picture

mpotter commented

To test this:

1) Create new feature
2) Add a field_storage for a field that has a module dependency
3) Generate the feature

the features.info.yml doesn't have any dependency in it.

Seems like the dependencies are set in the FeatureManager::assignConfigPackage() function. But this function isn't called when adding new items to a new feature?

nedjo’s picture

nedjo
he/him/his
Primary language English
commented

@mpotter: yes, let's get a new issue filed for this, since it's a separate problem.

As you note, it's supposed to be handled by these lines in FeaturesManager:assignConfigPackage():

          // For configuration in the InstallStorage::CONFIG_INSTALL_DIRECTORY
          // directory, set any module dependencies of the configuration item
          // as package dependencies.
          // As its name implies, the core-provided
          // InstallStorage::CONFIG_OPTIONAL_DIRECTORY should not create
          // dependencies.
          if ($config_collection[$item_name]->getSubdirectory() === InstallStorage::CONFIG_INSTALL_DIRECTORY && isset($config_collection[$item_name]->getData()['dependencies']['module'])) {
            $module_dependencies = array_merge($module_dependencies, $config_collection[$item_name]->getData()['dependencies']['module']);
          }
mpotter’s picture

mpotter commented

OK, moved to #2687245: Config dependencies not getting added to Feature export.

Leaving this issue open to review the work needed to get the patch working as reported as failing in #9

mpotter’s picture

mpotter commented
Priority: Major » Normal

The "Major" issue was committed in #7 and this issue just remains for getting the test working.

mpotter’s picture

mpotter commented
Status: Needs work » Needs review
StatusFileSize
new 2653046-17.patch1.96 KB

Here is a patch with a fixed test. We needed to change the return expectation of getModuleList so the isset() would be true.

  • mpotter committed 81679b1 on 8.x-3.x 
    Issue #2653046 by nedjo, mpotter: Missing dependency on module providing...
mpotter’s picture

mpotter commented
Status: Needs review » Fixed

Committed to 81679b1.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.