If a constraint validator set on an entity field (a string product "sku" field, for example) adds a violation, the following code in WidgetBase::flagErrors() triggers:

        $violations_by_delta = array();
        foreach ($violations as $violation) {
          // Separate violations by delta.
          $property_path = explode('.', $violation->getPropertyPath());
          $delta = array_shift($property_path);
          // Violations at the ItemList level are not associated to any delta,
          // we file them under $delta NULL.
          $delta = is_numeric($delta) ? $delta : NULL;

          $violations_by_delta[$delta][] = $violation;
          $violation->arrayPropertyPath = $property_path;
        }

The $delta is set to NULL in this case, since $property_path is empty.

The PHP manual says:

Null will be cast to the empty string, i.e. the key null will actually be stored under "".

So our $delta key gets converted back from NULL to an empty string. Further down there is this piece of code:

        foreach ($violations_by_delta as $delta => $delta_violations) {
          // Pass violations to the main element:
          // - if this is a multiple-value widget,
          // - or if the violations are at the ItemList level.
          if ($handles_multiple || $delta === NULL) {
            $delta_element = $element;
          }

The $delta === NULL check fails, and the system crashes.

We need to either do the NULL cast further down the line, or replace the strict NULL check with an is_numeric check.

CommentFileSizeAuthor
#8 2319719-2-fix-widgetbase-crash-8.patch6.85 KBtim bozeman
#7 interdiff.txt1.55 KBtim bozeman
#7 2319719-2-fix-widgetbase-crash-7.patch5.56 KBtim bozeman
#5 2319719-2-fix-widgetbase-crash-5.patch6.79 KBundertext
#1 2319719-1-fix-widgetbase-crash.patch1.29 KBbojanz
#2 2319719-2-fix-widgetbase-crash.patch6.78 KBpcambra
#2 2319719-2-fix-widgetbase-crash-fail.patch5.49 KBpcambra

Comments

bojanz’s picture

bojanz commented
Status: Active » Needs review
StatusFileSize
new 2319719-1-fix-widgetbase-crash.patch1.29 KB

Something like this perhaps.
Does anyone know where the test should live?

pcambra’s picture

pcambra
he/him
Primary language Spanish
commented
StatusFileSize
new 2319719-2-fix-widgetbase-crash-fail.patch5.49 KB
new 2319719-2-fix-widgetbase-crash.patch6.78 KB

Here's a failing test and the patch fix.

The last submitted patch, 2: 2319719-2-fix-widgetbase-crash-fail.patch, failed testing.

penyaskito’s picture

penyaskito
he/him
Primary language Spanish
Location 🧑🏽‍🌾 Seville 💃 Andalusia, UTC+2 🇪🇺
commented
Status: Needs review » Needs work

The fix itself looks good, and nice test coverage!

Some nitpicking.

  1. +++ b/core/modules/system/tests/modules/entity_test/src/Entity/EntityTestConstraintViolation.php
@@ -0,0 +1,58 @@
+ * Contains \Drupal\entity_test\Entity\EntityTest.

    copypasted? 

  2. +++ b/core/modules/system/tests/modules/entity_test/src/Entity/EntityTestConstraintViolation.php
@@ -0,0 +1,58 @@
+    $fields['name']->addConstraint('FieldWidgetConstraint', array());
+
+
+
+    return $fields;
+  }

    Too much spacing. 

  3. +++ b/core/modules/system/tests/modules/entity_test/src/Plugin/Validation/Constraint/FieldWidgetConstraint.php
@@ -0,0 +1,23 @@
+ * id = "FieldWidgetConstraint",
+ * label = @Translation("Field widget constraint.")

    Bad indentation.

undertext’s picture

undertext commented
Status: Needs work » Needs review
StatusFileSize
new 2319719-2-fix-widgetbase-crash-5.patch6.79 KB
dawehner’s picture

dawehner
Primary language German
commented
Status: Needs review » Needs work

Having a proper test for this is great! Let's fix the two small bits.

  1. +++ b/core/modules/system/src/Tests/Entity/FieldWidgetConstraintValidatorTest.php
