EntityForm::validate() should be able to modify the form structure

Because:
FormInterface::validateForm(array &$form, FormStateInterface $form_state)

… it's a necessity for EntityForm to follow that too.

Which makes me wonder how this was even possible? I suppose because PHP's interface inheritance doesn't actually check pass-by-reference vs. pass-by-value?

Tested this patch out locally and it worked as expected. It also bring consistency which is a beautiful thing.

This is disruptive for contrib but it is blocking contributed projects and it will be simple to fix. Committed d4d8357 and pushed to 8.0.x. Thanks!

This broke HEAD for example Drupal\entity_reference\Tests\EntityReferenceFieldDefaultValueTest

Thanks for the fast turnaround.

Committed and pushed to 8.0.x.

#13 says #12 was pushed, but I don't see that to be the case, so back to RTBC.

Can't even blame drupal.org this time. :)

Would have been nice to at least have a beta evaluation here and an explanation of why this is actually a problem and why you're supposed to change $form there. This broke a lot of contrib modules.

I'm going to revert for now - to get a good reason for how this blocks contrib. Let's get a beta evaluation to the issue summary.

This was originally proposed as part of https://www.drupal.org/node/2453175
In the original summary there you can see that entity level form validators to be called twice, but since $form is not passed by reference, any modifications will be lost.

Why you might need to update the $form structure in the validation phase?

One popular example: A form containing a #required element, and a CAPTCHA element. The user successfully champions the CAPTCHA, but fails to enter something into the #required element. In this case, the validation handler of the CAPTCHA needs to be able to alter $form or $element to hide the CAPTCHA.

Also - by changing this in D8 to not pass by reference, core functionality is modified and a bug is introduced. While its unfortunate that early D8 adopter contrib modules followed this bug, it's better to catch it now.

So I kind of get the original issue.

And this is inconsistent with FormBase, but validate() vs. validateForm() is actually the bigger inconsistency there...

So this only affects *entity forms* that are trying to do something in validate(). It doesn't affect form alters, it doesn't affect other forms.

So how does this *block* a contrib module, the only case where this affects you if it's your own entity form, and then you could as a (pretty ugly) workaround use a different validate callback? I'd love to see a specific use case :)

I get that we'd like to clean this up and make it more consistent, but is that worth the API change?

In the case of Mollom, for example, we may serve a CAPTCHA element on the page if textual analysis of the content is unsure. This CAPTCHA is attached to some existing entity like a comment post or a new node. Therefore we are altering the entity form to add our own form element. We also add form level validators as we need access to all of the text on the page to perform textual analysis. If the user of the form fills in the CAPTCHA correctly but still fails some other validation (plus there is the rabbit hole of preview vs. save) then we need to be able to track that throughout the form and hide the form level CAPTCHA element. This was always possible in Drupal 7 as the $form was passed by reference.

As you can see from the example above, this isn't using our own entity form nor would that be a possibility as we need to attach to existing entities in order to protect them.

So how does this *block* a contrib module, the only case where this affects you if it's your own entity form

The issue is that (Content)EntityForm::validate() call \Drupal::service('form_validator')->executeValidateHandlers($form, $form_state);, so a module like Mollom that adds to the form-level $form['#validate'] of various entity (and non-entity) forms and expects $form by reference, doesn't get it by reference due to the button-level validate() wrapper not getting it by reference.

but validate() vs. validateForm() is actually the bigger inconsistency there

Right. The question is how to make them consistent. This issue's approach is to at least make them consistent in their signature. I also just now added a patch to #2453175-59: Remove EntityFormInterface::validate() and stop using button-level validation by default in entity forms to try out removing validate() entirely.

Ok, I see, that validate processing is a problem, we wanted to get rid of that, but I can see how the button-level issue is blocking that.

Yes, I've seen the update there, didn't look at the patch yet. Doing this first would result in two API changes for modules that are trying to keep up, maybe we want to wait a bit and see if that approach is going to work out, then we don't need this anymore?

One very annoying related problem is that it's crazy complicated to alter the correct submit button of a node form, if there would only be one then it wouldn't be that hard, then it would be a lot easier to add the validation to the button. but that's a different topic ;)

Closing as duplicate of #2453175: Remove EntityFormInterface::validate() and stop using button-level validation by default in entity forms, this method no longer exists.