[regression] Plugins extending from classes of uninstalled modules lead to fatal error during discovery

I can confirm that we experienced the same problem in the Entity browser -> File entity area (file entity extends field widget that entity browser provides).

This issue is indeed very limiting in cases where a module wants to extend a plugin but doesn't necessarily declare the original module as a hard dependency.

This is affecting the core Migrate API as well: https://www.drupal.org/node/2560795#comment-11590071

I suspect even a class_exists won't help because when the class loader loads the class file php fatals.

The road ahead is very similar to the unicorn issue phenaproxima linked above: when parsing the annotation , use the classloader as the class finder instead of the mock finder and there you can filter out plugins extending from nonexistent classes. Won't help implementing nonexisting interfaces...

Here's a revert patch for #2763157: Allow plugins to provide multiple forms although I think outside in is using this already.

Confirming my suspicion in #4, changing to class MachineNameController extends broken implements ContainerInjectionInterface { and running drush ev 'var_dump(class_exists("Drupal\system\MachineNameController"));' results in a fatal error. PHP, never change, my life would be dull.

I think the simplest is to remove the fallback code, because:

a) the 'configure' is very specific to blocks and not generic anyway, so it does not apply to 99% of plugins. Also it is a very magic fallback, so can do that as well in something that is more block oriented.

b) This can be done easily at run time, too:

    if ($this->hasFormClass($operation)) {
       // ...
    }
+    else if ($operation == 'configure' && $this instanceof PluginFormInterface) {
+     return get_class($this);
+   }

That is all that is needed IMHO to keep BC and fix the issue.

Because the only new class in the original patch that called ->getPluginForm() instead of getPlugin() is BlockForm.

Hence there is no BC break for other code as it is either unchanged or if it changed to the new method it did not depend on the fallback in the first place.

And if there is code in contrib/ that already changed things like Block, then it will need to be changed again - as the 8.2.x-dev is not yet released anyway.

The @Fabianx suggestion in patch form (primary credit should go to him if this approach is committed).

"What about tests?" you ask? Well, we already have a test in core which should be failing for this case: #2797269: Improve MigrationPluginListTest. It appears that autoloading under phpunit is overly generous - I think #2536610: Ensure all modules are autoloaded with PSR-4 regardless if enabled or not is the issue to address to make this testable.

I agree with this solution.

Yeah, need to patch hasFormClass() as well obviously:

+  public function hasFormClass($operation) {
+    return isset($this->getPluginDefinition()['forms'][$operation]);
+  }

but that is tricky, because getFormClass calls hasFormClass(), so the code will need to change like:

+  /**
+   * {@inheritdoc}
+   */
+  public function getFormClass($operation) {
+    if ($this->hasFormClass($operation)) {
+      $plugin_definition = $this->getPluginDefinition();
+      // Fallback to the class of this plugin if the key is not set.
+      return isset($plugin_definition['forms'][$operation]) ? $plugin_definition['forms'][$operation] : get_class($this);
+    }
+  }
+
+  /**
+   * {@inheritdoc}
+   */
+  public function hasFormClass($operation) {
+    return isset($this->getPluginDefinition()['forms'][$operation]) || ($operation == 'configure' && $this instanceof PluginFormInterface);
+  }

+++ b/core/modules/system/tests/modules/image_test/src/Plugin/ImageToolkit/Operation/test/ExtendNonInstalledClassOperation.php
@@ -0,0 +1,18 @@
+ * @ImageToolkitOperation(
+ *   id = "test_extend_non_installed_class_operation",
+ *   toolkit = "test",
+ *   operation = "extend_non_installed_class_operation",
+ *   label = @Translation("Extend non installed class operation"),
+ *   description = @Translation("Operation extending from a non-installed module class.")
+ * )
+ */
+class ExtendNonInstalledClassOperation extends OperationBase { }

This doesn't surprise me. If I were writing this module, I would write an unannotated class, and then use hook_image_toolkit_operation_alter() to conditionally add it to the available definitions.

Or, conversely, leave it annotated but remove it in the alter (which might work in this case, but not for the bug at hand, since it runs after processDefinitions).

I only just found this issue but we've been struggling with the bug for a couple of weeks already. We decided to move the plugin into a submodule, which is a subpar solution.

