Entity forms cannot be properly extended

subscribe

subscribe :-)

We need to do both 1) and 2).

And as long as there is no new Form API facility that would allow us to bundle/group any entity submit/update callbacks to ensure at least some sanity in the order of #validate and #submit functions, we should continue the hook_ENTITY_submit() pattern. At least for D7.

Using form-level #submit handlers for updating the entity and only because of that moving the handler for saving to the button level could maybe be called unclean?

Yeah, I still find this somewhat suspect, and am concerned about rushing something like that into D7. Currently, we only do this for node forms, and that's cause that's how we did it in D6, but I'm not convinced yet this is a pattern we want to spread. Note, I'm not opposed to adding a form_execute_default_handlers() function per se, but I'm not so sure we want to make the updating of an entity be the form-level #submit. Please see Barry's comment and my reply about this issue of using a form-level #submit in this way. For D8, we can have time to think about things like #pre_submit or #default_submit, but changing the conceptual relationship between form-level #submit and button-level #submit for D7 doesn't sit quite right with me.

Therefore, I think my preference is hook_(comment|user|taxonomy_term)_(validate|submit)(), though I agree that this is unfortunate, because since multiple forms can be created for the same entity type, and even the main form can be altered, it means every implementation of such a hook will need to have an if statement that checks whatever assumption about the form it needs to check, as menu_node_submit() does.

I'm definitely still open to other ideas, if we think they're do-able for D7.

I'm adding the FAPI challenge tag to this, because maybe the right solution will be to add more FAPI properties to have finer control of validate|submit handler execution. Probably not, but maybe.

What about making #builder_function be an array?

Then forms that extend an entity form and add a custom property can add their builder function in their form_alter:

$form['#builder_functions'][] = 'my_custom_builder_function';

In my_custom_builder_function(), the module would set its properties in $form_state['node'] based on $form_state['values'].

In node_form_submit() and node_form_build_preview(), in place of
$node = $form['#builder_function']($form, $form_state);
we'd have

form_execute_builder_functions($form, $form_state);
$node = $form_state['node'];

and in form.inc

function form_execute_builder_functions($form, &$form_state) {
  foreach($form['#builder_functions'] as $builder_function) {
    $builder_function($form, $form_state);
  }
}

node_form_submit_build_node() can stay as is now, except it wouldn't return $node anymore.

That would be an API change, but we changed the API of #builder_function a couple of days ago anyway, and I don't think it's used a lot in contrib as of now.

I could go along with #6/#7 if someone wants to roll that patch. I think the basic premise here is that form-level #validate is already what we want. It's not often there's button-level #validate, and if there is, and if it wants to invoke the form-level ones before or after calling its own, that makes conceptual sense. I think what I'm stuck on is making the form-level #submit of entity forms incomplete by only being the thing that builds the entity, but not doing anything with it: other than node forms, all our forms have form-level #submit that does the complete primary action of the form (i.e., for entity forms, saving the entity), and then we use button-level #submit for all the "other" buttons, but not the "primary" one. And that's why we have a new property like #builder_function, or whatever we may want to change that to, for building the entity independent of what we then do with it.

While we're still in the brainstorming phase, here's another option to throw into the mix. Leave #builder_function alone (if we care at all about BC here) to be used by the module that defines the form, and add a new #entity_submit property for modules that implement hook_form_alter() to extend the form. See patch.

Good ideas. I'd vote for #entity_submit, using Frando's proposal of functions to invoke, i.e., #8 and #6 combined, merging #builder_function into #entity_submit.

Anyone available to push this along?

