Remove EntityFormInterface::validate() and stop using button-level validation by default in entity forms

I have never understood ContentEntityForm::validate() or even EntityForm::validate().

There are two bugs here, they may need two issues.

First, EntityFormInterface is clearly wrong and should have array &$form exactly like FormInterface::validateForm() does.
Secondly, the confusing workaround of manually executing the validators.

I'm going to see what breaks with removing the second part, let's hope we had test coverage that explains this!

(Also, why does ContentEntityForm::validate() return the $entity and EntityForm::validate() did not?)

Not an abstract class.

Okay I think I have a grasp of what to do next. Patch coming soon.

Short story:
Regular forms only run $form-level handlers if there are no handlers on the button.
Entity forms run both.

This patch switches entity forms to behave like all other forms, and fixes the 3 broken instances.

This needs tests for the &$form change, and to enforce the $form['#validate'] vs $form['actions'][$key]['#validate'] difference

So the normal logic is, if there are any #validate handlers on the button, use those. If not, fall back to $form['#validate'] (same goes for #submit).

However, EntityFormInterface::validate() always counts as a #validate callback, so it will never run $form['#validate'].

In HEAD, it was worked around by *always* running $form['#validate'] for entity forms.

My current patch is ensuring that $form['#validate'] is *never* run for entity forms.

We either need to find a middleground, to mimic regular forms, or we need to document how HEAD works, or document that form-level handlers will never run.

This attempts to achieve what I described as the third option in the IS. Working around EntityFormInterface::validate() to mimic the usual form-level vs button-level behavior.

I think this is a flawed approach so far, but I want to keep iterating.