#2775963: FacetsSerializer views style plugin depends on optional rest module

When using the patch from mykeryan #9, I also had to patch the BlockBase as proposed by Fabianx.

Without the patch, you won't be able to place any block in the block admin.

Now we just need to combine patch #13 with patch #21 (and re-roll it to apply).

Tagging Novice for that.

Here's how I would change this;

  public function getFormClass($operation) {
    if (isset($this->getPluginDefinition()['forms'][$operation])) {
      return $this->getPluginDefinition()['forms'][$operation];
    }
    elseif ($operation == 'configure' && $this instanceof PluginFormInterface) {
      return static::CLASS;
    }
  }
  public function hasFormClass($operation) {
    return !empty($this->getFormClass($operation));
  }

The thing is, the proposed version of getFormClass presumes the logic of hasFromClass. It presumes that if hasFromClass returns TRUE while isset($plugin_definition['forms'][$operation]) is FALSE then it must mean ($operation == 'configure' && $this instanceof PluginFormInterface) is TRUE without actually testing for that. This is an excellent trap for the future should hasFromClass ever get more complicated. If getFormClass needs to do the same testing as hasFormClass then there's no point in having both so I made hasFormClass call getFormClass and be done.

+1 to #28

Thanks much for quickly fixing this! I think this now needs timplunkett to approve.

