Problem/Motivation

The features generation form lists existing features in a given bundle regardless of whether they have been enabled.

If attempting to regenerate a set of features including at least one that has not been assigned any configuration (which easily can result if the feature was never enabled on the site), you get the following errors:

Notice: Undefined index: info in Drupal\features\Plugin\FeaturesGeneration\FeaturesGenerationArchive->preparePackage() (line 57 of /path/to/site/modules/features/src/Plugin/FeaturesGeneration/FeaturesGenerationArchive.php).

Recoverable fatal error: Argument 2 passed to Drupal\features\FeaturesManager::arrayMergeUnique() must be of the type array, null given, called in /path/to/site//modules/features/src/FeaturesGenerationMethodBase.php on line 71 and defined in Drupal\features\FeaturesManager->arrayMergeUnique() (line 795 of /path/to/site/modules/features/src/FeaturesManager.php).

These errors result because the info file has not been read into the feature that is being regenerated. Relevant lines in FeaturesManager::addPackageFiles():

    // Only add files if there is at least one piece of configuration
    // present.
    if (!empty($package['config'])) {
      // Add .info.yml files.
      $this->addInfoFile($package);

In this case, there is no configuration to package, so the info file is not added, so later when the generator plugin is trying to merge into the info array that array doesn't exist.

#2599282: Don't generate new empty features is closely related in that it also relates to generating features that lack any assigned configuration.

Proposed resolution

Remove checkboxes for packages that:

  • exist and are disabled, or
  • have no configuration assigned and are not the profile, or
  • are the "unpackaged" pseudo-package [this is already done].

Remaining tasks

User interface changes

API changes

Data model changes

Comments

nedjo created an issue. See original summary.

nedjo’s picture

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

  • nedjo committed 33d029b on 8.x-3.x 
    Issue #2599376 by nedjo: fix error when regenerating empty features.
nedjo’s picture

nedjo
he/him/his
Primary language English
commented
Status: Active » Fixed

  • nedjo committed 9f18b8e on 
    Issue #2599376 by nedjo: object can't be serialized in form.
nedjo’s picture

nedjo
he/him/his
Primary language English
commented
Status: Fixed » Active

Fixed a serialization error I introduced in the previous commit in this ticket.

However, I haven't tested yet but suspect the special casing I've done for the profile leaves a case where the error will still occur: when regenerating an install profile that has no assigned configuration.

mpotter’s picture

mpotter commented
Status: Active » Needs work

Re-opening this one for more work. The comment in the code says:

// Remove checkboxes for packages that:
// - exist and are disabled, or
// - have no configuration assigned and are not the profile, or
// - are the "unpackaged" pseudo-package.

I don't think it is correct to remove checkboxes for packages that "exist and are disabled". If a package has been exported and has Changes detected, we want to be able to bulk-export those packages by checking the box on the main screen.

nedjo’s picture

nedjo
he/him/his
Primary language English
commented

If a package has been exported and has Changes detected, we want to be able to bulk-export those packages by checking the box on the main screen.

On a site other than a site that the feature was originally produced, if a feature is present but was never installed, typically most or all of its configuration will show up as missing. Attempting to regenerate the feature in that state will leave it broken--missing most or all of its configuration. That's why I think we should not be offering to regenerate it.

This leaves the workflow issue: user creates a feature on a site, exports it, makes changes to the site, and wants to regenerate the feature. I think here we should require enabling the feature before regenerating it.

Is there a downside to having to enable a feature before editing it?

mpotter’s picture

mpotter commented

OK, I can understand that issue. I wonder if the test should be to remove the checkbox if any config is "missing" rather than just looking at enabled/disabled.

nedjo’s picture

nedjo
he/him/his
Primary language English
commented

I wonder if the test should be to remove the checkbox if any config is "missing" rather than just looking at enabled/disabled.

Well, it's a valid part of the workflow to reconfigure assignment plugins such that assignments change. Example:

  • Generate a set of features using a bundle.
  • Enable all features in the set.
  • Tweak the assignment plugin configuration.
  • Regenerate all features in the bundle to get their new assignments.

Maybe part of the issue is that what we're calling "missing" covers two distinct cases:

  • Configuration is not present on the site.
  • Configuration is present on the site but hasn't been reassigned to the feature, possibly because it was assigned instead to a different feature.

For now, I think we should keep it as it is: we allow regenerating a feature with "missing" configuration as long as the feature is enabled. We could open a new issue to distinguish between missing and reassigned configuration and handle them distinctly.

If we do more here, we should consider starting by moving logic from the UI to corresponding API methods, since currently we're suppressing checkboxes but not affecting for example Drush calls.

mpotter’s picture

mpotter commented
Status: Needs work » Closed (fixed)

Now that we have improved the handling of "Missing" config by adding the "Import Missing" button to the Edit form, I think removing the checkbox is fine, so closing this issue again as fixed.