PluginDependencyTrait::calculatePluginDependencies() does not handle theme provided plugins

Can you try this patch?

I had the same issue, I was unable to import configuration into a staging site because my layouts were creating an unsatisfiable dependency to a non-existent module. The patch in #3 fixed the problem. All other configurations exported the same, and my theme was moved to the correct section on the necessary configurations.

The failed tests appear to simply need to be updated to include the required new dependencies for that trait in the testing system.

Updated the tests

+++ b/core/lib/Drupal/Core/Plugin/PluginDependencyTrait.php
@@ -30,13 +44,26 @@ protected function calculatePluginDependencies(PluginInspectionInterface $instan
+      $provider = $definition->getProvider();
+      if ($this->moduleHandler()->moduleExists($provider)) {
+        $this->addDependency('module', $provider);
+      }
+      elseif ($this->themeHandler()->themeExists($provider)) {
+        $this->addDependency('theme', $provider);
+      }
...
+      $provider = $definition['provider'];
+      if ($this->moduleHandler()->moduleExists($provider)) {
+        $this->addDependency('module', $provider);
+      }
+      elseif ($this->themeHandler()->themeExists($provider)) {
+        $this->addDependency('theme', $provider);
+      }
+

Seems like this could stand to be put into its own method.

I don't like that we have to add the module and theme handler to the trait to do this. Is it possible that building a "dependency.calculator" service that does this work for us might be a better solution? Just a thought.

Eclipse

I can't really comment on #8, but I did find the code duplication rather ugly, so I refactored the patch from #7 a little bit.

The patch breaks because of the changes in #2939925: Add a getPluginDependencies() method to \Drupal\Core\Plugin\PluginDependencyTrait.

Yes @eelkeblok - you are right.

This is a continual annoyance for site builders & themers. As soon as you put a Field Layout inside a theme, any new content types you build and export to code must be manually modified else you get an error when sending up to a testing environment and importing the config:

$ drush cim -y
core.entity_view_display.node.typename.default  update
Import the listed configuration changes? (y/n): y
Drupal\Core\Config\ConfigImporterException: There were errors validating the config synchronization. in Drupal\Core\Config\ConfigImporter->validate() (line 728 of web/core/lib/Drupal/Core/Config/ConfigImporter.php).
The import failed due for the following reasons: 
Configuration core.entity_view_display.node.typename.full depends on the themename module that will not be installed after import.

Also, changing the title of the issue to remove the words "theme" and "field layout", while potentially technically more accurate, has had the unfortunate effect of making this issue very hard to find and track down.

Rerolled. Added "theme" back into the issue title.
d.o search also includes info from the issue summary, which already mentions Field Layout.

@jwilson3 can you review?

Stupid mistake on my part.

Had to make the same addition to the other test method

Marked #2964635: Layouts in THEME lead to broken MODULE dependency in configuration as a duplicate. This still needs a review.

Thank you very very much, #20 works great with 8.6.x. GREAT NEWS!
Please let us have more feedback from the others here. RTBC +1 from me.

Is it possible to make the patch available for 8.5.x to already use it until 8.6.x will be released or are there other important dependencies? Currently the patch doesn't apply on 8.5.x

#10 should work for 8.5.x
This should be a candidate for backport, IMO
But that happens after this gets in for 8.6.x

The change to PluginDependencyTrait looks good. Just a couple test questions.

Re #8 I think that could be follow up if needed. Not sure where else the new service would be needed.

1. Should we update \Drupal\Tests\Core\Config\Entity\ConfigEntityBaseUnitTest::providerCalculateDependenciesWithPluginCollections() to provide a test case the dependency is a theme?

2. +++ b/core/tests/Drupal/Tests/Core/Plugin/PluginDependencyTraitTest.php
   @@ -5,6 +5,8 @@
   +use Drupal\Core\Extension\ThemeHandlerInterface;
   

   This is actually an unused use statement.

   Maybe you had meant to add theme test case?

   We should probably add theme test case in \Drupal\Tests\Core\Plugin\PluginDependencyTraitTest::providerTestPluginDependencies

1) #10 doesn't apply to 8.5.3

~/App/Drupal/drupal-8.x-dev (8.5.x=)
$ git apply 2904550-plugin_dependency-20.patch
error: patch failed: core/lib/Drupal/Core/Plugin/PluginDependencyTrait.php:16
error: core/lib/Drupal/Core/Plugin/PluginDependencyTrait.php: patch does not apply
error: patch failed: core/tests/Drupal/Tests/Core/Plugin/PluginDependencyTraitTest.php:23
error: core/tests/Drupal/Tests/Core/Plugin/PluginDependencyTraitTest.php: patch does not apply