1. +++ b/core/lib/Drupal/Core/Block/BlockBase.php
   @@ -276,16 +277,19 @@ public function setTransliteration(TransliterationInterface $transliteration) {
   -    if ($this->hasFormClass($operation)) {
   +    if (isset($this->getPluginDefinition()['forms'][$operation])) {
   ...
   -    return isset($this->getPluginDefinition()['forms'][$operation]);
   +    return !empty($this->getFormClass($operation));
   

   Hah! Elegant :)

2. +++ b/core/lib/Drupal/Core/Block/BlockBase.php
   @@ -276,16 +277,19 @@ public function setTransliteration(TransliterationInterface $transliteration) {
   +    elseif ($operation == 'configure' && $this instanceof PluginFormInterface) {
   +      return static::CLASS;
   +    }
   

   Yeah, this works for me. Nit, we usually don't capitalize that constant: static::class

Also very pleased to see #2799867: Skip discovery of ImageToolkitOperations for which the relative toolkit is not installed. While we need to remove the regression, any code hitting this bug is wrong, and should be fixed anyway.

Yes, this is RTBC! Looks great to me!

Only concern might be the test changes, but I think we only test the right behavior not the wrong now.

Thanks! I think we maybe should add more explicit test coverage?

Isn't ExtendNonInstalledClassOperation doing that?

+++ b/core/lib/Drupal/Core/Block/BlockBase.php
@@ -276,16 +277,19 @@ public function setTransliteration(TransliterationInterface $transliteration) {
   public function getFormClass($operation) {
-    if ($this->hasFormClass($operation)) {
+    if (isset($this->getPluginDefinition()['forms'][$operation])) {
       return $this->getPluginDefinition()['forms'][$operation];
     }
+    elseif ($operation == 'configure' && $this instanceof PluginFormInterface) {
+      return static::class;
+    }
   }
 
   /**
    * {@inheritdoc}
    */
   public function hasFormClass($operation) {
-    return isset($this->getPluginDefinition()['forms'][$operation]);
+    return !empty($this->getFormClass($operation));
   }

This has no test coverage.

This looks like a great fix to me. Very elegant! In addition to #41, here's some additional things that I think are worth doing prior to commit.

1. +++ b/core/lib/Drupal/Core/Plugin/DefaultPluginManager.php
   @@ -247,12 +247,6 @@ public function processDefinition(&$definition, $plugin_id) {
   -      $definition['forms']['configure'] = $definition['class'];
   

   Should we have an empty update function as well, in order to force a cache rebuild to get people's sites up to date with this change?

2. +++ b/core/lib/Drupal/Core/Block/BlockBase.php
   @@ -276,16 +277,19 @@ public function setTransliteration(TransliterationInterface $transliteration) {
      public function getFormClass($operation) {
   ...
      public function hasFormClass($operation) {
   

   Can we move these to a trait? That way, we can provide instructions (either via an update to https://www.drupal.org/node/2773829 or a new CR) for how any contrib plugins other than blocks that might already be taking advantage of #2763157: Allow plugins to provide multiple forms can continue to work after this patch. This would also mean that #41's test coverage should be a test of the trait, not specific to blocks.

3. +++ b/core/modules/system/tests/modules/image_test/src/Plugin/ImageToolkit/Operation/test/ExtendNonInstalledClassOperation.php
   @@ -0,0 +1,18 @@
   +class ExtendNonInstalledClassOperation extends OperationBase { }
   

   Can we move this test out of image_test and into DefaultPluginManagerTest, since we should have this test coverage explicitly as part of the plugin system, not as a side-effect of image toolkits.

Tim is on vacation, so I'm taking over here. @mondrake, if you'd be able to reroll, that'd be splendid. If not, I'll be working on a reroll that includes test coverage tomorrow.

> Tim is on vacation

Vacation?? I thought from the moment you start to maintain more than five core subsystems you are not allowed to go on vacation. The things you all come up with these years :P

@mondrake Okay, on it!

@chx:

Vacation?? I thought from the moment you start to maintain more than five core subsystems you are not allowed to go on vacation. The things you all come up with these years :P

Even if it sometimes seems we don't, we actually do learn from our mistakes :)

#42:

1. I don't think so? It'll behave exactly the same as in HEAD.
2. I don't think a trait makes sense:
   1. Block plugins that ship with multiple forms will automatically get this because 100% of them extend BlockBase (try implementing a block by just implementing the interface, it's pretty much impossible).
   2. The logic in BlockBase::(get|has)FormClass() is very much tied to blocks. It's unlikely that many other plugin managers will want to use this. @Fabianx already said exactly that in #8.a): the 'configure' is very specific to blocks and not generic anyway, so it does not apply to 99% of plugins.
   3. The only reason why HEAD was triggering PHP fatals like #2796827: Non-functional without Imagemagick installed is because it was the is_subclass_of() in the default plugin manager. Which caused it to load the plugin's class. So this caused 99% of plugin managers (all of those that extend DefaultPluginManager) to load the plugin's class.
   4. Finally, as @tim.plunkett describes in #17 and #18, it is also a problem that some plugins rely on other modules' classes, if those other modules are no dependencies of the module that ships the plugin that uses them. But this patch doesn't change anything about that. The commit at #2763157: Allow plugins to provide multiple forms indeed exposes all of them, but only because of the prior point. However, the changes in this patch don't help anything to address that deeper problem. The only change in this patch, is to longer trigger PHP fatals for every plugin that uses an undepended module's class.
   5. So this would only help other plugin managers if they also use the configure operation in particular. If they use any other operation, this trait wouldn't help.

   Therefore I think introducing such a trait would be at best premature at this stage. If it turns out that all of the custom plugin managers that provide multiple forms converge on the configure operation, then sure… but only time will tell.

3. +1. I started doing this, I moved that plugin… but then I realized it wouldn't work. Because DefaultPluginManagerTest is all about testing just the plugin manager, and not discovery, so I'd have to hardcode it in there. Actually, that's fine. We have \Drupal\KernelTests\Core\Plugin\Discovery\AnnotatedClassDiscoveryTest for the discovery, and they all use the same test plugins. So it just means I have to update multiple tests. It also

Still need to do #41. Need to expand \Drupal\Tests\Core\Block\BlockBaseTest. Should be trivial compared to this reroll.

#41: test coverage added!

Note: The test-only patch contains only this new test, not any of the prior test changes/additions. Just to show that this test too is testing what it should be.

+++ b/core/lib/Drupal/Core/Block/BlockBase.php
@@ -276,16 +277,19 @@ public function setTransliteration(TransliterationInterface $transliteration) {
+    elseif ($operation === 'configure' && $this instanceof PluginFormInterface) {

In working on the test coverage, I realized the $this instanceof PluginFormInterface is actually nonsensical.

Because BlockBase implements BlockPluginInterface extends PluginFormInterface… in other words: this is true for every block plugin!

AFAICT this is now ready.

Yes, this is RTBC :) from my side.

#49's test-only patch didn't include all of the tests from #49. Here's all of them, so we can see all the failures for the added test coverage.

Silly bot. Of course, a test-only patch will fail :)

I don't think a trait makes sense...The logic in BlockBase::(get|has)FormClass() is very much tied to blocks. It's unlikely that many other plugin managers will want to use this. @Fabianx already said exactly that in #8.a): the 'configure' is very specific to blocks and not generic anyway, so it does not apply to 99% of plugins.

Why is 'configure' specific to blocks? We've had PluginFormInterface in the D8 codebase for a long time now, lots of plugins implement it, and its methods are (build|validate_submit)ConfigurationForm(). So I would argue that 'configure' is the implicit form operation for every plugin that implements PluginFormInterface. What's special about PluginWithFormsInterface, is that it allows a plugin to retain the implementation of the implicit 'configure' form, while also supporting additional forms, that it can delegate elsewhere.

In core, blocks are the only plugins with these additional forms. But since beta1, we've had a CR that told contrib about this being a generic capability that can be opted into by implementing PluginWithFormsInterface. And this patch will now break those implementations if they were relying on 'configure' already being mapped for them in their definition by DefaultPluginManager.

It's fine for us to break BC from beta1 to fix a critical bug, but the question is then what is the most responsible way to break it. If we provide a trait, then we can tell people that any class that implemented PluginWithFormsInterface and relied on 'configure' automatically being added to the definition can simply remove its (get|has)FormClass() implementations and use PluginWithFormsTrait. That would also be good future-proofing, per the documentation in https://www.drupal.org/core/d8-bc-policy#interfaces that says "Where a base class or trait is provided, use those". On the other hand, if we don't provide the trait, we're telling people that in order to make their PluginWithFormsInterface implementations work, they need to add a 'configure' fallback to their existing (get|has)FormClass() implementations themselves.

I think it's better to get this into RC2 without the trait, than to miss the RC2 window, so leaving at RTBC. But holding off on commit for now, since I'd prefer to do this with the trait, if others agree, and someone can update the patch accordingly. I don't think Wim will be able to get to it for at least the next 16 hours, and I'd prefer to commit it well before then.

Also, re #42.1 /#47.1: Without an update function, we'll have this theoretical problem, related to #56.

Suppose something in contrib other than a block implements PluginWithFormsInterface. And implements (get|has)FormClass() the same way that BlockBase does in HEAD. Then updates core to include the fix in this issue once it's committed. What will happen is that module will continue to work until the next cache clear, because the mapping of 'configure' that's referenced in #42.1 will still exist in the cache. But then, after a cache clear, 'configure' will be removed from the cache, and a call to PluginFormFactory::createInstance($plugin, 'configure') will throw an exception until the module updates its (get|has)FormClass() implementations similarly to this patch's BlockBase changes.

Again, it's fine for us to break BC from betas/RCs to fix a critical issue, but probably best for module authors to discover the breakage immediately, not after some later cache clear. So, I think this is NW for needing an empty update function, unless someone points out an error in the above reasoning. Should be an easy addition to the patch: views.install has plenty of examples to copy from.

In my comments above, I dismissed/didn't consider the possibility of contrib modules already relying on the "multiple forms" support for their plugins in their plugin managers (assuming they extend DefaultPluginManager.) I can't poke a hole in your reasoning. So, #57++.

#56: ok. Done.

Thank you!

Here's a small change to make BlockBase use the trait. And also to add back in the instanceof check removed in #49 since unlike BlockBase, the trait can't assume it to be true. It means we should also have test coverage for the case where the plugin doesn't implement PluginFormInterface, but that can be a post-RC2 followup.

Assuming this comes back green, is anyone online right now who's familiar enough with this issue to review the interdiffs since last RTBC and if appropriate, re-RTBC it?

Ugh, of course. It was very late when I posted that reroll.

RTBC'ing just that last interdiff.

+++ b/core/modules/system/system.install
@@ -1681,3 +1686,14 @@ function system_update_8200(&$sandbox) {
+function system_update_8201() {

We can use post update hook now to clear the cache.

What's the advantage of doing it in post_update vs. update?

Without an answer to #63, and with RC2 approaching fast, I'll re-RTBC this. If someone has an answer to #63, please reset to needs work or needs review. Thanks!

See #2743313: [policy, docs] Use post updates for empty 'clear the cache' updates for more details.

That issue was last touched 3 months ago. Nothing is using that proposed new approach yet. Let's not hold back this issue for that then?

Ticking credit boxes.

The issue in #65 makes good sense. If there was more time before RC2 or if this patch needed backport to 8.1 (where the update number would need to change), I'd agree with holding it up on that. But given that this doesn't need backport to 8.1, I think it's fine for us to use up the 8201 number for this. Therefore, pushed to 8.3.x and cherry-picked to 8.2.x. Thanks all!

I think we should update https://www.drupal.org/node/2773829 to mention PluginWithFormsTrait. I don't know the best way to communicate the BC break for existing implementations of PluginWithFormsInterface not yet using the new trait. Tagging "8.2.0 release notes" which maybe is enough for that communication.

Updated CR: https://www.drupal.org/node/2773829/revisions/view/10076773/10076779.

This still needs an issue title update because it definitely does not solve the issue in the title, that would require something I outlined in #4 -- and that's still a valid, open issue just much harder to hit.

Per #73, rescoping the title to what this issue actually solved.

I think what #73 is also pointing out is that there still remains the problem of the plugin still being discovered though, and then later failing upon instantiation if there's calling code that attempts to instantiate it. #18 is the workaround currently available for that. #2786355: Convert multiple provider plugin work from migrate into plugin dependencies in core is potentially a way to solve it generically.

That's not what's solved here ; not even close.

The generic problem where module A wants to extend a class from module B but not require it; just augment B if it's installed is not solved . The current -- undocumented! -- solution is to always require B and if you want to be optional then write a submodule. In the future, core will require PHP 7 and catch the Error throwable... but for now it's a fatal.

@chx: I don't understand #75. Modules are completely free to ship with classes that extend classes from uninstalled modules. Many core modules ship with test classes that extend Drupal\simpletest\WebTestBase and those modules work fine without Simpletest installed. Similarly, Drupal\node\NodeViewsData extends Drupal\views\EntityViewsData and Node module still works fine without Views module.

The key is for there to not be calling code that tries to load a class that has unmet dependencies. This issue solved a regression with such calling code appearing in DefaultPluginManager::processDefinition().

#18 and/or #2786355: Convert multiple provider plugin work from migrate into plugin dependencies in core is a way for such calling code to not appear elsewhere with respect to plugin instantiation.

As to cases outside of plugins, we'll need to look at those case by case, but are there currently any presenting a problem?

The generic case is very complicated and it is not restricted to plugins or core. For example, if module A provides an event subscriber extending a class from module B, the moment you try to load the class file of A you get a fatal error because the class is not found. Any way that includes a class from A extending from an uninstalled module B will immediately fatal. The same is true for plugins and while we do not blow up during discovery (maybe) surely we do the moment someone tries to use the plugin extending from an uninstalled module. Something like #4 would fix that but the real fix is requiring PHP 7 and handling Error smartly. The need is not dire yet because there is not really a richly vowen contrib ... yet ... but the time is coming and this issue will thwart us heavily.

Edit: Similarly, Drupal\node\NodeViewsData extends Drupal\views\EntityViewsData and Node module still works fine without Views module. -- yes because NodeViewsData is dormant but try to include it without views installed and watch the fireworks.

If i'm not mistaken this issue (the more general one chx is describing) has already bitten us multiple time in site development / deployment.
This happens for example in the following scenario.
You have a custom module with all small alters and extensions for your site 'custom_site_module'.
You now enable flag module in your website, and extend that in the already enabled 'custom_site_module'.
You now deploy to 'dev' in one go. Config change to enable flag / new code to extend flag in already module.
You start build job with drush update (and boom fatal error).
Not sure this is exactly related here (but seeems so), but posting for information purposes.

Yup! That's one way to have this, existing module newly extending a module to be enabled and it goes kaboom.

Yup! That's one way to have this, existing module newly extending a module to be enabled and it goes kaboom.

Well, IMHO if they do that kind of stuff, its their responsiblity to write an update hook to enable the modules. Update hooks should not call out to hooks and what not, so things shouldn't break in there. On top of that there is hook_post_update_N that should be used, once a system is in a consistent state again.