Problem/Motivation

A @MediaSource can have an entity_reference field as its sourceField. It can set a SourceFieldConstraint. This constraint might report a violation. But this violation does not propagate to the form. Because of that, the constraint cannot prevent form submission.

Proposed resolution

\Drupal\Core\Field\Plugin\Field\FieldWidget\EntityReferenceAutocompleteWidget::errorElement() should return the $element when there's no target_id in it.

Remaining tasks

  • Provide more background info (ie. how the proposed resolution was reached).
  • Discuss the proposed resolution and/or come up with a better one.
  • Write tests.
  • Roll the patch. (This is the easiest item at the time of reporting this.)

User interface changes

None.

API changes

None.

Data model changes

None.

CommentFileSizeAuthor
#13 2887731-13-proper-fix.patch8.66 KBamateescu
#13 interdiff.txt800 bytesamateescu
#13 2887731-6-test-only-and-2784015-17.patch7.88 KBamateescu
#6 entityreferenceautocompletewidget_constraint-2887731-6-test-and-fix.patch4.11 KBboobaa
#6 entityreferenceautocompletewidget_constraint-2887731-6-test-only.patch3.29 KBboobaa
#5 entityreferenceautocompletewidget_constraint-2887731-5.patch848 bytesboobaa

Comments

Boobaa created an issue. See original summary.

boobaa’s picture

boobaa
he/him
commented
Title: Entityreference @MediaSource's constraint cannot stop submission » @MediaSource with EntityreferenceAutocompleteWidget: constraint cannot stop form submission
Related issues: +#2574095: File upload and entity autocomplete results in fatal, +#2274433: Do not allow to alter Locked field via UI

I wrote a @MediaSource plugin with an entity_reference as its source field, wrote a constraint which reported a violation, double-checked that my code runs as intended, then had to face the fact that the violation's error is not displayed, and my form submission is not prevented at all. Then I rewrote my @MediaSource plugin with a string field as its source field, changed the constraint accordingly, then had to face the fact that the violation's error IS displayed properly and DOES prevent form submission this time. XDebug revealed that the only difference between the two is that \Drupal\Core\Field\WidgetBase::flagErrors() calls $form_state->setError() for string field but not for entity_reference field. The reason is that \Drupal\Core\Field\Plugin\Field\FieldWidget\EntityReferenceAutocompleteWidget::errorElement() returns FALSE when there's no $element['target_id'], which is the case this time.

Git history reveals three possibly related issues: #2574095: File upload and entity autocomplete results in fatal, #1959806: Provide a generic 'entity_autocomplete' Form API element and #2050835: Move Widget, Formatter, and FieldType plugin types to the Core\Field system. These latter two are only the introduction of the related files (I didn't track them further; possibly they're coming from the contrib era of entityreference.module). I agree with the first part of the full patch in #2574095-3: File upload and entity autocomplete results in fatal, but I think returning FALSE is invalid here: in the case of missing $element['target_id'] the whole $element should be returned, as it's done in the parent WidgetBase class. When the whole $element is returned instead of FALSE, the @MediaSource constraint's violation message DOES propagate to the form, thus prevents form submission.

But the story doesn't end here. \Drupal\Core\Field\WidgetBase::flagErrors() seems to be the only case where this FALSE is handled, but I couldn't find the reason for this, so this might need some patching, too. Additionally, I could NOT reproduce the problem on the UI with core only, which means a new module will be needed(?) even for testing this. OTOH, as this problem is not in media.module's scope but more generic, I filed this against the entity system.

And why is it needed at all? Firstly, any constraint returning a violation should prevent the form submission IMO (that's why I'm filing this as a bug). Secondly, until #2274433: Do not allow to alter Locked field via UI lands, there's a way to change a @MediaSource plugin's sourceField which might render the rest of the plugin unusable/not working, so having a constraint for that seems to be straightforward.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

dpavon’s picture

dpavon commented

Nice catch. I'd say I've run into the the same problem. My situation:

  • Node type with a select/list field.
  • Same node type also has Entity reference field (entity: user) with Autocomplete widget. Its entries come from a view.
  • I created a Constraint+Validator for the list: It worked, error shows up.
  • I tried to create a Constraint+Validator for the entity reference: Validate method is executed, addViolation is executed as well, but form submission is not stopped, as no error happened.
  • Changing the Widget of the field in the form display makes the constraint work again.
