Rename field_attach_submit(), because it is also called from within validation [#1846648]

Posted by Wim Leers on November 21, 2012 at 11:06am

9 followers

Issue Summary

Coming from #1824500-16: In-place editing for Fields:

+++ b/core/modules/edit/includes/form.inc @@ -0,0 +1,147 @@ + // 'submit' in D8 is for "building the entity object", not for actual + // submission. It appears though that if there were no validation errors, it + // is submitted automatically. + field_attach_submit($entity->entityType(), $entity, $form, $form_state, $options);

1. I don't understand the second sentence of this comment. What exactly is happening automatically, and is that good or bad?
2. Please open an issue for us to rename field_attach_submit(), because in both HEAD, and here, it is called from within validation, and that is very confusing.
Add a @todo comment here linking to that issue.

And my explanation:

In D8, there is no actual submit handler, the EntityFormController stuff (or whichever class it is) takes care of that. IIRC, field_attach_submit() (just maps the form values onto the entity object (see entity_form_submit_build_entity() and EntityFormController::buildEntity()), field_attach_validate() validates the entity object itself, and if the EntityFormController notices there are zero validation errors, then it'll just save the entity object. No submit handler is involved at all. It took me a very long time to figure out this bizarre way of handling validation/saving. I'm sure it's just an artifact of the major changes that have been going on though :)

We need to fix this :)

Change records for this issue

field_attach_submit() renamed to field_attach_extract_form_values()

Comments

#1

Posted by yched on November 21, 2012 at 4:20pm

Yes, I noticed that when working on "widgets as plugins".
At some point during the EntityFormController work, field_attach_submit() calls got moved to EntityFormController::buildEntity(), which runs during validate.
Thus, I'm not sure what's the meaning of 'submit' anymore :-)

On this topic, also see #1768526: NodeFormController::validate() calls buildEntity() twice

#2

Posted by Wim Leers on November 21, 2012 at 4:37pm

Hehe, exactly :)

It's like there's this alternative Field API universe where "submit" means something completely different :)

Anyway, not a big issue, but it is a big WTF for those reading the code of course. I'm sure it'll get fixed :)

#3

Posted by yched on November 21, 2012 at 4:44pm

Well, it used to mean "submit", but then the outside world changed the moment it called the function :-)

#4

Posted by yched on November 21, 2012 at 4:54pm

So it seems something like field_attach_build() would be a more accurate name. It does lack a bit of 'form' context though.
[edit: also, 'form build' is yet something else for FAPI...]

Suggestions welcome :-)

#5

Posted by effulgentsia on November 29, 2012 at 1:17am

FWIW, the Symfony Forms component calls this bind(). So, I'm tentatively thinking field_attach_form_bind(). Without knowing Symfony's terminology, this is a little unintuitive though (at least for me). field_attach_form_bind_input(), field_attach_form_bind_values(), or field_attach_form_bind_submission() would all be more explicit, but then again, we don't add any of those suffixes for field_attach_form_validate() (though with validate, it's more clear, since what can you possibly validate other than the input/values/submission).

#6

Posted by yched on November 29, 2012 at 1:21am

I kind of like "bind". But, true, it would be a little isolated on the drupal side. Unless there are other parts of the form flow that could be named accordingly ?

#7

Posted by effulgentsia on November 29, 2012 at 1:39am

field_attach_submit() just maps the form values onto the entity object (see entity_form_submit_build_entity() and EntityFormController::buildEntity())

Note that there's a very important thing about buildEntity(), which is that it clones $form_state['entity'], and then calls field_attach_submit() and field_attach_form_validate() only on that clone. $form_state['entity'] is only updated during EntityFormController::submit(), which like any form submit handler, only runs if the form submission was valid. This way, invalid user input can never leak into $form_state['entity'].

if the EntityFormController notices there are zero validation errors, then it'll just save the entity object. No submit handler is involved at all. It took me a very long time to figure out this bizarre way of handling validation/saving.

EntityFormController::actions() creates a submit button and sets its #submit to its own 'submit' and 'save' methods. What's bizarre about that? It's two perfectly reasonable submit handlers, which like all submit handlers, only run if the form validated successfully.

What I do agree is bizarre though is that we name the function that copies $form element values to $entity field values (with the possibility of this not necessarily being a direct copy, but can include data transformation of some kind) "submit". Which is what this issue is trying to address.

#8

Posted by effulgentsia on November 29, 2012 at 1:43am

What about field_attach_bind_form_values()? Since we're not binding straight user input/submission: FAPI has already done that prior to setting $element['#value']/$form_state['values']. Instead, we're doing a separate process of binding form values to entity field values.

Or, field_attach_bind_from_form()?

#9

Posted by yched on December 1, 2012 at 1:35pm

