Field Storage isn't automatically selected when a Field is selected

[Disclaimer: I haven't worked much with the custom feature generation code, so my remarks here are about assignment plugins rather than specific UIs.]

This seems to be a missing feature.

We have a dependency package assignment plugin, but currently, for a given config item, it adds dependent configuration. That is, it adds configuration that is dependent on configuration already in a package--not configuration that's required by what's already there.

Concretely: when we've assigned a content type to a package, the dependency plugin should assign configuration that's dependent on that content type, including fields. But, for field storages, the dependency relationship is in the other direction. We would need to assign configuration dependencies. Specifically, this might look like:

• Add a method FeaturesManager::assignConfigDependencies() parallel to the existing FeaturesManager::assignConfigDependents().
• Call this method in the dependency plugin after FeaturesManager::assignConfigDependents().

I implemented a ForwardDependency assignment plugin to do this on our site. Is this roughly what you're looking for?

/**
 * @file
 * Contains \Drupal\ewsite8_export\Plugin\FeaturesAssignment\ForwardDependency.
 */

namespace Drupal\ewsite8_export\Plugin\FeaturesAssignment;

use Drupal\features\FeaturesAssignmentMethodBase;

/**
 * Class for assigning configuration to packages based on forward
 * dependencies.
 *
 * @Plugin(
 *   id = "forward_dependency",
 *   weight = 15,
 *   name = @Translation("Forward dependency"),
 *   description = @Translation("Add to packages configuration depended on by items already in that package."),
 * )
 */
class ForwardDependency extends FeaturesAssignmentMethodBase {
  /**
   * The package assignment method id.
   */
  const METHOD_ID = 'forward_dependency';

  /**
   * {@inheritdoc}
   */
  public function assignPackages($force = FALSE) {
    $config_collection = $this->featuresManager->getConfigCollection();
    $remain = $config_collection;
    while (!empty($remain)) {
      foreach ($remain as $key => $item) {
        // If an item already has a package, we're not interested.
        if ($item->getPackage()) {
          unset($remain[$key]);
        }

        // Only look at items if we've already processed their deps.
        $deps = $item->getDependents();
        $to_process = array_intersect($deps, array_keys($remain));
        if (!empty($to_process)) {
          continue;
        }

        // Ok, we'll process this item.
        unset($remain[$key]);

        // Find packages that claim the dependents.
        $packages = [];
        foreach ($deps as $dep) {
          if (isset($config_collection[$dep])) {
            $dep_item = $config_collection[$dep];
            if ($package = $dep_item->getPackage()) {
              $packages[$package] = TRUE;
            }
          }
        }
        if (count($packages) != 1) {
          // If zero or multiple packages claim a dependent, no way to know
          // what to do.
          continue;
        }

        $package = key($packages);
        $this->featuresManager->assignConfigPackage($package, [ $key ]);
      }
    }
  }

}

Ok, and here's a cleaner implementation, with a test.

The code only assigns a dependency to a package if it's unambiguous what package it belongs in. If items from multiple packages both want to pull it in as a dependency, we do nothing.

@vasi: thanks for digging into this!

A few broader questions we maybe should consider here:

• We have an existing assignment plugin, dependency. Maybe, rather than a new plugin, we could simply add this to the existing dependency plugin, perhaps making the plugin configurable to be able to select one or both kinds of dependency handling (the current kind, which adds dependent items, and the new one, which adds items required by already-assigned items).
• The method FeaturesManager::assignConfigDependents() is at least parallel to what we're introducing here. Could we make the two methods more similar, or even reuse the same method, with an argument that designates the desired kind of dependency handling?
• We call FeaturesManager::assignConfigDependents() in two places besides the dependency assignment plugin. Should we invoke the new dependency handling also in one or both of those instances?
• Should we recurse to handle dependencies of dependencies? See #2677540: FeaturesManager::assignConfigDependents() should recurse.

Thanks for the quick review.

• I'm not sure we should combine these into a single assignment. The user might want to give them different weights, for example. Maybe it would help the confusing naming of assignments? But it might also be even more complicated for the user.
• Hmm, it might be possible to merge the functions. I'm a bit confused about the existing one though, it seems to be missing some important parts. I guess it doesn't care about ordering, because it doesn't do recursion? And how does it deal with multiple packages requesting the same item, the first one just gets it?
• You might not like this, but my feeling is that both the existing calls may be incorrect:
  1. The 'base' assignment calls it, which duplicates the functionality of the 'dependency' assignment. This prevents the user from turning that off.
  2. The features edit form calls it. I'm not entirely sure what for? But again, if the user has disabled the 'dependency' assignment, why should the edit form call this? It's extremely difficult for me to understand that function, though.
• My plugin does do the recursion, the test even checks for it. That's why I do the whole song and dance with Graph and ordering the items, to make sure we see parents before their children. I guess this would get more complicated if we merged the methods, since I'd have to support an $item_names parameter.

@vasi: yeah, overall it seems that your approach here is more refined than what we already have in the dependency assignment plugin and ideally, we'd rework the existing ::assignConfigDependents() method to more closely parallel what you have. But that could be done as followup.

The 'base' assignment calls [::assignConfigdependents], which duplicates the functionality of the 'dependency' assignment. This prevents the user from turning that off.

Agreed that's a kludge. IIRC I put it there because I thought in default settings we needed dependency handling in two places: once immediately after the base features were created and then again as a clean up at the end. Really, that translates into, should it be possible to apply a given plugin multiple times, or to invoke one plugin from another, either of which is out of scope for this issue.

The features edit form calls it. I'm not entirely sure what for?

Me neither. I haven't worked much with the features editing code.

So I think I'm agreeing this deserves a new plugin as you've done. Naming, as you say, is potentially confusing. I'm having a hard time coming up with clear vocabulary to distinguish between the existing and the new case. I suppose the dependency plugin we currently have handles depedees, while the new one handles dependers, but both these terms seem fairly technical. Maybe we could call the new plugin requirements or required to avoid reusing dependency? And/or change the label of the existing dependency plugin to Dependents?

I like "Dependents" versus "Dependencies", that's a good idea.

Some more issues:

• We have to figure out what the default weight of this new assignment should be. I guess it would make sense if it came before Core? Then field storages used by just one content type are taken by that type, and ones used by multiple content types would be taken by Core.
• It looks like the FeaturesBundle::setEnabledAssignments() method works by iterating through the assignments it already has. It looks like that prevents us from adding any new assignments? Maybe I'm missing something, but otherwise that will be trouble.

It looks like that prevents us from adding any new assignments?

Yeah, see #2663692: New assignment plugins not registered to features bundle and the ticket referenced there, #2621658: FeaturesBundle should implement EntityWithPluginCollectionInterface.

We have to figure out what the default weight of this new assignment should be. I guess it would make sense if it came before Core? Then field storages used by just one content type are taken by that type, and ones used by multiple content types would be taken by Core.

I agree that's the expected/desired outcome. The problem with putting it before Core is that we then don't get this dependency handling for items that are added subsequently (including, by default, all items added to the core and site packages).

Currently, the namespace assignment plugin helps address this need. That is, if site builders use a naming convention in which e.g. fields are prefixed with a corresponding content type's machine name (field_article_subtitle for a field that should be used only with the article content type), the field storages will be assigned by the namespace assignment plugin to the correct feature.

So, for now, I suggest we give this new plugin the same weight as the existing dependency plugin (though which of the two should run first I'm not sure...).

Glad to see this issue. I ran into this same issue with field storage and would like to get this into the next release.

I agree with nedjo that having it the same weight as the existing dependency plugin would be good. I also thought about combining the plugins but decided I agree with keeping them separate in case the weight should be different and because there is no reason to have fewer plugins...more modular small plugins is better.

So, I tried testing this. I added a field to the Page content type and then went into Features to export the Page and it auto-detected the Field, but still didn't detect the Field Storage. I *did* go into the bundle config and enable the plugin. Not sure why it's not working then.

Oh, interesting. The problem seems to be that I cannot enable the plugin. When I check the box and click Save in the bundle config, it doesn't save.

Edited: Yep, this issue #2663692: New assignment plugins not registered to features bundle.

OK, I disabled and re-enabled Features to get the plugin recognized just to debug this. The plugin still isn't working.

I added a field "MikeField" to the Page content type (just normal Text field). Then when I edit the Page feature it auto-detects the field instance, but doesn't detect the field storage.

Instead, it added the "node.field_display_summary_on_page" to the field_storage box. Which is weird because that doesn't even look like a field storage item.

Ah ha. So, the order is more important than I thought.

This plugin needs to run *before* the Core plugin because Core has the "field_storage" option selected by default. Thus, all field storage is going into core and is already assigned by the time this new plugin runs.

So this is in conflict with #11. We can't have it both ways.

My opinion is that this "forward dependent" config should be assigned to the package that contains the dependent rather than going into core, site, or other "placeholder" package.

So, patch seems good. We just need to argue about default weight. Since I don't like dumping config into generic packages like "Core", my opinion is to run this before core.

Here is a patch with a better filename and changes the weight to run before core.

Shoot, forgot to add the actual plugin file. Here we go:

Since I don't like dumping config into generic packages like "Core", my opinion is to run this before core.

Well, if we're going to talk about getting rid of the core assignment plugin, let's do so in a dedicated issue. Meantime, we have core, and shouldn't knowingly break it.

While the original post was about field storages, the plugin - and hence the issue - is generic, so we need to be considering all types of shared configuration.

Like view modes. Often, the whole point of a view mode is that it can be used site-wide so that, for example, it's possible to display multiple different content types in a given view.

If we assign forward dependencies before we run core, that view mode may be mis-assigned to the first feature that includes configuration that requires it, rather than to core.

The new plugin partially addresses this problem by not assigning configuration that is required by multiple features. But if we're building out features iteratively (e.g., build one content type, export it, build a second), it will still break the core plugin.

So, unless and until we remove core, we need to weight the new plugin higher than core.

The new plugin partially addresses this problem by not assigning configuration that is required by multiple features.

Exactly! This means it needs to run *before* core. That allows the field-storage of a non-shared field to be added to the module that contains the field, but anything used by multiple features will get passed on, which will then be assigned to Core.

The problem with running it after Core is that core will assign *all* the field storage and won't have anything left for this plugin to process.

I think core should be one of the last plugins to run as being "if it hasn't been assigned by other plugins then let's dump it in core".

If we assign forward dependencies before we run core, that view mode may be mis-assigned to the first feature that includes configuration that requires it, rather than to core.

That will always be the case. Which is actually why our "best practices" here puts views into their own individual features. But speaking of field-storage, this will still always be an issue. Just putting them all into core doesn't solve this problem. Other than name-spacing (which is not followed in many cases) Core has no way to know if it should contain the field storage.

The assignment plugins are never going to be able to handle 100% of all config cases. We'll always need to edit features to tweak stuff as real-life is more complicated than simple feature examples. Devs still need to think about this and make decisions around where config gets organized. No set of assignment plugin rules is going to completely solve that.

All we can do is get as close as possible to the majority of use cases. If people are building features iteratively, then they are already likely using the Edit screen and controlling their exports. Assignment plugins can't read the dev's minds on what might be added iteratively.

In any case, to summarize I still worry about the Core plugin because it blocks anything after it from assigning this config and to me that means it should run later rather than sooner to allow other plugins to have a voice.

Wouldn't it be easier if the disallowed elements were selected according to what has already been written in an enabled Feature instead of trying to predict what could be written if the user wanted to generate everything at once without even thinking about what s/he is doing?

Yes, probably when manually generating a feature we should disallow auto-registration of any new packages. #2660760: Assign configuration according to which packages are requested is related.

It's not even just when exporting to the filesystem. Until a module is enabled you might also want to reassign config across packages that have just been exported.

Selecting the "Allow Conflicts" option on the Edit page is supposed to allow you to select config that is already assigned to another package or already exported. That currently isn't working but is a different issue.

We want to support both cases where A) you just export all the pre-detected config at once from the main Features page, as well as B) where you create/edit each feature manually. Supporting these two cases is tricky but is what we are currently working on.

