RequirementsException gives an Invalid argument supplied for foreach() when the requirements property has a single value array.

Quick easy patch to check for the condition and handle it.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

lostkangaroo created an issue. See original summary.

lostkangaroo’s picture

Helps to upload the patch

lostkangaroo’s picture

Status: Active » Needs review
mikeryan’s picture

Status: Needs review » Needs work

Could you add a test demonstrating the failure?

Exactly what scenario triggered it? Would it be better to fix the thrower that didn't properly form the array?

lostkangaroo’s picture

To be quick, this was encountered when a module was disabled in D6 when a migration package included it as a source dependency. There was only one requirement, in this case "source provider", with only one value. Following the trail this value gets sent as an array to RequirementsException as a flat array as opposed to the 2D array that is needed for the nested foreach loops of getRequirementsString(). I would say that it is a good idea to fix the thrower of the exception to avoid this but we would need to guarantee that $this->pluginDefinition['source_provider'] always returns an array to do so.

Tests are always a good idea and will look to add some shortly.

lostkangaroo’s picture

Running the tests solo without the fix patch to show the error.

lostkangaroo’s picture

Status: Needs work » Needs review
lostkangaroo’s picture

Tests with fix applied.

The last submitted patch, 6: requirements_test.patch, failed testing.

phenaproxima’s picture

Status: Needs review » Needs work

Looks good except for two things:

  1. +++ b/core/modules/migrate/src/Exception/RequirementsException.php
    @@ -60,8 +60,13 @@ public function getRequirements() {
    +      if (is_array($requirements)) {
    +        foreach ($requirements as $value) {
    +          $output .= "$requirement_type: $value. ";
    +        }
    +      }
    +      else {
    +        $output .= "$requirement_type: $requirements. ";
    

    Code repetition makes me scrunch up my face. Maybe do something like this:

    if (is_scalar($requirements)) {
      $requirements = array($requirements);
    }
    
  2. +++ b/core/modules/migrate/tests/src/Unit/Exception/RequirementsExceptionTest.php
    @@ -0,0 +1,55 @@
    +  protected $missing_requirements = ['random_jackson_pivot', '51_Eridani_b'];
    

    Should use camelCase.

phenaproxima’s picture

+++ b/core/modules/migrate/tests/src/Unit/Exception/RequirementsExceptionTest.php
@@ -0,0 +1,55 @@
+   * Provides a list of requirements to test

Nit: missing a period.

phenaproxima’s picture

One more.

+++ b/core/modules/migrate/tests/src/Unit/Exception/RequirementsExceptionTest.php
@@ -0,0 +1,55 @@
+  public function getRequirements() {

The core convention is to use the word "provider" in data provider names. So maybe getRequirementsProvider() is a better name for this method.

lostkangaroo’s picture

Addressed all from #10, #11, and #12

I chose to use !is_array() rather then is_scalar because according to the second note on http://php.net/manual/en/function.is-scalar.php NULL values will be passed through which would cause the original php error.

phenaproxima’s picture

Status: Needs review » Reviewed & tested by the community
+++ b/core/modules/migrate/src/Exception/RequirementsException.php
@@ -60,6 +60,10 @@ public function getRequirements() {
+      if(!is_array($requirements)) {
+        $requirements = array($requirements);
+      }

Tiny annoying code style nit: that should be if (!is_array($requirements)). Can be fixed on commit, though.

This has passed DrupalCI and otherwise looks fine to me. RTBC!

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Fixed that, and...

Committed and pushed to 8.0.x. Thanks!

Although those random test names are truly... random. :)

  • webchick committed 14c552f on 8.0.x
    Issue #2549783 by lostkangaroo, phenaproxima: RequirementsException...

Status: Fixed » Closed (fixed)

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