+++ includes/common.inc
@@ -6709,6 +6714,13 @@ function entity_form_submit_build_entity($entity_type, $entity, $form, &$form_st
+    foreach ($form['#builder'] as $function) {

I guess that works - but #builders absolutely needs to be plural.

Are all core entities invoking entity_form_submit_build_entity() already?

Powered by Dreditor.

Looks good to go for me.

Sorry, minor :

+++ includes/common.inc
@@ -6687,15 +6687,20 @@ function entity_form_field_validate($entity_type, $form, &$form_state) {
+ * not field data, and calling field_attach_submit() to copy field data. Also
+ * this helper invokes any additional, in $form['#builders'] specified builder
+ * functions.

typo / missing word ?

47 critical left. Go review some!

Thks, back to RTBC.

Changing to major as per tag.

Tag update

+++ includes/common.inc
@@ -6738,6 +6743,13 @@ function entity_form_submit_build_entity($entity_type, $entity, $form, &$form_st
+  // Invoke all specified builders for copying form values to entity properties.
+  if (isset($form['#builders'])) {

Coming back to this issue after a while, I'm still happy with this patch, but I first wondered

"ok, what's a 'builder' again?"

"...and, is that a Form API property, or to which API/module does it belong?"

If that other property wouldn't be called #builder_function already, then I'd really have to insist on proper namespacing, i.e., #entity_builders.

Powered by Dreditor.

I'm also confused by '#builder', especially because we already have $form['#builder_function']. Gotta say, something doesn't feel quite right.

AFAICS, #builder_function was only left for backwards compatibility. If @Dries in #27 was a "go, break those APIs!", then we can merge #builder_function + #builders into a new #entity_builders

Personally, I think we can still break APIs while we're in ALPHA. We don't support HEAD2HEAD database updates either, so I don't see why we would treat code entirely different. So, let's break the API if (and only if) that makes it easier to understand. Once we're in beta, this will obviously change.

+++ includes/common.inc
@@ -6729,16 +6729,20 @@ function entity_form_field_validate($entity_type, $form, &$form_state) {
+ * modules may specify additional builder functions in $form['#entity_builder']

@@ -6753,6 +6757,13 @@ function entity_form_submit_build_entity($entity_type, $entity, $form, &$form_st
+  // Invoke all specified builders for copying form values to entity properties.
+  if (isset($form['#entity_builder'])) {

The property should still be plural -- similar to #theme_wrappers

+++ modules/comment/comment.module
@@ -1979,7 +1978,7 @@ function comment_form($form, &$form_state, $comment) {
+  $comment = comment_form_submit_build_comment($form, $form_state);

+++ modules/node/node.pages.inc
@@ -305,7 +303,7 @@ function node_form_delete_submit($form, &$form_state) {
+  $node = node_form_submit_build_node($form, $form_state);

+++ modules/taxonomy/taxonomy.admin.inc
@@ -830,7 +829,7 @@ function taxonomy_form_term_submit($form, &$form_state) {
+  $term = taxonomy_form_term_submit_build_taxonomy_term($form, $form_state);

Shouldn't the function name pattern rather be

MODULE_form_submit_build_ENTITYTYPE()

?

At least, that would make sense to me, and is the case for all functions here, except taxonomy's, which repeats "term".

Powered by Dreditor.

Thanks!

#36: extensible-reroll.patch queued for re-testing.

Although badly needed, this is D8 material according to the rules (I had to learn today).

Per #29, this needs to be ruled in or out before we get to beta, adding tag.

Given we're past beta1, and about to be in beta2, can #36 be modified to still be in consideration for D7?

Also, another issue related to our lack of consistent and mature generic entity / entity-type-specific APIs: #949576: Missing hook_entity_view() and hook_entity_view_alter().

This needs committer feedback, so RTBC. Also, this patch was ready a long time ago, and the sanity it introduces/restores is badly needed.

+1

This patch is required for the http://drupal.org/project/title project (which turns entity labels/titles into fields, for many use-cases, but primarily to make them translatable).

Title module needs to act before field_attach_submit() is invoked -- which is exactly what this patch here does.

#36: extensible-reroll.patch queued for re-testing.

sun graciously spent some time with me in IRC on this patch.

So first off, I was concerned because Dries's comment back in #29 talked about breaking APIs pre-alpha. We're now way post-alpha, and even post-beta, really. But sun explained that the #builder_function property is a core-only construct and the only contributed modules that would be affected would be those actively overriding one of these core properties. For confirmation, I grepped the latest checkout of Drupal Commerce, which is the project I'm aware of making the fullest use of entities, and there's no #builder_function anywhere to be found. So I think this change is ok, BC-wise.

What I don't like about this patch is #entity_builders effectively creates an alternate hook_form_alter() for entities only. I still don't really understand why that's needed. But since Dries didn't raise this as a concern in his review, and since this is the last issue as part of #367006: [meta] Field attach API integration for entity forms is ungrokable to resolve in D7 and has buy-in from all the right people, I guess we'll go with it, and clean this whole mess up later in D8.

Committed to HEAD. Thanks!

Is the only API change in #38?

Please explain the impact of this change on existing calling code and let me know if it should be announced.

Thanks!