Entity forms skip validation of fields that are edited without widgets

Tagging all the things.

Not sure what we want to do here and how it should happen.

First patch for experimenting. We do validate the configurable fields right now, so we should expand that to the entity base fields. Not sure how to map back the errors to the correct input field on the form ...

TODO:
* ->validate() should be called on the entity object, since there might be constraints registered on the entity level besides the field level constraints. Right?
* Field widgets for entity base fields (there must be an open issue somewhere) that can be flagged in the case of errors.

#2 You're right, validate has to be called on the entity, because an entity can define its own constraints

Re: #2, perhaps you refer to #2010930: [META] Apply formatters and widgets to rendered entity base fields (all besides node.title) and its companion issue which converts node.title field.

Same problem as in #2002158: Convert form validation of comments to entity validation, what should we do with empty string values that reach the primitive validator? Just ignore there as I propose here?

1. @@ -325,6 +325,17 @@ public function validate(array $form, array &$form_state) {
   +          $field_violations = $entity->getNGEntity()->get($field_name)->validate();
   

   ->get() always works on the NG entity, you don't need getNGEntity().

2. @@ -325,6 +325,17 @@ public function validate(array $form, array &$form_state) {
   +            // @todo: Map the error to the correct form element.
   +            form_set_error('', $violation->getMessage());
   

   Maybe go up in the tree and add the error to the first element that we find?

   title.0.value => no match.
   title.0 => no match.
   title => match.

   That is I think kind of the inverse of buildEntity(), which does $entity->set($field_name, $form_state['values'][$field_name} and then the setValue() implementations are responsible for passing it through.

   We could move that line into a separate method, then it's easy to override if you have a weird form structure. I don't think that will be common.

3. @@ -327,10 +327,6 @@ protected function actions(array $form, array &$form_state) {
   -    if ($node->id() && (node_last_changed($node->id(), $this->getFormLangcode($form_state)) > $node->changed)) {
   -      form_set_error('changed', t('The content on this page has either been modified by another user, or you have already submitted modifications using this form. As a result, your changes cannot be saved.'));
   -    }
   -
   

   Do we actually have test coverage for this? (upload a patch that just removes this to verify? Possibly remove a few additional validations too.

4. @@ -327,10 +327,6 @@ protected function actions(array $form, array &$form_state) {
        // Validate the "authored by" field.
        if (!empty($node->name) && !($account = user_load_by_name($node->name))) {
          // The use of empty() is mandatory in the context of usernames
   

   In the NG conversion issue, I had to refactor this stuff quite a bit, as it is no longer possible to set an invalid reference. This still works around that by introducing a temporary ->name property that doesn't actually exist outside of the edit form.

timplunkett  	beejeebus: no entities have validation
beejeebus    	• beejeebus giggles hysterically

#6: entity-forms-validation-2002180-6.patch queued for re-testing.

Restoring tags unintentionally dropped in #11.

So yeah, currently we only run field constraint validation on fields where we know how to report the errors back - that is, fields that are rendered through widgets. This now can include some base fields (only node title for now, probably more to come).

#2095195: Remove deprecated field_attach_form_*() moves this step to a separate validateWidgetsValues() in a separate helper class usable by other forms than EntityFormControllers.

The rest of the fields:

- Might be rendered by arbitrary ad-hoc FAPI code in EntityFormController or hook_form_alter(). A rule like the one suggested by @Berdir in #7.2 might be applied to "best guess" where the error should be reported. I don't think the violation messages contain the field label currently, so it needs to be prepended so that the displayed errors make sense.

- Might just not be present in the form. They still can raise violations for some reason (e.g constraints have changed since the entity was last saved). We do not want to raise form errors for those, that would block form submission on stuff the user cannot fix himself in the form.

Also, it's been agreed in #2070429: Configurable fields aren't validated against the "required" constraint except in forms that field validation should not run on fields on which FAPI-level errors have already been reported, because:
- that might cause double report (eg required checked by both #required and a field constraint)
- the presence of FAPI errors for the field means the submitted form values are invalid/broken to begin with, and we shouldn't even try to turn them into actual model values in the entity.

See also #2111887-28: Regression: Only title (of base fields) on nodes is marked as translatable. For both content translation UI and validation error reporting, I think we'll want to replace all entity form elements that correspond to fields with widgets in #2010930: [META] Apply formatters and widgets to rendered entity base fields (all besides node.title). If we want a fallback along the lines of 7.2, then I think simply a form_set_error() of the top-level violation path (i.e., just 'title') is a good start: we could refine from there if needed, but I don't think we'll need it. Since form_set_error() is based on #parents rather than #array_parents, something like form_set_error('uid') will correctly flag $form['author']['uid']. It would also automatically flag all children of that. So the only problem would be if there are some children we don't want flagged, but that means we have a multiproperty base field not using a widget: an edge case, and one that can be fixed by converting it to use a widget.

Needs reroll.

Reroll needs knowledge of what to do with the changes in Drupal\Core\Entity\EntityFormController::validate. This method has drastically changed.

diff --git a/core/lib/Drupal/Core/Entity/EntityFormController.php b/core/lib/Drupal/Core/Entity/EntityFormController.php
index ceb09de..3742a8b 100644
--- a/core/lib/Drupal/Core/Entity/EntityFormController.php
+++ b/core/lib/Drupal/Core/Entity/EntityFormController.php
@@ -325,6 +325,17 @@ public function validate(array $form, array &$form_state) {
           $violations[$field->getName()] = $field_violations;
         }
       }
+      // Now trigger the validation for entity base fields that are not
+      // configurable.
+      foreach ($definitions as $field_name => $definition) {
+        if (empty($definition['configurable'])) {
+          $field_violations = $entity->getNGEntity()->get($field_name)->validate();
+          foreach ($field_violations as $violation) {
+            // @todo: Map the error to the correct form element.
+            form_set_error('', $violation->getMessage());
+          }
+        }
+      }
     }

we cannot do it safely - we cannot ensure which fields the constraint have been using for the validation

Is this a reply to the current agreed approach of "field validation should not run on fields on which FAPI-level errors have already been reported" ?

There might be some entity-level/cross-field constraints, but the vast majority of the constraints are specified on isolated fields. For those, we totally have the ability to skip them.

The only safe approach seams to be enforcing having a valid entity *before* starting to edit it

.
Seems hard to reach. We cannot prevent UI or direct API/config changes from changing "max allowed value" to 10 because there exists an entity with value 15 somewhere ?

+++ b/core/lib/Drupal/Core/Entity/EntityFormController.php
@@ -325,6 +325,17 @@ public function validate(array $form, array &$form_state) {
+          foreach ($field_violations as $violation) {
+            // @todo: Map the error to the correct form element.
+            form_set_error('', $violation->getMessage());

The primary trouble here is to find proper form element by using $violation->propertyPath

There are probably overlaps with this issue #1493324: Inline form errors for accessibility and UX.

Patch in #6 applies cleanly for me. Removing 'Needs reroll' tag and retesting.

6: entity-forms-validation-2002180-6.patch queued for re-testing.

My mistake, I was squelching file-not-found errors. Resetting tag.

I've been chasing this around and I think #2278403: Remove uses of form_execute_handlers() is also related (and that this issue blocks that #2278403: Remove uses of form_execute_handlers()). EntityFormController.php has been removed, so the previously submitted patches are a bit moot. I think the patch should really be implemented in EntityForm::validate() now.

The previous patches sort of indicate a proposed resolution: we want to have a generalized approach to validating fields on entities. The generalized approach would be to loop through field_defintions on the entity and check them against their constraints. From patch #6:

@@ -325,6 +325,17 @@ public function validate(array $form, array &$form_state) {
+      // Now trigger the validation for entity base fields that are not
+      // configurable.
+      foreach ($definitions as $field_name => $definition) {
+        if (empty($definition['configurable'])) {
+          $field_violations = $entity->getNGEntity()->get($field_name)->validate();
+          foreach ($field_violations as $violation) {
+            // @todo: Map the error to the correct form element.
+            form_set_error('', $violation->getMessage());
+          }
+        }
+      }

Then things like NodeForm can just call parent::validate() (which is ContentEntityForm), and its parent would call grandparent::validate() which finally gets us back to EntityForm::validate().

Question: Is there ever a case that there would be form elements that are not field-driven but also need validation? Perhaps in EntityForm::validate() we don't need this case because subclasses will take care of the specifics that might be outside of field-driven form elements, if they exist.

If I can get some confirmation that my assessment is correct, I'll update the Issue Summary with more details.

Another update for anyone new looking at this: EntityFormController wasn't removed, it was renamed:

commit c79ac2f74160f7d0a1626d3e762c4c98fc83cfff
Author: Alex Pott
Date: Sat Apr 26 00:12:39 2014 +0100

Issue #2238077 by Berdir, alexpott, tim.plunkett: Rename EntityFormController to EntityForm.

See #2238077: Rename EntityFormController to EntityForm.

It seems like we're trying to account for different scenarios as we think about this. Do we have tests written for Entity validation? We should write tests so we know what it is that's breaking, then we can fix it.

I'm going to work on the Issue Summary and an initial patch based on our current direction.

This is a very early patch (especially considering that I don't feel we've actually decided on how the validation should be run). Please see the comments for direction for the next steps in the patch.

This is a patch based on the proposed resolution of validating the entire entity.

EDIT: My bad, messed up the patch name's comment number.

Discussed this a bit more with effulgentsia and we figured something along the following lines would be good to deal with violations:

So this sounds more like solution two, do you want to update the IS to point people to the way how it could be solved?

Tagging.

Are we sure that we don't want all fields to have widgets? See #2378947: Hiding a field using form modes should not remove it from the form object

I discussed this with @catch and we decided that dealing with fields that can't be edited on the form at all is a different scope than fields that can be edited but are skipping validation due to using a custom form element rather than a widget. We also decided that only the latter is Critical priority. So, I opened #2395831: Entity forms skip validation of fields that are not in the EntityFormDisplay as a Major issue, and updated the IS of this one to a smaller scope.

If anyone disagrees with this, please comment.

Find all occurrences (if any) of core entity forms that edit a field value with a direct form element rather than a widget.

The issue summary in #2010930: [META] Apply formatters and widgets to rendered entity base fields (all besides node.title) has a list in its "Remaining Tasks" section. Would be good for someone to confirm if that list is accurate and complete.

If there are any such occurrences, fix them to use a widget.

I considered making that meta issue and its children critical to match the priority of this issue. However:

1. It's possible that not all of the work in those child issues is necessary to solve this. For example, those issues are converting remaining base fields to use both widgets and formatters, but this issue only makes the widget part of that critical.
2. It's possible someone can present a good argument for not having to complete those conversions, and instead solve this issue in a different way (for example, document that element-level validation of those cases is adequate and add tests that ensure that).

So, I'm not making those issues critical at this time, but I do think that attempting to complete them is a good way to resolve this issue, unless for some reason it turns out not to be.

See also #2227381-44: Apply formatters and widgets to User base fields 'name' and 'email' for a specific example where we should decide on the relative merits of "use a widget" vs. "use custom validation". And possibly make a decision there that applies to the other remaining examples in core.

What is the difference of this issue vs. #2395831: Entity forms skip validation of fields that are not in the EntityFormDisplay?

Wouldn't #2395831: Entity forms skip validation of fields that are not in the EntityFormDisplay fix this as well? Why do we need two criticals to track this?

based on #51

Discussed with @xjm agreed is a duplicate of #2395831: Entity forms skip validation of fields that are not in the EntityFormDisplay, will update IS over there.

Thanks @larowlan!