NodeFormController::validate() starts with
$node = $this->buildEntity($form, $form_state);
and then calls parent::validate(), which does its own call to
$entity = $this->buildEntity($form, $form_state);

The #entity_builders callbacks and field_attach_submit() are thus called twice for validation only.
With EntityFormController::submit() performing its own call to buildEntity(), that's 3 iterations for one node edit submission.

CommentFileSizeAuthor
#1 1768526-1.patch1.58 KBswentel
#7 1768526-7.patch3.33 KBBerdir
#10 interdiff.txt2.9 KBswentel
#10 1768526-10.patch4.06 KBswentel
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

swentel’s picture

swentel CreditAttribution: swentel commented
Issue summary: View changes
Status: Active » Needs review
Related issues: +#1938352: Things (for a lack of a better description) are happening twice with some entity forms
FileSize
1768526-1.patch1.58 KB

#1938352: Things (for a lack of a better description) are happening twice with some entity forms is maybe related.

Just trying out something here.

Status: Needs review » Needs work

The last submitted patch, 1: 1768526-1.patch, failed testing.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented

That will actually survive form submissions because we serialize and then unserialize the (entity) from object, so that won't work. We need to rebuild after re-submitting.

yched’s picture

yched CreditAttribution: yched commented
Priority: Normal » Major

Bumping the issue, and also bumping the priority.

On a node form submit, we run "extract form values into field values" 3 times on all fields. Same for aggregator feed forms :
NodeForm::validate() & FeedForm::validate() each call buildEntity() for their own logic, and then call parent::validate(), which calls buildEntity() itself as if we hadn't just built it.

Feels a little absurd that the entity being built is lost between MyEntityForm::validate() and the parent implementation ?
Also feels a little absurd that we'll need to rebuild the entity once more in the submit ?

yched’s picture

yched CreditAttribution: yched commented

This was mentioned in #2406103: Remove hook_node_validate() and hook_node_submit() because they bypass the entity API, which removes most of, but not all, the content of NodeForm::validate().

In #19 / #20 over there:

[@Berdir] we should be able to fix that by returning $entity in parent::validate(), just like we already do for save()?
[@yched] That sound like a plan, yes, it feels silly to have each override of validate() re-do a buildEntity() because the results are lost between parent and child implementations.

Any takers ? :-)

yched’s picture

yched CreditAttribution: yched commented
Issue tags: +Performance
Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Status: Needs work » Needs review
FileSize
1768526-7.patch3.33 KB

Something like this?

yched’s picture

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

Sweet !

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Needs work 
+++ b/core/modules/comment/src/CommentForm.php
@@ -287,8 +287,7 @@ public function buildEntity(array $form, FormStateInterface $form_state) {
    * {@inheritdoc}
    */
   public function validate(array $form, FormStateInterface $form_state) {
...
+    $comment = parent::validate($form, $form_state);

Now the inheritdoc is wrong because this implementation does not return the entity. Should we change EntityFormInterface::validate to return the entity? And then fix all implementations?

swentel’s picture

swentel CreditAttribution: swentel commented
Status: Needs work » Needs review
FileSize
1768526-10.patch4.06 KB
interdiff.txt2.9 KB

Like this ? I guess it makes sense to return because when for some reason I'd swap out one of these forms and extend on the defaults, I wouldn't have the validated entity in my custom class.

yched’s picture

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

Thanks @swentel :-)

alexpott’s picture

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

This issue addresses a major bug and is allowed per https://www.drupal.org/core/beta-changes. Committed 0b5f5ce and pushed to 8.0.x. Thanks!

  • alexpott committed 0b5f5ce on 8.0.x 
    Issue #1768526 by swentel, Berdir: NodeFormController::validate() calls...
fago’s picture

fago
Primary language German
Location Vienna
CreditAttribution: fago commented

#2443797: Follow-up to fix EntityFormInterface::validate() return documentation

+++ b/core/lib/Drupal/Core/Entity/EntityFormInterface.php
@@ -101,6 +101,9 @@ public function buildEntity(array $form, FormStateInterface $form_state);
+   * @return \Drupal\Core\Entity\ContentEntityTypeInterface
+   *   The built entity.

That was wrong - created #2443797: Follow-up to fix EntityFormInterface::validate() return documentation.

Status: Fixed » Closed (fixed)

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