+++ b/core/lib/Drupal/Core/Entity/EntityForm.php
@@ -230,11 +230,16 @@ protected function actions(array $form, FormStateInterface $form_state) {
+  public function validate(array &$form, FormStateInterface $form_state) {
+    $validate_handlers = $form_state->getValidateHandlers();
+    // Remove this method from the list of validate handlers.
+    unset($validate_handlers[array_search('::validate', $validate_handlers)]);
+    // If there are no other validate handlers, make sure the form-level
+    // validate handler runs.
+    if (count($validate_handlers) === 0) {
+      $form_state->setValidateHandlers([]);
+      \Drupal::service('form_validator')->executeValidateHandlers($form, $form_state);
+    }
   }

This is still wrong. Will look at adding more tests first, then trying to fix it again.

@tim.plunkett, can we safely assume we can use this patch for the time being to continue the Mollom module development for Drupal 8? At the moment this is a blocker for porting it to Drupal 8. Also, please let us know if we can do anything to help this move along.

Thanks!

I would not assume anything yet. This patch will break under specific configurations, and a proper solution may not be possible. Instead we might have to go with option #2 from the issue summary, though that is not desirable.

Working on a new approach after discussing with @effulgentsia.
Will update the issue summary if it seems feasible.

Completely overhauling the issue.
I'll open a separate issue for the $form/&$form change.

No interdiff because this was starting over from scratch.

Opened #2459753: EntityForm::validate() should be able to modify the form structure for the other issue.

This should get the number of failures down.

Reverting some of those changes.

Okay, I made some incorrect assumptions. Many of the usages of #suppress_form_level_submit are just to replicate the old approach, and shouldn't be necessary anymore, but this isn't the place to rearchitect that.

Reverted another change to Views.

This should help!

This speaks to a general assumption that the submit handler is always for the "Submit" button. It is for any form submission, and if it should be only for one button, it should be attached as such.

This should be green. Next I'm going to work on reducing the amount of changes, mostly in the entity form. But other than needing more docs, I think this patch should be functional.

I didn't review the whole patch, but the summary and API changes make a lot of sense. I think this is still a good thing although it represents an API changes, as
- it makes entity forms and other forms consistent, thus reducing the number of WTFs
- keeping validation logic by default is safer thus better default

Per #22, I suggested this approach, but I'm still a little unsure about it. For me, it boils down to what should be the meaning of $form['#submit']? Is it the stuff you want to run for all buttons (unless a button specifically opts out of it via '#suppress_form_level_submit', which should be rare)? Do we have any examples in core of such submit functions? I can imagine a contrib one such as a logger, but can we think of any other examples besides logging? OTOH, most of the FormInterface::submitForm() implementations we have are not that, but are actually pretty specific to the 'submit' button. In which case, if we pursue this approach, should we change FormBuilder::prepareForm() from adding '::submitForm' to $form['#submit'] to adding it to $form['actions']['submit']['#submit'] instead?

To complicate the above though, what about FormInterface::validateForm()? Should that by default run for all buttons or only the $form['actions']['submit'] button? From a security standpoint, I suspect it's better to leave that as form-level, but then if we change submitForm() to be button-level, that's kind of introducing a weird split.

Related, issue summary says:

Entity forms will always run button-level and form-level handlers.

But I don't think that's true. It's only true for validate() due to HEAD's implementation of EntityForm::validate(), which has a @todo to change, but it's not true for #submit. So this patch introduces a change to how we deal with #submit that isn't directly justified by an existing inconsistency between entity and non-entity forms.

I still think there's something here that's worth pursuing, just chewing on the above points and curious for feedback from others on them.

Here's an idea: what if we:
- Retain HEAD's behavior with respect to #submit (meaning, button-level always suppresses form-level with no additional #suppress property needed).
- Change HEAD's behavior with respect to #validate as suggested in the current summary (meaning, button-level runs in addition to form-level, unless a #suppress_form_level_validate property is used).

It would mean that #submit and #validate get treated differently, but there's already precedent for that: for example, we have a #element_validate property but no #element_submit.

And I think it would be the smallest change to HEAD, because entity forms would continue running form-level validation regardless of button, and most non-entity forms don't have custom #validate functions on secondary buttons anyway. Besides, given that so much of Drupal 8 has been converted to entities, if we want consistency, I think it's better to make non-entity stuff more consistent with entity stuff than the reverse.

For Drupal 9, I wonder if it will make sense to get rid of form-level #submit entirely, or at least relegate it to minor use cases like logging, but seems like it's too late to do something that drastic in D8.

Decide if form-level handlers should run before or after button-level handlers.

If we're only talking about #validate, then I think it's obvious: before. Just like we run #element_validate before. In other words: make sure the elements are valid, then make sure the form is valid, then make sure to do whatever button-specific validation is needed.

I'm not ready to give up on the patch that changes both, not just yet.

I think we'll want to weight the impact of this API change against the possible ongoing confusion that splitting the behavior of #validate and #submit will cause.

Still haven't quite fixed EntityForm yet, but here's more progress.

Okay, sorted that out.

- Retain HEAD's behavior with respect to #submit (meaning, button-level always suppresses form-level with no additional #suppress property needed).
- Change HEAD's behavior with respect to #validate as suggested in the current summary (meaning, button-level runs in addition to form-level, unless a #suppress_form_level_validate property is used).

hm, that introduces an inconsistency between submit and validated. That's something else than having more features like #element_validate - those don't add confusion.

But I don't think that's true. It's only true for validate() due to HEAD's implementation of EntityForm::validate(), which has a @todo to change, but it's not true for #submit.

I'd say it is true. The default save button runs submit, just as all added buttons e.g. by node. Only buttons like "delete" don't run it, as this functions more like a link and just skips everything. So what that example show us is that running validation and submit usually works together - if I validate the whole entity form I also want to submit it to have the base rebuilding behaviour.

But I agree it would be the smaller API change to only change validation, I'm not so sure it's still the better thing to do. I'd rather leave it consistent an do not run form level validation by default either?

Fixed a few nitpicks.

Rerolled.

Another vote to mark this as RTBC. Probably we need a couple more code reviews

Todo: write tests to ensure that form_alters still work as intended when switching the order of validate/submit handlers.

While I like what this does in theory, I'm still not 100% convinced that the disruption this late in D8 is justified. What's the actual bug in HEAD that is getting in the way of Mollom? If none, then this would need to get reclassified from a bug fix to a task, and a case made that the improvements are worth the disruption.

This not only causes form-level validation handlers to be called twice - once from the ContentEntityForm validate function and once from the FormStateValidator validateForm function.

I'm unable to reproduce this. Which validation function is called twice? What are the steps to reproduce that?

In the first case, the form is not passed by reference and any changes made are lost.

#2459753: EntityForm::validate() should be able to modify the form structure is RTBC. Once that is fixed, and if the "called twice" problem no longer exists either, is there any actual bug left?

#2459753: EntityForm::validate() should be able to modify the form structure just (re)landed. So, if the answer to #53 is that there's nothing in HEAD currently blocking Mollom, then responding to the other motivations for this issue mentioned in the summary:

If button-level handlers are set, the form-level handlers are ignored.

However, this is not true for validation of entity forms.
Entity form submit handlers behave the same way as regular forms.
But entity forms will always run button-level and form-level validate handlers.

Motivation 1 for this issue is to have consistency between non-entity forms and entity forms. Is it possible to achieve that consistency by removing the button-level '#validate' => array('::validate'), overrides and just using the form-level validateForm() (by virtue of there not being a #validate override)? I don't really see much implemented in the current validate() methods that we specifically want not running by default for other buttons that don't otherwise override #validate.

Adding a button-level handler is often more desirable, but it can have the unwanted side-effect of killing the form-level handler.

I agree that this can lead to problems. But it's how Drupal's always worked, so why change now? AFAIK, the only exception to this pattern in Drupal 7 and earlier was node forms, where the button-level validation explicitly forwarded to form-level validators. And in 8.x., we applied that node form pattern to all entity forms, but with a @todo to remove that. So why not remove that as the @todo says and be done? For already ported Drupal 8 modules that didn't see or ignored the @todo and started relying on behavior the @todo said would go away, it would be a BC break, which is a shame. But is that any worse than the alternative (and I think much bigger) BC breaks proposed here? Meanwhile, for people porting from Drupal 7 to Drupal 8 after this gets resolved, the only change would be node forms no longer being the sole exception, rather than redefining the rules for every form.

Also, one more idea: what about adding a FormValidator::executeFormLevelValidateHandlers() method (that executes the form-level handlers regardless of if there's also a button-level one) and letting button-level #validate overrides that want to opt-in to also calling the form-level ones do something like:

$button['#validate'][] = 'form_validator:executeFormLevelValidateHandlers';

Would that solve some of the goals of this issue without any BC break, just an addition?

FWIW, I filed a security issue for a D7 contrib module a couple months ago, that was caused by overlooking that "button validate override form-level validate" behavior - https://security.drupal.org/node/145458 if that's of interest (that URL is only accessible by the sec team)

See also #2022875: Resolve difference between submitForm(), submit(), and save() in EntityFormController

From #54:

Is it possible to achieve that consistency by removing the button-level '#validate' => array('::validate'), overrides and just using the form-level validateForm() (by virtue of there not being a #validate override)?

From #2022875-62: Resolve difference between submitForm(), submit(), and save() in EntityFormController:

I would prefer to look into validate()/validateForm() in a separate issue.

So, here's a patch that tries that out. No interdiff, because this is a totally different approach from #48. If folks here prefer to continue this issue with the direction of #48, I'm okay with moving this patch's approach to a separate issue.

Picking up this approach.

Patch looks good to me, it's an API change but one that makes sense and helps to clean up differences between normal forms and entity forms.

issue summary and title needs an update I think.

Working on tests now.
Still needs an issue summary update, but that title seems more correct.

The new approach makes sense to me also - I like how it makes entity-forms less special.

It does not address the issue of easily missing entity validation when you add a custom validation handler to a button. That's not nice security wise, but existing, general form behaviour.

Idk where my tests went, I thought I posted a patch here.
This is just a reroll.

@fago Could you review this?

Bumping this up. Could someone, with knowledge of this subsystem, review this please? This is critical to the Mollom module that it gets in Drupal core or it won't work. Thanks!

Per #68

If this is a confirmed contrib blocker, and a regression from D7 to D8 (in that it extended an undesirable feature/bug of node forms to all entity forms), I think it makes sense to raise it to Critical for now, to be discussed on the next critical triage call if it doesn't land by then.

I'm fine with this change too.

Looks like sadly the tests that Tim still need to be recovered or rewritten :(

Also, issue summary needs an update, and we need a beta evaluation. It's critical yes, but we need to summarize why.

Yep, I already did - as said the new approach works for me and the patch looks good also. But as berdir points out, this misses tests, an issue summary update and beta evaluation.

1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityFormInterface.php
   @@ -61,4 +61,21 @@ public function getFormLangcode(FormStateInterface $form_state);
   +   * {@inheritdoc}
   +   *
   +   * Note that extending classes should not override this method to add entity
   +   * validation logic, but define further validation constraints using the
   +   * entity validation API and/or provide a new validation constraint if
   +   * necessary. This is the only way to ensure that the validation logic
   +   * is correctly applied independently of form submissions; e.g., for REST
   +   * requests.
   +   * For more information about entity validation, see
   +   * https://www.drupal.org/node/2015613.
   +   *
   +   * @return \Drupal\Core\Entity\ContentEntityTypeInterface
   +   *   The built entity.
   

   Should this also mention how to use the constraints for custom form elements just defined by the particular class?

2. +++ b/core/modules/contact/src/MessageForm.php
   @@ -168,8 +168,8 @@ public function actions(array $form, FormStateInterface $form_state) {
        $elements['preview'] = array(
   +      '#type' => 'submit',
   

   Sometimes it seems just random that things actually work

3. +++ b/core/modules/menu_ui/menu_ui.module
   @@ -412,7 +412,7 @@ function menu_ui_form_node_type_form_alter(&$form, FormStateInterface $form_stat
   -  $form['actions']['submit']['#validate'][] = 'menu_ui_form_node_type_form_validate';
   +  $form['#validate'][] = 'menu_ui_form_node_type_form_validate';
   

   This is soo much better

First just a reroll. Working on this today.

Bad reroll.

Fixing recently added tests.

@tim.plunkett
So do you think we should do the improvement of the doc, see my comment 1?

For the test, we need to:

• Create a test form (subclass of EntityForm)
• add a form-level #validate
• ensure that it runs and can modify the $form

In Drupal\system\Tests\Form, we have AlterTest for ensuring that hook_form_alter() runs, and we have ValidationTest, which seems to have coverage for #validate handlers being able to modify $form, but I think it only covers the validateForm() method itself, not #validate added by hook_form_alter().

So, per #84, I think it would be good to add a test that:
- hook_form_alter() can add a $form['#validate']
- that when it does, the form class's validateForm() and the added #validate both run
- that both can modify $form
- that all of the above is true for both a non-entity form and an entity form

NW for #85 and #84.

Working on the missing stuff.

I updated the IS with the currently implemented solution (hopefully I got it right :)

I tried in good faith to write a beta evaluation for this, but I can't say I'm completely ok with the current approach:

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes

Issue category	Bug because we have a regression from D7 to D8, in that in D8 we extended an undesirable feature/bug of node forms to all entity forms. This was done during the initial conversion of entity forms to the new system, but was meant to be removed before release.
Issue priority	Critical because the current behavior cannot be fixed without an API change and should definitely not be part of the final Entity Form API. Additionally this inconsistency introduced a contribute module blocker, because the EntityFormInterface::validate() method prevents form and form state from being altered.
Prioritized changes	The main goal of this issue is making APIs more consistent, as entity forms will behave more like regular forms, and reduce technical debt.
Disruption	Disruptive for core/contributed and custom modules overriding entity validation or providing additional submit-level validation handlers, as those will suddenly prevent form-level validation from happening, which means among the rest that $entity->validate(); won't run.

What made me stop and think is the Disruption paragraph: that basically means that a single contrib module not updated to accommodate for this change may be enough to completely break validation and introduce potentially huge security issues.

I'm wondering whether it would make sense to fix this in a slightly different way:

• Remove EntityFormInterface::validate() and move the related logic to FormInterface::validateForm() implementations.
• Remove manual triggering of validation in ContentEntityForm::validateForm()
• Add helper methods to FormBuilder to declare validation and submission handlers, figuring out whether a handler needs to be specific to one submit action or global. The latter would add the handler to all the available submit buttons. This way we can keep button-level validation and we avoid the risk of breaking "global" validation handlers. I'm not sure we have any of them in core, btw, as ContentEntityForm::validateForm() makes sense mainly for the save action.

Bug because we have a regression from D7 to D8, in that in D8 we extended an undesirable feature/bug of node forms to all entity forms.

I was wrong when I wrote this in #71. Or rather, I mixed up #validate and #submit.

In D7, node_form_submit_build_node(), called by a button-level #submit, executes the form-level #submit handlers. That has already been removed from D8.

Also, in D7, only the node form uses a button-level #submit for the primary "Submit" button. Other entity forms, such as comment and user, use the form-level submit. In D8, all entity forms use button-level #submit. This patch does not change that. But this difference from D7 is ok, because different buttons do different things, so if you're a module that wants to add an additional #submit handler to some entity form, you probably need to decide which button to target for that anyway.

But in D7, button-level #validate was very rare: the Upload and Remove buttons in file.module, the Add image effect and Override image style buttons in image.module, the Update bulk operation in node.module, and the Add role button in user.module. But in D8 HEAD, usage of button-level #validate was added to all entity forms, which I think is confusing. This patch removes that back, restoring consistency with D7. This patch does not change the non-entity-action buttons, such as Upload and Remove for file.module, so even there, we retain consistency with D7.

What made me stop and think is the Disruption paragraph: that basically means that a single contrib module not updated to accommodate for this change may be enough to completely break validation and introduce potentially huge security issues.

In D7, if a module added a button-level #validate, it prevented form-level validation. In D8, same is true for non-entity forms. So, you're talking about a D8-only module that is intentionally adding a button-level #validate on an entity form and relying on the fact that only for entity forms, in D8 HEAD, that does not currently suppress form-level validation?

So, I think we have 2 issues here:

1) The critical portion is that EntityFormInterface::validate() doesn't take $form by reference. That prevents Mollom from being able to modify the form during validation, which is a necessary thing to do for captcha processing. I think that can be fixed just by changing the signature from $form to &$form.

2) The maybe-not-critical portion is that D8 HEAD's current state of using a button-level #validate of EntityFormInterface::validate() as distinct from a form-level #validate for FormInterface::validateForm(), but having the former invoke the latter, is a) weird, b) inconsistent with D7, and c) inconsistent with non-entity forms. This patch fixes all that, but per #89, could be disruptive to already ported D8 modules that were relying on that weirdness.

+1 on #92, I reached the same conclusions independently here at the sprint :)

So, you're talking about a D8-only module that is intentionally adding a button-level #validate on an entity form and relying on the fact that only for entity forms, in D8 HEAD, that does not currently suppress form-level validation?

Yes. Well, more or less :)

The D8-only assumption is probably fair in most cases but misleading: if a D7 module were updated to use button-level form handlers while being ported to D8 we would run into the same problems.

Since 2) might in a sense be critical too, after discussing this with @tim, I tried to put together a proof-of-concept to see whether my proposed solution makes any sense. The attached patch seems to work on manual testing, obviously it's not meant to be committed as-is, but just to showcase the suggested API :)

Edit: I think adding a #submit_id key to submit elements makes sense independently of this issue because without it, identifying the triggering element provided by the form state is very difficult without relying on its human-readable label, which is quite fragile. However, as you can see, the patch does not depend on that.

Nothing to do here until I get some feedback on #94.

Discussed this with @amateescu, @dawehner, @effulgentsia and @Wim. Final proposal: add a flag to determine whether entity validation has run and throw an exception before saving the entity if it didn't. This will prevent a rogue module from breaking validation without noticing it.

On this again

This implements the discussed fix. Additional test coverage still to do.

Added draft CR at https://www.drupal.org/node/2524364

Added beta evaluation

+1 to #98, looks great!

Works for me!
Still needs tests, I won't have time until Monday to write them, so feel free if you have time.

I have almost working tests, I will complete them tomorrow.

Looks great to me, just some nitpicks and partially related comments. Not sure how much cleanup we want to do here, but e.g. using the return value of save() might actually be easier than the current change of using isNew() before saving.

1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityConfirmFormBase.php
   @@ -121,9 +118,10 @@ public function delete(array $form, FormStateInterface $form_state) {}
   -  public function validate(array $form, FormStateInterface $form_state) {
   +  public function validateForm(array &$form, FormStateInterface $form_state) {
        // Override the default validation implementation as it is not necessary
        // nor possible to validate an entity in a confirmation form.
   +    return $this->buildEntity($form, $form_state);
      }
   

   Why do we need to build the entity in a confirm form? They don't usually change anything in the entity apart from inside the submit/save method?

2. +++ b/core/modules/aggregator/src/FeedForm.php
   @@ -22,7 +22,10 @@ class FeedForm extends ContentEntityForm {
      public function save(array $form, FormStateInterface $form_state) {
        $feed = $this->entity;
        $insert = (bool) $feed->id();
   -    $feed->save();
   +
   +    // Make sure we do not skip the parent's entity save logic.
   +    parent::save($form, $form_state);
   +
   

   If you're changing this anyway, parent::save() actually returns the return value of $entity->save, so you could just use that for the new/update check. Not sure how much cleanup we want to do here, might also be a nice novice follow-up to clean up our save methods.

3. +++ b/core/modules/block_content/src/BlockContentForm.php
   @@ -224,7 +227,8 @@ public function save(array $form, FormStateInterface $form_state) {
      public function validateForm(array &$form, FormStateInterface $form_state) {
   -    if ($this->entity->isNew()) {
   +    $entity = parent::validateForm($form, $form_state);
   +    if ($entity->isNew()) {
          $exists = $this->blockContentStorage->loadByProperties(array('info' => $form_state->getValue(['info', 0, 'value'])));
   

   This is one of the cases that violate that documentation that we have up there. There's an issue for this, however.

4. +++ b/core/modules/comment/src/CommentForm.php
   @@ -361,7 +360,9 @@ public function save(array $form, FormStateInterface $form_state) {
        if ($this->currentUser->hasPermission('post comments') && ($this->currentUser->hasPermission('administer comments') || $entity->{$field_name}->status == CommentItemInterface::OPEN)) {
   -      $comment->save();
   +      // Make sure we do not skip the parent's entity save logic.
   

   access checks in save() are super weird but that's unrelated.

5. +++ b/core/modules/menu_link_content/src/Form/MenuLinkContentForm.php
   @@ -120,18 +121,18 @@ public function buildEntity(array $form, FormStateInterface $form_state) {
      public function save(array $form, FormStateInterface $form_state) {
   -    // The entity is rebuilt in parent::submit().
   -    $menu_link = $this->entity;
   -    $saved = $menu_link->save();
   +    try {
   +      // Make sure we do not skip the parent's entity save logic.
   +      parent::save($form, $form_state);
    
   -    if ($saved) {
   +      // The entity is rebuilt in parent::submit().
   

   using if ($saved) like this doesn't make sense. try/catch is doing what it was meant to do, but do we really need it? Especially if we're eating the actual exception message and don't display it. Maybe just drop the if without replacing it? Or make it part of the mentioned clean-up-save follow-up.

6. +++ b/core/modules/menu_ui/menu_ui.module
   @@ -415,7 +415,7 @@ function menu_ui_form_node_type_form_alter(&$form, FormStateInterface $form_stat
    
   -  $form['actions']['submit']['#validate'][] = 'menu_ui_form_node_type_form_validate';
   +  $form['#validate'][] = 'menu_ui_form_node_type_form_validate';
      $form['#entity_builders'][] = 'menu_ui_form_node_type_form_builder';
    }
    
   

   I was wondering how many contrib/custom modules are doing something like this.

   I found exactly one case in a custom module in our project, no contrib modules in all the modules that we use/I ever cloned.

   Given that, I think the impact of the API change is going to be quite limited.

7. +++ b/core/modules/taxonomy/src/TermForm.php
   @@ -96,8 +96,8 @@ public function form(array $form, FormStateInterface $form_state) {
       */
   -  public function validate(array $form, FormStateInterface $form_state) {
   -    parent::validate($form, $form_state);
   +  public function validateForm(array &$form, FormStateInterface $form_state) {
   +    parent::validateForm($form, $form_state);
    
        // Ensure numeric values.
        if ($form_state->hasValue('weight') && !is_numeric($form_state->getValue('weight'))) {
   

   This is another bad usage of validateForm(). Not to be fixed here but we should open a follow-up.

   It's just integer validation, which should actually just work. Only problem could be that weight is in a fieldset, not sure if the validation logic will find it there?

   That follow-up could also change the form element to #type number, then we have browser validation already as well.

8. +++ b/core/modules/user/src/RegisterForm.php
   @@ -102,9 +102,10 @@ public function save(array $form, FormStateInterface $form_state) {
    
   -    // Save has no return value so this cannot be tested.
   -    // Assume save has gone through correctly.
   -    $account->save();
   +    // Save has no return value so this cannot be tested. Assume save has gone
   +    // through correctly. Make sure we do not skip the parent's entity save
   +    // logic.
   +    parent::save($form, $form_state);
   

   The first comment is completely bogus. If someone goes wrong, then an exception is thrown. Just replace it instead of adding to it?

This implements additional test coverage and should address #104.

Test-only patch

Rerolled test-only patch

So, now we have:

• additional test coverage
• a draft change record
• a beta evaluation
• and especially a green patch (see #105)

Go for it!

+++ b/core/modules/system/tests/modules/entity_test/src/EntityTestForm.php
@@ -51,35 +51,40 @@ public function form(array $form, FormStateInterface $form_state) {
+      \Drupal::state()->set('entity_test.form.save.exception', get_class($e) . ': ' . $e->getMessage());

It just makes me sad that we don't let the exception be thrown

It just makes me sad that we don't let the exception be thrown

Life is sad sometimes :)

1. +++ b/core/modules/menu_link_content/src/Form/MenuLinkContentForm.php
   @@ -120,21 +121,15 @@ public function buildEntity(array $form, FormStateInterface $form_state) {
      public function save(array $form, FormStateInterface $form_state) {
   +    // Make sure we do not skip the parent's entity save logic.
   +    parent::save($form, $form_state);
   +
        // The entity is rebuilt in parent::submit().
   -    $menu_link = $this->entity;
   -    $saved = $menu_link->save();
   -
   -    if ($saved) {
   -      drupal_set_message($this->t('The menu link has been saved.'));
   -      $form_state->setRedirect(
   -        'entity.menu_link_content.canonical',
   -        array('menu_link_content' => $menu_link->id())
   -      );
   -    }
   -    else {
   -      drupal_set_message($this->t('There was an error saving the menu link.'), 'error');
   -      $form_state->setRebuild();
   -    }
   +    drupal_set_message($this->t('The menu link has been saved.'));
   +    $form_state->setRedirect(
   +      'entity.menu_link_content.canonical',
   +      array('menu_link_content' => $this->entity->id())
   +    );
      }
    
    }
   diff --git a/core/modules/menu_ui/menu_ui.module b/core/modules/menu_ui/menu_ui.module
   

   +1 to do less wrong things

RTBC + 1 (reviewed the patch again in depth)

Created follow-up @Berdir was suggesting in #104: #2525794: Improve consistency in EntityFormInterface::save() implementations.

I took another look at the latest patch. It overally looks great - thanks!

Here a few minor remarks:

+++ b/core/modules/forum/src/Form/ForumForm.php
@@ -76,8 +76,14 @@ public function buildEntity(array $form, FormStateInterface $form_state) {
+    // See ContentEntityForm::save().
+    if (!$form_state->getTemporaryValue('entity_validated')) {

This shows a little problem with the re-usability of the save() method. As soon as a parent (like TermForm) overrides save() we cannot re-use the parent implementation. The solution is copying those few lines of code, but that poses maintainability problems: We are not able to update this few lines of codes centrally any more. What about moving that simple check to a helper method "ensureEntityIsValidated()" which can be easily re-used even if parent save() implementations cannot?

Besides that, I think the change record could be a bit improved to clarify when it's required to have a custom button-level validator calling the main validation and when not?
I think that documentation would be good to have at validateform() in general, such that non-upgrades are able to find it also.

+++ b/core/lib/Drupal/Core/Entity/ContentEntityForm.php
@@ -87,14 +79,25 @@ public function validate(array $form, FormStateInterface $form_state) {
   /**
+   * {@inheritdoc}
+   */
+  public function save(array $form, FormStateInterface $form_state) {
+    // Make sure button-level validation handlers do not prevent entity
+    // validation from being executed.
+    if (!$form_state->getTemporaryValue('entity_validated')) {
+      throw new \LogicException('Entity validation was skipped.');
+    }
+    return parent::save($form, $form_state);
+  }

I discussed this with @dawehner and @plach. I'm not sure that requiring EntityForm::save() implementation to call parent for ensuring validation is a great idea. We should set a requireValidation flag on the entity in EntityForm::buildEntity and ensure it has been set to TRUE in EntityInterface::save().

+++ b/core/modules/menu_link_content/src/Form/MenuLinkContentForm.php
@@ -9,6 +9,7 @@
+use Drupal\Core\Entity\EntityStorageException;

Not used

@plach is looking at this.

Addressed #116 and #117.

+++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
@@ -152,6 +152,20 @@
+   * @bool
...
+   * @bool

Bad PHP docs

Thanks @eshta, it would be great if you could test the last iteration. I don't expect it to be different from #105 wrt to Mollom, but better safe than sorry :)

1. +++ b/core/modules/system/src/Tests/Entity/EntityFormTest.php
   @@ -167,7 +167,7 @@ public function testValidationHandlers() {
        $this->drupalPostForm('entity_test/add', [], 'Save');
   -    $this->assertEqual($state->get('entity_test.form.save.exception'), 'LogicException: Entity validation was skipped.', 'Button-level validation handlers behave correctly.');
   +    $this->assertEqual($state->get('entity_test.form.save.exception'), 'Drupal\Core\Entity\EntityStorageException: Entity validation was skipped.', 'Button-level validation handlers behave correctly.');
      }
    
   

   I hate the fact that we eat the original exception there and our exception handler can't deal with nested exceptions at all. Unrelated but I think we should change that to re-throw the original exception until we can display it in a useful way

2. +++ b/core/modules/user/src/RegisterForm.php
   @@ -102,8 +102,9 @@ public function save(array $form, FormStateInterface $form_state) {
    
   -    // Make sure we do not skip the parent's entity save logic.
   -    parent::save($form, $form_state);
   +    // Save has no return value so this cannot be tested.
   +    // Assume save has gone through correctly.
   +    $account->save();
   

   Kinda sad to see this nonense re-added (or the removal reverted). Can we explicitly mention this in the follow-up that you opened?

This addresses the reviews from @fago and @alexpott, and #119 is green (#120 is just a doc fix). RTBC again :)

1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
   @@ -334,6 +348,22 @@ public function isTranslatable() {
   +  public function preSave(EntityStorageInterface $storage) {
   +    // An entity requiring validation should be saved if it has not been
   +    // actually validated.
   +    if ($this->validationRequired && !$this->validated) {
   +      throw new \LogicException('Entity validation was skipped.');
   +    }
   +    else {
   +      $this->validated = FALSE;
   +    }
   +
   +    parent::preSave($storage);
   +  }
   +
   +  /**
   +   * {@inheritdoc}
   +   */
      public function preSaveRevision(EntityStorageInterface $storage, \stdClass $record) {
   

   We could have some quick unit test here \Drupal\Tests\Core\Entity\ContentEntityBaseUnitTest, do you think we should do?

2. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
   @@ -334,6 +348,22 @@ public function isTranslatable() {
   +      throw new \LogicException('Entity validation was skipped.');
   

   We could add a todo to the assertion issue: #2408013: Adding Assertions to Drupal - Test Tools.

3. +++ b/core/modules/aggregator/src/FeedForm.php
   @@ -22,10 +22,7 @@ class FeedForm extends ContentEntityForm {
   -
   -    // Make sure we do not skip the parent's entity save logic.
   -    parent::save($form, $form_state);
   -
   +    $feed->save();
   

   <3

Addressed #125 <3

Removed some useless code

Even better

I like how we are fixing this issue and ensuring on lower levels of the API that we are doing sane things.

Thank you for also providing unit test coverage ... so yeah I'm pretty happy with it.

RTBC + 1

I'm not qualified to RTBC this patch, but wow — much more elegant :)

Discussed the last iteration with @alexpott, so assigning to him to make sure he has a chance to perform a final review.

Minimal PHP docs / comments clean-up, keeping the RTBC status.

Leaving this for alexpott, but I caught up with the progress here and I like the final version of the patch, looks solid.

Committed de19304 and pushed to 8.0.x. Thanks!

I've credited myself for reviewing this issue since #117 had a large architectural impact on the patch.

Well deserved, @alexpott++

Ok, help me understand this. Why does adding submit-level validation cause the form-level validation to *not* run?

I had to move my form-level validations to the submit buttons because of an AJAX call in a contrib module - an "Add another item" button on a multi-value reference field. When I did, I started getting the "Entity validation was skipped." error.

So now I'm stuck between two incompatible methods. What to do?