re #8:
Actually, 'extract_form_values' was an 'op' in D7 (field_default_extract_form_values(), was called in both field_attach_submit() and field_attach_validate().

So I guess, yes, reintroducing that extract_form_values denomination would be the way to go.
field_attach_submit() -> field_attach_extract_form_values()
WidgetInterface::submit() -> WidgetInterface::extractFormValues()

Or "bind" rather than "extract", but "extract" is what we have in D7, and "bind" isn't used anywhere else in core right now AFAIK.
I don't have a strong opinion here.

#10

Posted by yched on December 3, 2012 at 10:28am

I'll submit a patch for the rename when "edit in place" is committed.
Better to avoid a reroll there...

#11

Posted by Wim Leers on December 3, 2012 at 3:23pm

#10: Thanks for your consideration :)

#12

Posted by yched on December 21, 2012 at 6:02pm

Status:

active

» needs review

OK, #1824500: In-place editing for Fields got in (kudos !).
Here's a patch.

Attachment	Size	Status	Test result	Operations
field_attach_submit-1846648-12.patch	14.48 KB	Idle	FAILED: [[SimpleTest]]: [MySQL] 46,053 pass(es), 2,723 fail(s), and 2,014 exception(s).	View details

#13

Posted by System Message on December 21, 2012 at 6:51pm

Status:

needs review

» needs work

The last submitted patch, field_attach_submit-1846648-12.patch, failed testing.

#14

Posted by yched on December 21, 2012 at 8:01pm

Status:

needs work

» needs review

Er, rather the non half-baked version, then.

Attachment	Size	Status	Test result	Operations
field_attach_submit-1846648-14.patch	14.77 KB	Idle	FAILED: [[SimpleTest]]: [MySQL] 49,191 pass(es), 32 fail(s), and 7 exception(s).	View details

#15

Posted by System Message on December 21, 2012 at 8:51pm

Status:

needs review

» needs work

The last submitted patch, field_attach_submit-1846648-14.patch, failed testing.

#16

Posted by yched on December 21, 2012 at 9:21pm

Status:

needs work

» needs review

Forgot that field_ui calls the method directly.

Attachment	Size	Status	Test result	Operations
field_attach_submit-1846648-16.patch	15.91 KB	Idle	PASSED: [[SimpleTest]]: [MySQL] 49,347 pass(es).	View details

#17

Posted by swentel on January 21, 2013 at 5:18pm

Rerolled after removal of $entity_type parameter.

Attachment	Size	Status	Test result	Operations
field_attach_submit-1846648-17.patch	15.59 KB	Idle	FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch field_attach_submit-1846648-17.patch. Unable to apply patch. See the log in the details link for more information.	View details

#18

Posted by System Message on January 21, 2013 at 8:07pm

Status:

needs review

» needs work

The last submitted patch, field_attach_submit-1846648-17.patch, failed testing.

#19

Posted by swentel on January 21, 2013 at 8:10pm

Status:

needs work

» needs review

#17: field_attach_submit-1846648-17.patch queued for re-testing.

#20

Posted by swentel on January 22, 2013 at 8:50am

#17: field_attach_submit-1846648-17.patch queued for re-testing.

#21

Posted by Wim Leers on January 22, 2013 at 10:01am

Status:

needs review

» reviewed & tested by the community

Straightforward function renaming patch that prevents confusion about what the function actually does. Tests pass (and I know several of them are very stringent, so I trust them), code comments updated.

#22

Posted by effulgentsia on January 23, 2013 at 7:42pm

Title:	field_attach_submit() is not actually a submit handler in Drupal 8 and is called from within validation	» field_attach_submit() is called from within validation, causing developer confusion, so rename it
Category:	bug report	» task

RTBC+1. It's not a bug though, so recategorizing and retitling.

#23

Posted by effulgentsia on January 23, 2013 at 7:44pm

Title:

field_attach_submit() is called from within validation, causing developer confusion, so rename it

» Rename field_attach_submit(), because it is also called from within validation

Trying to learn to be less verbose :)

reviewed & tested by the community

» needs work

The last submitted patch, field_attach_submit-1846648-17.patch, failed testing.

#27

Posted by swentel on January 24, 2013 at 9:54pm

Status:

needs work

» reviewed & tested by the community

Re-rolled, should be fine.

Attachment	Size	Status	Test result	Operations
field_attach_submit-1846648-27.patch	15.58 KB	Idle	PASSED: [[SimpleTest]]: [MySQL] 49,287 pass(es).	View details

#28

Posted by catch on February 4, 2013 at 2:31pm

Title:	Rename field_attach_submit(), because it is also called from within validation	» Change notice: Rename field_attach_submit(), because it is also called from within validation
Priority:	normal	» critical
Status:	reviewed & tested by the community	» active

Committed/pushed to 8.x, thanks!

Will need a short change notice.

#29

Posted by yched on February 4, 2013 at 8:10pm

Priority:	critical	» normal
Status:	active	» fixed

Thanks !
Change notice created at http://drupal.org/node/1907724.

Change notice: Rename field_attach_submit(), because it is also called from within validation

» Rename field_attach_submit(), because it is also called from within validation

#33

Posted by System Message on February 18, 2013 at 10:10pm

Status:

fixed

» closed (fixed)

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

Project:	Drupal core
Version:	8.x-dev
Component:	field system
Category:	task
Priority:	normal
Assigned:	Unassigned
Status:	closed (fixed)

Rename field_attach_submit(), because it is also called from within validation

Jump to:

Issue Summary

Change records for this issue

Comments

#1

#2

#3

#4

#5

#6

#7

#8

#9

#10

#11

#12

#13

#14

#15

#16

#17

#18

#19

#20

#21

#22

#23

#24

#25

#26

#27

#28

#29

#30

#31

#32

#33