@@ -0,0 +1,48 @@
+
+    // Pretend the form has been built.
+    
$form_state['build_info']['callback_object'] = \Drupal::entityManager()->getFormObject($entity_type, 'default');
+    \Drupal::formBuilder()->prepareForm('field_test_entity_form', $form, $form_state);
+    drupal_process_form('field_test_entity_form', $form, $form_state);
+
+    // Validate the field constraint.
+    $display->validateFormValues($entity, $form, $form_state);

    Wow, it is cool that it is possible to validate via API . drupal_process_form() can be directly replaced by \Drupal::formBuilder()->processForm 

  2. +++ b/core/modules/system/tests/modules/entity_test/src/Entity/EntityTestConstraintViolation.php
@@ -0,0 +1,56 @@
+ * Defines the test entity class.

    Let's adapt this one line description as well.

tim bozeman’s picture

tim bozeman commented
Status: Needs work » Needs review
StatusFileSize
new 2319719-2-fix-widgetbase-crash-7.patch5.56 KB
new interdiff.txt1.55 KB

Reroll of #5.

I changed drupal_process_form('field_test_entity_form', $form, $form_state);
to \Drupal::formBuilder()->processForm('field_test_entity_form', $form, $form_state);

And changed the comment to Defines the test entity class for testing entity constraint violations.

tim bozeman’s picture

tim bozeman commented
StatusFileSize
new 2319719-2-fix-widgetbase-crash-8.patch6.85 KB

Whoops. Last patch was missing a file.

bojanz’s picture

bojanz commented
Status: Needs review » Reviewed & tested by the community

Thanks, Tim.

This one should be ready to go once the tests come back green.

The last submitted patch, 7: 2319719-2-fix-widgetbase-crash-7.patch, failed testing.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed 62edf52 and pushed to 8.0.x. Thanks!

diff --git a/core/modules/system/src/Tests/Entity/FieldWidgetConstraintValidatorTest.php b/core/modules/system/src/Tests/Entity/FieldWidgetConstraintValidatorTest.php
index 9c6382a..5fa873d 100644
--- a/core/modules/system/src/Tests/Entity/FieldWidgetConstraintValidatorTest.php
+++ b/core/modules/system/src/Tests/Entity/FieldWidgetConstraintValidatorTest.php
@@ -8,9 +8,7 @@
 namespace Drupal\system\Tests\Entity;
 
 use Drupal\Core\Form\FormState;
-use Drupal\Core\TypedData\DataDefinition;
-use Drupal\entity\Entity\EntityFormDisplay;
-use Drupal\simpletest\DrupalUnitTestBase;
+use Drupal\simpletest\KernelTestBase;
 use Drupal\system\Tests\TypedData;
 
 /**
@@ -18,7 +16,7 @@
  *
  * @group Entity
  */
-class FieldWidgetConstraintValidatorTest extends DrupalUnitTestBase {
+class FieldWidgetConstraintValidatorTest extends KernelTestBase {
 
   public static $modules = array('entity', 'entity_test', 'field', 'user');
 
diff --git a/core/modules/system/tests/modules/entity_test/src/Entity/EntityTestConstraintViolation.php b/core/modules/system/tests/modules/entity_test/src/Entity/EntityTestConstraintViolation.php
index deb5d44..f90698d 100644
--- a/core/modules/system/tests/modules/entity_test/src/Entity/EntityTestConstraintViolation.php
+++ b/core/modules/system/tests/modules/entity_test/src/Entity/EntityTestConstraintViolation.php
@@ -7,12 +7,7 @@
 
 namespace Drupal\entity_test\Entity;
 
-use Drupal\Core\Entity\ContentEntityBase;
 use Drupal\Core\Entity\EntityTypeInterface;
-use Drupal\Core\Field\BaseFieldDefinition;
-use Drupal\Core\Entity\EntityStorageInterface;
-use Drupal\user\EntityOwnerInterface;
-use Drupal\user\UserInterface;
 
 /**
  * Defines the test entity class for testing entity constraint violations.

Fixed on commit. DrupalUnitTestBase is deprecated and everything else was not used.

  • alexpott committed 62edf52 on 8.0.x 
    Issue #2319719 by Tim Bozeman, pcambra, undertext, bojanz: Fixed Widget...

Status: Fixed » Closed (fixed)

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

amateescu’s picture

amateescu commented

Sadly, this patch did not fix the issue entirely, see #2784015-11: Widget validation crashes on ItemList violations for widgets with a custom errorElement() implementation.