So we seem to have two goals here:

1. When a feature is based on a content type, we'd like dependencies (like field storage) to come along with it
2. When some config is included in the "core" feature, we'd like its dependencies to come along with it

@nedjo points out that these conflict right now: either the new 'forward_dependency' assignment comes first, or the 'core' one comes first. But they don't have to conflict, it's just an artifact of the way assignments currently work. Some options for fixing this:

1. Change how 'core' works. Currently, for each config type, you can tell 'core' to (1) include it, or (2) not. But it might make sense to have a third choice: include config items of this type only if multiple packages depend on them. This might be a better expression of what 'core' is supposed to be. It still wouldn't deal with incrementally creating features, though.
2. Don't make forward_dependency an assignment, but an attribute of other assignments. Each assignment could have a checkbox 'Add dependencies', that causes dependency resolution to happen for each config item it assigns. Maybe a second checkbox for reverse dependencies? This feels slightly dirty, but only slightly.
3. It might make sense in general for each assignment to potentially have multiple instances in the bundle configuration. That way you could say this:
   • base
   • forward_dependency
   • core
   • forward_dependency

   It would also mean that we could collapse the core/site assignments into one, with a configurable name. This solution would require UI and schema changes, so that's potentially trouble.

I'm sure there are other ways to square this circle. Any ideas?

