The logic that automatically adds configuration dependencies to a feature seems to be a bit buggy. I'm not sure if this is a problem with features, config update, or the core config system.

Quite frequently when creating a new feature or updating an existing feature, we see Features automatically selecting dependencies that are most definitely not related. For instance, we have two completely independent content types on our site living in two separate features, and yet whenever we try to re-export one of the features, Features wants to include both of the content types and all of their fields. This leads to confusion at best, and conflicts at worst (the same content types and fields living in multiple features simultaneously).

So there seems to be at least two things wrong with the config dependency detection logic... it's incorrectly detecting some dependencies (false positives), and also not detecting when a dependency would produce a conflict (even if feature is set to not allow conflicts).

Sometimes this can be "fixed" by re-triggering the detection logic by simply selecting or deselecting any component for the feature... it will then come to its senses and get rid of the incorrect and conflicting dependencies. Sometimes, but not always.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Dane Powell created an issue. See original summary.

Dane Powell’s picture

I think part of what's happening here is that "reverse" dependencies are being included. For instance, if a feature exports a field, Features is also trying to include views that depend on that field. I think this is caused by #2724835: Improve ConfigDependencyManager::getDependentEntities() documentation

mpotter’s picture

That could indeed be the cause. Features itself just depends on the core dependency info. Anything that is marked as dependent on the config in the Feature will get added by the Dependency assignment plugin. So if the view has a config dependency on the field, the view will get auto-detected and added.

But I'm more interested in your original post talking about two different content types that seem cross-related. Can you give more information on how to reproduce this with content types and fields?

darrell_ulm’s picture

Also seeing this, and working on getting some concrete examples.

nedjo’s picture

We have two assignment plugins that focus on dependencies: FeaturesAssignmentDependency ("Add to packages configuration dependent on items already in that package") and FeaturesAssignmentForwardDependency ("Add to packages configuration on which items in the package depend"). The latter was added in #2666836: Field Storage isn't automatically selected when a Field is selected.

This bug may be caused by the interaction of these two plugins, either in a single or in successive runs of the assignment plugins.

Dane Powell’s picture

Here's one of many ways to reproduce this that addresses Mike's question about content type cross-talk.

  1. On a fresh install, create a content type foo and a content type bar.
  2. Add a new field to foo, and add the same field ("re-use existing field") to bar.
  3. Go to the features export page to featurize content type foo (admin/config/development/features/edit/foo). Note that by default, features is only exporting configs directly required by foo, as expected. It's not exporting the field storage (probably because the bundle wants to put it into a core feature).
  4. Choose to also export the field storage for your new field. Note that as soon as you check the box for the field storage, the feature auto-includes the field instance, entity view display, and entity form display for content type bar. That's definitely not expected behavior!

This could pretty clearly be caused if getDependentEntities() is returning both forward an backward dependencies.

mpotter’s picture

Thanks for that procedure. We also just ran into this locally on a client project, so there is definitely a problem. Will look into it today.

mpotter’s picture

OK, so this actually logically makes some sense:

1) When you add an item to a Feature, it auto-detects any other items that are dependent on the added item. e.g., if you add a content-type, it adds the fields that depend on the content type.

2) In this case, we are adding a field-storage, so it auto-detects any items that are dependent on the field-storage, which is all of the field-instances.

3) So it adds both the foo and bar field instances because neither of those have been exported to any other module yet.

Thinking of this another way: If I was creating a standalone feature for this field, I would add the field-storage and I'd expect it to auto-detect all usages of that field-storage, which would be all of the different field-instances.

If the "bar" field instance had already been exported to the "bar" feature, then this wouldn't be happening. It's only because it's not exported anywhere else yet that it gets added as an auto-detect.

So, I need to think about how to fix this case without breaking the fundamental way in which features is meant to work.

mpotter’s picture

Nedjo: Could you comment on this if you can?

I'm trying to figure out if this is "works by design" even though it's really annoying. I can't determine what the difference is between adding a content-type to the feature (where you EXPECT it to auto-detect the fields of the content type) vs adding a field-storage (where you DONT want it to add all the shared field instances).

Also, this doesn't have anything to do with any assignment plugins. The code to add the auto-detect items to the UI is handled in FeaturesUIController it its own getConfigDependents() function.

But field-instances depend on the field-storage, just as field-instances depend on the content type. Since adding a content type should add its fields, adding a field-storage is also going to add its fields. This is not a "forward dependency"...it's a normal config dependency.

mpotter’s picture

Also confirmed that in #6, if you export both the "foo" feature and the "bar" feature before trying to add the field-storage in step 4 then it works fine (does not add the bar field to the foo feature)

Dane Powell’s picture

I'm pretty sure that even if a configuration has already been exported to another feature, Features will still try to re-export it if it detects it as a forwards or backwards dependency. At least in some cases.

mpotter’s picture

Dane: That's what we need to try and find a way to reproduce. I'm not saying that it doesn't happen, but we need a procedure for triggering the problem so we can debug and fix it and also write a test for it.

mpotter’s picture

OK, learned some more about this.

After exporting *both* foo and bar, adding the field-storage to "foo" doesn't auto-detect or add any bar config.

However! If I then export the new "foo" module containing the field-storage, then re-edit the "foo" feature, then I see the "bar" fields being added as auto-detects.

So now I think I am back to looking at the assignment plugins rather than the handling of the checkboxes in the Edit form.

mpotter’s picture

OK, found the source of this bug I think. Now I just need to think of a solution.

In the FeaturesEditForm, when building the checkboxes, it calls:

$this->featuresManager->assignConfigDependents($package_config, $package_name);

in order to auto-detect dependent config. But since it specifies a specific $package_name, it triggers this code in assignConfigDependents():

              // If a Package is specified, force assign it to the given
              // package.
              $this->assignConfigPackage($package_name, [$dependent_item_name], !empty($package));

Which forces the "bar" fields to be assigned to the "foo" feature even though they are already exported. I'll need to review the code history to see why we are forcing config to be reassigned here. I know the assignConfigPackage() routine was refactored fairly recently and perhaps that caused this to break or perhaps we don't need to force it anymore.

mpotter’s picture

Status: Active » Needs review
FileSize
3.29 KB

OK, here is a patch attempt. Since the refactored assignConfigPackage() seems to handle stuff better now, I removed the "force" when specifying a package. This is only used when adding dependencies in the Edit form, so shouldn't have side effects.

Then, in the handling for the Edit Form, I also found that if the field storage is added on the initial feature creation form, it could remove some of the auto-detected items initially added to the package, so added code to preserve any existing auto-detected items.

NOTE: If neither foo or bar are exported yet and you add the field-storage to foo, it will still auto-detect and add the "bar" fields, which is just how this stuff works (same as adding the fields when you select a content type). But in my testing, if the "bar" package is already exported this will no longer happen.

Dane Powell’s picture

So far that patch in #15 is working wonders for us. Obviously it's a complex problem and it's hard to say that it's 100% effective, but we're seeing far fewer false dependencies (possibly none). This makes me very happy :) We'll bang on it a bit more and report back.

mpotter’s picture

Status: Needs review » Reviewed & tested by the community

Thanks for testing Dane. Going to RTBC this one for now since I think it's important to get a new release asap with this.

  • mpotter committed 37abbca on 8.x-3.x
    Issue #2720167 by mpotter: Config dependency detection logic
    
mpotter’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 37abbca.

Status: Fixed » Closed (fixed)

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