2) When I manually reroll the patch for Drupal 8.5, it is giving a namespace error looking for non-existent 'Drupal\Core\Plugin\NestedArray'. It looks like this has already been fixed in #2939925: Add a getPluginDependencies() method to \Drupal\Core\Plugin\PluginDependencyTrait, but that issue is not slated for backport. So I'm not sure if I should add the required use Drupal\Component\Utility\NestedArray; to a backport patch here or what. :confused:

3) After fixing the above, then re-saving my node's "Manage Display" tab, /admin/structure/types/manage//display/full and export with drush cex and check the diff, it no longer tries to add my theme as a dependency (yay!), however there are a number of unrelated module dependencies also removed. For example this is the diff:

   module:
-    - address
-    - daterange_compact
     - field_layout
-    - layout_discovery
-    - themename
-    - user

The Address and DateRange_Compact dependencies were previously added because the content type has various fields with configurations that are using those modules. I would expect the layout_discovery would still be needed because the theme is using custom layouts, but maybe this is already covered by the field_layout dependency (?). Not entirely sure why the user dependency was there in the first place, so maybe that one is okay to remove.

I don't know if the removal of these other modules is a symptom of the deep merge / nestedarray functionality in PluginDependencyTrait.php or what.

4) Finally, I would have expected there to be a dependency section added for the theme, with my themename added, but this was not the case.