Also, it might be worth in the meantime getting the forward_dependency assignment committed, so at least it's possible for someone to configure features such that field storage gets pulled in. Then we can add a follow up issue with this more general discussion about priorities.

OK, I am going to mark this as RTBC because the plugin itself is very good and works as designed. The only question left is what weight to give it, which we can address here or in another issue. But I agree we should get this added so people can start to play around with it.

The latest version of the plugin has it going before the core plugin. If people don't like this it's easy to move it or disable it in their own bundle config. But I think it's a good starting default for most sites.

From #24, I think #2 is out...we don't want to need to adjust all the other plugins for this...too messy for my taste. #3 is an interesting idea although figuring out how to implement it cleanly might be an issue. There is an issue to make plugins a ConfigCollection which would preclude this, so it might be difficult.

I think #1 is most interesting to me. Maybe there is a way to improve core to be a bit smarter. As you said, that's sort of the purpose of core as I see it also.

@vasi: thanks for the cogent summary of options and for your work in this issue.

I agree with committing this now and moving remaining questions to a follow-up issue.

Committed to 1bab737.

@mpotter: It looks like you forgot to commit the file FeaturesAssignmentForwardDependency.php !

Shoot, sorry about that. Added the file to commit b5591cd.

Hi, I have the same issue.
I'm selecting the content type and FeaturesUIController's detect method is returning content type fields, view display, and form display, but not returning field storage.

Reproduced in Drupal core version 9.0.7 and Features module version 3.11.