boobaa’s picture

boobaa
he/him
commented
Title: @MediaSource with EntityreferenceAutocompleteWidget: constraint cannot stop form submission » EntityreferenceAutocompleteWidget constraint cannot stop form submission
StatusFileSize
new entityreferenceautocompletewidget_constraint-2887731-5.patch848 bytes

Attached patch seems to solve the problem – but to the best of my understanding a new module will be needed for testing this, as there's no way to add constraints on the UI.

EDIT: And that new module should provide its custom entity type, as it seems it's only possible to add custom constraints for baseFields/during (entity? field?) creation: https://www.drupal.org/docs/8/api/entity-validation-api/providing-a-cust...

boobaa’s picture

boobaa
he/him
commented
Status: Active » Needs review
StatusFileSize
new entityreferenceautocompletewidget_constraint-2887731-6-test-only.patch3.29 KB
new entityreferenceautocompletewidget_constraint-2887731-6-test-and-fix.patch4.11 KB

Looks like I found a way to test this.

The last submitted patch, 6: entityreferenceautocompletewidget_constraint-2887731-6-test-only.patch, failed testing. View results

tamasd’s picture

tamasd commented
Status: Needs review » Reviewed & tested by the community
xjm’s picture

xjm
she/her
Primary language English
commented
Status: Reviewed & tested by the community » Needs review

Nice work on the test and also the full background on how you researched this.

+++ b/core/tests/Drupal/KernelTests/Core/Entity/FieldWidgetConstraintValidatorTest.php
@@ -58,6 +58,33 @@ public function testValidation() {
+    \Drupal::formBuilder()->prepareForm('field_test_entity_form', $form, $form_state);
+    \Drupal::formBuilder()->processForm('field_test_entity_form', $form, $form_state);

Is this line repeated intentionally?

xjm’s picture

xjm
she/her
Primary language English
commented
Issue tags: +Needs subsystem maintainer review, +Needs followup

Looks like this could also use susbsystem maintainer review, on account of #2. #2574095: File upload and entity autocomplete results in fatal is still presumably still working as the test did not fail, but we should probably look more at whether that FALSE value should really be used... that could also be a followup discussion.

boobaa’s picture

boobaa
he/him
commented
Status: Needs review » Reviewed & tested by the community

Sorry for setting my own patch back to RTBC, but I'm failing to see duplicated lines here (tho I must admit I had to look at it twice to recognize that prepareForm is not exactly processForm). :)

xjm’s picture

xjm
she/her
Primary language English
commented
Status: Reviewed & tested by the community » Needs review

Ah, I made the same mistake -- thanks Boobaa.

I still wold like a sub system maintainer to check on this change, especially given how we
)last time's fix introduced tge new bug.

amateescu’s picture

amateescu commented
Issue tags: -Needs subsystem maintainer review, -Needs followup
StatusFileSize
new 2887731-6-test-only-and-2784015-17.patch7.88 KB
new interdiff.txt800 bytes
new 2887731-13-proper-fix.patch8.66 KB

Thanks @xjm for requesting a subsystem maintainer review. Sadly, #2574095: File upload and entity autocomplete results in fatal did not receive the same treatment so the wrong fix was committed over there :(

This patch is another occurrence of the dozens of issues in this area that are fixing the symptom instead of the root cause, all of them trying to do stuff on the widget's errorElement() implementation while the actual problem is one layer above that.

#2784015: Widget validation crashes on ItemList violations for widgets with a custom errorElement() implementation has a patch that fixes this problem generically. As a proof, here's the test-only patch from #6 combined with the full patch from #2784015-17: Widget validation crashes on ItemList violations for widgets with a custom errorElement() implementation. Also, to demonstrate what I said above that #2574095: File upload and entity autocomplete results in fatal also did not provide the correct fix, the interdiff attached reverts the change to \Drupal\Core\Field\Plugin\Field\FieldWidget\EntityReferenceAutocompleteWidget::errorElement() from that issue, and it still passes the test added over there as well the one added here.

amateescu’s picture

amateescu commented
Status: Needs review » Closed (duplicate)

Given the explanation I have in #13 and unless the testbot proves me wrong, I'm going to to close this issue as a duplicate of #2784015: Widget validation crashes on ItemList violations for widgets with a custom errorElement() implementation.