So I thought the whole purpose of using our own ConfigInstaller (FeaturesConfigInstaller) was to allow Feature modules to be enabled even if they contain "pre-existing config".

I was playing around with Lightning, and I enabled lightning_media (which is a Feature export). Then I disabled it. When I tried to re-enable it, I got the error "exception 'Drupal\Core\Config\PreExistingConfigException' with message 'Configuration objects [error]
(core.entity_view_mode.media.embedded, core.entity_view_mode.media.media_library, editor.editor.rich_text,
field.storage.media.embed_code, field.storage.media.field_media_in_library, field.storage.media.image,
filter.format.rich_text, services.service_endpoint.lightning_media, user.role.media_creator,
user.role.media_manager) provided by lightning_media already exist in active configuration'"

Perhaps something has changed in core and our custom ConfigInstaller is no longer working? Will investigate further.

CommentFileSizeAuthor
#2 features_profile_preexisting_config-2704181-2.patch916 bytesmpotter
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mpotter created an issue. See original summary.

mpotter’s picture

mpotter CreditAttribution: mpotter at Phase2 commented
Status: Active » Needs review
FileSize
features_profile_preexisting_config-2704181-2.patch916 bytes

Found the problem. I was testing the new "base profile" patch for core here: #1356276: Allow profiles to define a base/parent profile. The lightning_media module I was trying to enable was within the base-profile /module directory. In FeaturesManager::getAllModules() we were using setProfileDirectories ourself to set the profile directory and bypassing the setProfileDirectoriesFromSettings function in core. So it wasn't looking for modules within the base-profile.

Regardless of the status of the core patch, we can improve getAllModules() to account for profile directories in a cleaner way and allow it to add the profile itself. Here is a patch to fix this.

  • mpotter committed a8f1d03 on 8.x-3.x 
    Issue #2704181 by mpotter: Feature not getting enabled because of...
mpotter’s picture

mpotter CreditAttribution: mpotter at Phase2 commented
Status: Needs review » Fixed

Committed in a8f1d03.

Status: Fixed » Closed (fixed)

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

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
CreditAttribution: phenaproxima at Acquia commented

I've been analyzing the getAllModules() method as part of another project, and I'm not sure that I understand its purpose. More specifically, it appears that ExtensionDiscovery already covers everything that the method, as written, does.

  1. static $modules;
    This is going to be static to the class, not the method, which means there can only be one instance of FeaturesManager without potentially causing unintended behavior. Not sure if that is intentional or not, but wouldn't it make sense for this to be a protected class member instead, since FeaturesManager is a persistent service?
  2.       $profile_directories = $listing->setProfileDirectoriesFromSettings()->getProfileDirectories();

    $listing->scan('module') will already do that, so the call is redundant...

  3.       $installed_profile = $this->drupalGetProfile();
      if (isset($bundle) && $bundle->isProfile()) {
        $profile_directory = 'profiles/' . $bundle->getProfileName();
        if (($bundle->getProfileName() != $installed_profile) && is_dir($profile_directory)) {
          $profile_directories[] = $profile_directory;
        }
      }
      $listing->setProfileDirectories($profile_directories);

    ...and apparently unnecessary. $bundle is never defined, as far as I can tell, so nothing in the if block will ever be executed.

Given all these factors, this method could be reduced down to basically one line:

$modules = array_merge($listing->scan('module'), $listing->scan('profile'))

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
CreditAttribution: phenaproxima at Acquia commented
Status: Closed (fixed) » Needs review
mpotter’s picture

mpotter CreditAttribution: mpotter at Phase2 commented
Status: Needs review » Closed (fixed)

Can you please create another issue for this and then reference this as a related issue. The patch in #2 was already committed and there isn't any subsequent patch to review here, so the status isn't correct.