I hope this might help @tim.plunket identify what more there might be to do here? (Also, agree with @tedbow that there should be a test of the theme provider.

I guess I'll add my D8.5 backport here if someone wants it, but please be aware of the caveats mentioned in the previous comment.

1. +++ b/core/lib/Drupal/Core/Plugin/PluginDependencyTrait.php
   @@ -29,24 +44,69 @@
   +      $provider = $definition->getProvider();
   ...
   +      if ($definition instanceof DependentPluginDefinitionInterface) {
   +        $config_dependencies = $definition->getConfigDependencies();
   

   I think provider should be set only when any dependencies returned by the provider

2. +++ b/core/lib/Drupal/Core/Plugin/PluginDependencyTrait.php
   @@ -29,24 +44,69 @@
   -      $this->addDependency('module', $definition['provider']);
   +      $provider = $definition['provider'];
   ...
          if (isset($definition['config_dependencies'])) {
   -        $this->addDependencies($definition['config_dependencies']);
   +        $config_dependencies = $definition['config_dependencies'];
   

   same here

@andypost I disagree, and that is a BC break of sorts. This is not about changing the logic of this method.

Posting 8.5 backports will just delay this from being backported. It needs to stay up to date for 8.6 first.

Here are tests as suggested by @tedbow

Confirming #28 works great. RTBC+1!
It would be nice to have this in 8.6 finally. Or should we even add it to an 8.5.x minor release?

Re #28 thanks for adding the test case.

+1 on RTBC

It's possible to for core to provide plugins. The examples are very limited at the moment - some typed data plugins and entity types but I think we need to just put it in module as we would have done. We add 'core' to the list of enabled modules in \Drupal\Core\Config\ConfigInstaller::getEnabledExtensions because of this. So let's just do something like:

+++ b/core/lib/Drupal/Core/Plugin/PluginDependencyTrait.php
@@ -33,20 +47,40 @@
+      if ($this->moduleHandler()->moduleExists($provider)) {

Let's do something like if ($provider === 'core' || $this->moduleHandler()->moduleExists($provider)) {

And add test coverage for this.

Quickfix backport from patch in #28 for Drupal 8.5 attached to make it work when you need an import.
Important: This is a QUICKFIX, no final solution!

Using this patch, I get this annoying strict warning.

Does it make sense to rename the property in the trait? Whatever we come up with, we can't guarantee it won't clash with a property in another composition (for custom code), but at least for this one we know it clashes. Would it make sense to "namespace" properties in a trait?

Can we somehow ensure that old, invalid configuration is updated when this patch becomes part of core?

Is the strict warning only on PHP<7.0? http://php.net/manual/en/language.oop5.traits.php#example-214
According to the documentation, that STRICT shouldn't be thrown anymore if the definitions are compatible.

We're using the patch #28 since more than two month now. Is #31 by @alexpott the last remaining point?

@alexpott, are you talking about these lines:

    if ($provider) {
      if ($this->moduleHandler()->moduleExists($provider)) {
        $dependencies['module'][] = $provider;
      }
      elseif ($this->themeHandler()->themeExists($provider)) {
        $dependencies['theme'][] = $provider;
      }
    }

suggesting to change them to

    if ($provider) {
      if ($provider === 'core' || $this->moduleHandler()->moduleExists($provider)) {
        $dependencies['module'][] = $provider;
      }
      elseif ($this->themeHandler()->themeExists($provider)) {
        $dependencies['theme'][] = $provider;
      }
    }

?

Is it correct to handle core as module dependency here? I'm not deep enough into core to see possible side effects.

On the other hand I think we should finalize this issue soon because it breaks config import when using layouts in theme for example, which is quite bad. I wish we had solved this before 8.7.

I'd like to help but I think I'd need some assistance.

Thank you.

@alexpott: Any feedback? It would be nice if you could have a look so that we can get this fixed in 8.7 finally. Thank you very much.

#38 is exactly right. Updated the test as well.

Thank you @tim.plunkett. With #40 I still run into the issue that a layout (core) applied to a node display (core.entity_view_display.node.news.card.yml) which is provided by the theme. It is detectet as "module" dependency and leads to failing configuration import because that "module" is not existing of course.
I'll try to find out why this happens. Perhaps its an issue in layouts. With the quickfix in #33 it worked in 8.5.x, but your patch looks correct. I'll have a look why "core" is returned. A module with the same name definitely doesn't exist.

Whao!! After debugging for two hours I finally found the reason... The patch WORKS! But in my case (using *.layout.yml's defined in a theme which are used in node displays) I had to re-save all node displays to get correct results in export!

I guess that the dependencies are "inherited" somehow or cached on entity basis (perhaps @tim.plunket knows the reason?) so that the old "module" key was used instead of "theme". Clearing cache (drush cr) didn't change anything.

Now everything is fine for me, but it would be perfect to find out the reason behind and handle this case in an update hook, otherwise this issue will be re-opened several times, I guess, if other users run into the same trap...

THANK YOU SO MUCH!! #40 RTBC ... but to be continued ;)

I also ran into this problem when using layouts defined in *.layout.yml within my custom theme. It is good to know that this problem does not just occur for node displays, but for any entity view display. In my case this occurred in the default display of a custom block. (block_content entity).

I tried fixing it by defining the theme dependency myself in `core.entity_view_display.block_content.*.default.yml` and re-installing my site with this configuration as default, but that didn't help either.

After applying #40 it works as expected, and my theme is no longer listed as a module dependency, but a theme dependency :)
Good to know: #40 also applies on 8.6

Here's an update path.

Need to check for the module first.

Thank you tim.plunkett, works perfectly now. RTBC+1 - will this be part of a 8.6.x minor release or 8.7.x?

It will be in 8.7.x first, and then it's up to the committers to backport it for a 8.6 minor if they so choose.

But someone has to RTBC it first.

lgtm

shoot, it _looked_ right but testing on a site the update path broke. What's happening in the entity system hear isn't clear to me but here's the trace:

Error: Call to a member function getDependencies() on null in Drupal\Core\Config\Entity\ConfigEntityUpdater->Drupal\Core\Config\Entity\{closure}() (line 103 of /var/www/html/web/core/lib/Drupal/Core/Config/Entity/ConfigEntityUpdater.php) 
#0 [internal function]: Drupal\Core\Config\Entity\ConfigEntityUpdater->Drupal\Core\Config\Entity\{closure}(Object(Drupal\field_layout\Entity\FieldLayoutEntityFormDisplay))
#1 /var/www/html/web/core/lib/Drupal/Core/Config/Entity/ConfigEntityUpdater.php(110): call_user_func(Object(Closure), Object(Drupal\field_layout\Entity\FieldLayoutEntityFormDisplay))
#2 /var/www/html/web/core/modules/system/system.post_update.php(186): Drupal\Core\Config\Entity\ConfigEntityUpdater->update(Array, 'entity_form_dis...')
#3 /var/www/html/vendor/drush/drush/src/Commands/core/UpdateDBCommands.php(272): system_post_update_recalculate_dependencies_for_theme_provided_plugins(Array)
#4 /var/www/html/vendor/drush/drush/includes/batch.inc(235): Drush\Commands\core\UpdateDBCommands->updateDoOnePostUpdate('system_post_upd...', Object(DrushBatchContext))
#5 /var/www/html/vendor/drush/drush/includes/batch.inc(183): _drush_batch_worker()
...

Field Layout incorrectly overrode ::calculateDependencies() without returning $this

I ran into that too, just finished tracking it down. Re-testing with #53 now

Okay, I've had a site running with #28 in production for some time, currently on 8.6.1. I spun up my dev copy, removed #28, re-saved a couple configs I knew to be problematic and exported. I confirmed that the theme dependency was incorrectly appearing under modules with no patch in the exported configs. I then applied #53, ran updates and exported again, and I can confirm the configs are exporting with the correct theme dependencies again without having to re-save them, so the upgrade path works.

This solution is based on the well tested solution in #28. It passes tests. I can confirm the upgrade path works. As such, I'm resetting to RTBC.

As this is a bugfix, pretty important to the layout initiative, and we can confirm the patch applies to both 8.6 and 8.7, hopefully the committers can backport this for 8.6.2

Confirming RTBC of #53 on 8.6.1. Would be nice to see this in the next core release.

+++ b/core/modules/system/system.post_update.php
@@ -169,3 +169,20 @@ function system_post_update_extra_fields(&$sandbox = NULL) {
+/**
+ * Recalculate dependencies for theme-provided plugins.
+ */
+function system_post_update_recalculate_dependencies_for_theme_provided_plugins(&$sandbox = NULL) {
+  // The only theme-provided plugin types in core are layouts and breakpoints.
+  $config_entity_updater = \Drupal::classResolver(ConfigEntityUpdater::class);
+
+  // Breakpoint plugins are stored by responsive image styles.
+  if (\Drupal::moduleHandler()->moduleExists('responsive_image')) {
+    $config_entity_updater->update($sandbox, 'responsive_image_style');
+  }
+
+  // Layout plugins are stored by entity displays.
+  $config_entity_updater->update($sandbox, 'entity_form_display');
+  $config_entity_updater->update($sandbox, 'entity_view_display');
+}

I think each of these needs to be either a separate post update or we need to do something clever with the batch. Since in there are 100s of responsive image styles and we stop processing them then we'll immediately start processing the entity_form_display and then entity_view_display and the sandbox info will be all messed up.

Also as far as I can see we have no upgrade path tests.

Just to add, I applied the patch in #53 and this solved the issue for me.

I did have to re-export my configuration, by making a change -> exporting -> change back -> export for each content type I was using my custom layout on. Once I did that this I was able to import my configuration after I had made changes and verified that the import was back to how I had set it with no error messages in Drupal Console.

+++ b/core/modules/system/system.post_update.php
@@ -169,3 +169,20 @@ function system_post_update_extra_fields(&$sandbox = NULL) {
+  // Layout plugins are stored by entity displays.
+  $config_entity_updater->update($sandbox, 'entity_form_display');
+  $config_entity_updater->update($sandbox, 'entity_view_display');

This code also should run only if layout_discovery module enabled

Patch adds the conditional requested in #60, and splits the post_updates into two separate functions requested in #58 (see interdiff). I did not add upgrade path tests, therefore after automated tests finish, this probably should be set back to needs work.

Fixed the mis-matched doc blocks (facepalm).

Setting to needs work for upgrade path tests.

The patch provided in #63 doesn't seem to work. On updb we get an

Error: Call to a member function getDependencies() on null in /var/www/.../web/core/lib/Drupal/Core/Config/Entity/ConfigEntityUpdater.php on line 103

I went to go write the update path tests for this and found the response image style dependencies are already correctly handled in: \Drupal\breakpoint\BreakpointManager::getGroupProviders:

langcode: en
status: true
dependencies:
  config:
    - image.style.large
    - image.style.medium
  theme:
    - breakpoint_theme_test
id: test_responsive_image
label: 'Test responsive image'
image_style_mappings:
  -
    breakpoint_id: breakpoint_theme_test.narrow
    multiplier: 1x
    image_mapping_type: image_style
    image_mapping: medium
  -
    breakpoint_id: breakpoint_theme_test.mobile
    multiplier: 1x
    image_mapping_type: image_style
    image_mapping: medium
breakpoint_group: breakpoint_theme_test
fallback_image_style: large

Also, seems like we can simply move the post update hooks into "layout_discovery" itself, to skip the moduleExists check. Also, as @alexpott pointed out we need an update hook per entity type, so splitting entity_form_display and entity_view_display into two.

Adding the upgrade path test. Somehow the fix @tim.plunkett wrote in #53 went missing too, which explains #65. Adding that back in.

Fixing a docblock.

+++ b/core/lib/Drupal/Core/Plugin/PluginDependencyTrait.php
@@ -33,20 +47,40 @@
+    // Add the provider as a dependency, taking into account if it's a module or
+    // a theme.
+    if ($provider) {
+      if ($provider === 'core' || $this->moduleHandler()->moduleExists($provider)) {
+        $dependencies['module'][] = $provider;
+      }
+      elseif ($this->themeHandler()->themeExists($provider)) {
+        $dependencies['theme'][] = $provider;
       }
     }

This is a slight change in the logic here. ATM in HEAD we assume it's a module an don't check. Now we won't set the provider if both $this->moduleHandler()->moduleExists() and $this->themeHandler()->themeExists() return false.

I think maybe we should consider the following code:

if ($provider) {
  // Themes can provide plugins. If it is not a theme assume it is a module.
  $provider_type = $this->themeHandler()->themeExists($provider) ? 'theme' : 'module';
  $dependencies[$provider_type][] = $provider;
}

And then we can remove the moduleHandler property and service.

I understand the desire to not break HEAD's code flow.
However, that flow makes little sense to me.

What about adding an else case that handles that assignment, but with either logging or a trigger_error?
If your provider is neither, shouldn't that be something you would want to know?

Poking this again. Perhaps if something is neither a module or a theme, we should just throw a helpful exception? It's probably better to weed out weird situations like that instead of silently assuming 'module'?

Here's #72. Aka, providing BC while discouraging the flawed code path.

Just some PHPCS fixes. I don't know how I feel about the BC discussion but otherwise I like where this is at.

Just tested the Patch at #75 on 8.6.10 and this worked ok.
(Was able to export and import my config with Layout from my theme).

I just tested the patch from #75.
I too was able to import and export my config, including layouts in my theme, without errors.
However my theme still gets listed as a a 'module' dependency in the exported config yml files. I am not sure if this is the intended solution? Semantically it feels wrong.

The updb issue from #65 seems fixed in the patch from #75.

Hi @K3vin_nl,

thank you for your important feedback! You hit a critical point:

However my theme still gets listed as a a 'module' dependency in the exported config yml files. I am not sure if this is the intended solution? Semantically it feels wrong.

please try #40 and see if that's fixed there. It was the main reason for the patch for us and we're still using #40. That part should definitely be solved in this issue and the latest rerolls!
Otherwise there are problems on import, for example when defining layouts in a theme. See #41 and #42 for details.

@Anybody,
I did a config export and subsequent import with the patch #75. Although my theme was still listed as a module dependency, the import didn't complain about not being able to find a module 'my_theme' as before and succeeded. So it seems the patch 'works'.

+++ b/core/modules/layout_discovery/tests/src/Functional/Update/LayoutDiscoveryDependenciesUpdateTest.php
@@ -0,0 +1,57 @@
+      $this->assertTrue(in_array('test_layout_theme', $dependencies['module']));
+      $this->assertFalse(isset($dependencies['theme']));
...
+      $this->assertFalse(in_array('test_layout_theme', $dependencies['module']));
+      $this->assertTrue(in_array('test_layout_theme', $dependencies['theme']));

The latest patch should absolutely be updating displays from having themes declared as module dependencies, to being correctly declared as themes (see the following test cases).

Are you able to double check what was going on in your case?

Other than that, the latest approach seems okay to me, but @alexpott should make the call given he raised concerns about it initially.

Moving back to an 8.7 issue, hopefully this can still make it.

I've read through the code and the last few comments, I believe the concerns were addressed but still up for @alexpott to decide.

Currently without the patch, importing config changes throws this, even though that was exported, so I agree it's major.

[ERROR] There have been errors importing: Configuration
core.entity_view_display.block_content.card.default depends on the
THEMENAME module that will not be installed after import.

My export now shows the theme as a dependency which is great, not sure what happened with #80. I got this in my config export:

+  theme:
     - MYTHEME

RTBC for #75

Thank you for your review, @joelpittet, I completely agree RTBC.#75

Queued a test on 5.5/5.6 because of the strict warning mentioned by @DuaelFr

Added issue credits

Fails on 5.5 and 5.6

Ughhhhh. Too true.

Fixing test.

LGTM

Queued 5.5 and 5.6

Committed 8847936 and pushed to 8.8.x. Thanks!

c/p to 8.7.x as this is a major bug with hopefully no disruption

Whao!!! Great news, thank you all so much for working on this! GREAT :)