Warning Warning: Invalid argument supplied for foreach() in Drupal\Component\Plugin\Discovery\DerivativeDiscoveryDecorator->getDerivatives()

Do we have any updated about this issue?

I applied the patch and works good for me.

Good Work and Good Weekend.

Best,

+++ b/core/lib/Drupal/Component/Plugin/Discovery/DerivativeDiscoveryDecorator.php
@@ -99,14 +99,16 @@ protected function getDerivatives(array $base_plugin_definitions) {
         $derivative_definitions = $deriver->getDerivativeDefinitions($plugin_definition);
-        foreach ($derivative_definitions as $derivative_id => $derivative_definition) {
-          $plugin_id = $this->encodePluginId($base_plugin_id, $derivative_id);
-          // Use this definition as defaults if a plugin already defined
-          // itself as this derivative.
-          if ($derivative_id && isset($base_plugin_definitions[$plugin_id])) {
-            $derivative_definition = $this->mergeDerivativeDefinition($base_plugin_definitions[$plugin_id], $derivative_definition);
+        if(!empty($derivative_definitions)) {

It should not be necessary to check the emptiness here.

According to the docs in \Drupal\Component\Plugin\Derivative\DeriverInterface::getDerivativeDefinitions() (see below) $derivative_definitions should always be an array. We need to work out why it is not. What are the steps to reproduce the problem? What contrib modules are installed?

  /**
   * Gets the definition of all derivatives of a base plugin.
   *
   * @param array $base_plugin_definition
   *   The definition array of the base plugin.
   * @return array
   *   An array of full derivative definitions keyed on derivative id.
   *
   * @see getDerivativeDefinition()
   */
  public function getDerivativeDefinitions($base_plugin_definition);

$derivative_definitions is an array as per the documentation. I have reviewed the patch file and it seems,which returns an array called
$derivative_definitions from $deriver->getDerivativeDefinitions($plugin_definition);
We have used foreach for derivative_definitions to look for each definition.

Here is the interdiff which removes the if(!empty($derivative_definitions))

#7 is just an interdiff, we need to have the patch as well, so maintainers don't need to apply both files.

Well, all the #7 interdiff does is revert back to the pre-patch state. Might as well just say don't use the patch.

We're running into the same problem on a Drupal 8.6 site, every time a node is changed via the UI.

I agree with alexpott (#6) and Dinesh18 (#7) that

According to the docs in \Drupal\Component\Plugin\Derivative\DeriverInterface::getDerivativeDefinitions() $derivative_definitions should always be an array.

but not with their conclusion

It should not be necessary to check the emptiness here.

Some modules (e.g.older version of ldap) fail to return an array (at least an empty one).

So in order to need to "work out why it is not" I think it would be better to test if $derivative_definitions is an array (an empty one is OK in foreach) and may be log a warning containing $base_plugin_id.
This way we know which module is responsible and can take action (and get rid of the PHP warning).

Patch attached (maybe we need dependency injection to use the logger?)

Had the same issue where this error was appearing after a cache clearing/rebuilding. Applied the patch in #11 and the error has not appeared since.

I agree with @collinhaimes

Also works good for me

+1 to it

+++ b/core/lib/Drupal/Component/Plugin/Discovery/DerivativeDiscoveryDecorator.php
@@ -99,14 +99,23 @@ protected function getDerivatives(array $base_plugin_definitions) {
+          // log warning about plugin uncorrectly implementing getDerivativeDefinitions()
+          \Drupal::logger('plugin discovery')->warning(
+            'getDerivativeDefinitions() does not return an array for plugin "@plugin".',
+            ['@plugin' => $base_plugin_id]
+          );

I think a trigger_error(E_USER_WARNING) is appropriate here. And let that do the logging. Then we don't have a call out to a service in the a component library.

Bring on strict types and return types in PHP7!

Also tbh we are replacing a warning with a warning so /shrug - is this really worth it? Make for the extra info but still.

Just to say that this log help me to debug a stupid error (forgot to return the famous array).
Working well... missing DI

Not sure how relevant this is. But adding patch to use trigger_error as per #14.

Also tbh we are replacing a warning with a warning so /shrug - is this really worth it? Make for the extra info but still.

This is a really good question that is hard to answer, I think the more specific error here is better, so I'd give this a +1

+++ b/core/lib/Drupal/Component/Plugin/Discovery/DerivativeDiscoveryDecorator.php
@@ -99,14 +99,23 @@ protected function getDerivatives(array $base_plugin_definitions) {
+          // log warning about plugin uncorrectly implementing getDerivativeDefinitions()

This comment goes over 80 chars wide, should be reflowed. It also probably needs to start with a capital letter.

Besides the comment issue mention by @borisson_ in #24 core/lib/Drupal/Component/Plugin/Discovery/DerivativeDiscoveryDecorator.php also changes from 644 to 755 and that displeases TestBot:

Checking core/lib/Drupal/Component/Plugin/Discovery/DerivativeDiscoveryDecorator.php

check failed: file core/lib/Drupal/Component/Plugin/Discovery/DerivativeDiscoveryDecorator.php should be 644 not 755
PHPCS: core/lib/Drupal/Component/Plugin/Discovery/DerivativeDiscoveryDecorator.php passed

Hi All, thanks for looking into this issue. I am also trying to help here.
I have made few changes in patch #11, as added one more check for array count.

Also, as suggested by @borisson_ in #24 , I have corrected the comment line. this may help.

Fixed Failed Commands on #26
Attached interdiff

This issue is being reviewed by the kind folks in Slack, #need-reveiw-queue. We are working to keep the size of Needs Review queue [2700+ issues] to around 200, following Review a patch or merge require as a guide.

Moving back to NW for an issue summary update as the solution appears to be add a more detailed log which isn't mentioned in the IS.

Also think this will need a test case