[meta] Field attach API integration for entity forms is ungrokable

Holy cow. Just tested by adding a multiple field to users, disabling JS, going to user/1/edit, clicking add more, nothing.

Better title, tagging.

I'm preparing this over at #118345: Revamp hook_user_form/_register/_validate/_submit/_insert/_update.

Maybe I'm getting field_attach_submit() wrong. If I do, then please replace "field_attach_submit" with "#builder_function" in the following.

1) field_attach_form() should add $form['#submit'][] = 'field_attach_submit'. Killing manual invocations of that function.

2) field_attach_form() should add $form['#object_type']. Therefore, passing $object_type to field_attach_submit().

3) field_attach_submit() can then be abstracted:

  $object_type = $form['#object_type'];
  $submit_callback = $object_type . '_submit';
  $object = $submit_callback($form_state['values']);

  field_attach_submit($object_type, $object, $form, $form_state);

  $form_state[$object_type] = (array)$object;
  $form_state['rebuild'] = TRUE;
  return $object;

Hah! I see now that field_attach_submit() is in that snippet I copied/merged from node_form_submit_build_node() and comment_form_submit_build_comment()... so I should have another look at the actual workflow.

Anyway, you get the point, hopefully ;)

ok, what I don't get is: Why does http://api.drupal.org/api/function/node_form_submit/7, resp. http://api.drupal.org/api/function/node_form_submit_build_node/7, needs to invoke form submit handlers manually?!

#6: Read http://drupal.org/update/modules/5/6#nodeapi-presave. e.g. upload module uses that. Thus the submit button has its own submit handler, then invoking form level submit handlers. Probably it would be cleaner to don't use #submit, but make #builder_function a concept "#entity_builder" or so taking more handlers.

>Actually, I'd say the proper place to abstract this is at 'entity API' level, not 'field API' level...
Agreed!

@general workflow:
I don't see why we have to merge in values at all. I think we should load the entity initially and then pass it around modifying the same entity, till saving it at the end. Thus when there is an entity in form_state, use it instead of the fresh one.

While that sounds like it makes sense to put the entity $form_state['storage'], that would automatically turn on form rebuilds. Putting the entity in $form['#entity'] instead would be more convenient as it gets cached, but doesn't turn on rebuilding.

Update: We could use $form['#. $entity_type ] - as we already have that in many places. ($form['#node'], $form['#user'], ..). So we could use that entity, update it with the form values and save it once the form workflow is finished.

Actually, I'd say the proper place to abstract this is at 'entity API' level, not 'field API' level...

Very true, and the attached patch does exactly that.

It abstracts form submitting into the EntityController classes and also adds a handy helper function to properly and easily enable multistep on entity forms. It sets rebuild to true and builds a proper entity out of the current form values which it then stores in $form_state['ENTITY_TYPE'] so that it can be used by the form callback when rebuilding. I implemented this for nodes, comments and taxonomy terms. Users should be easy to do. I also adopted node and comment preview to use it.

I also added extensive comments and a TODO about the two lines in which node.module calls the form submit handlers. Took me a while to figure it out, it is ugly and should be removed but not in this patch I think.

Also, submitFormValues now *always* returns an object for *all* entity types, so a bit less array vs object confusion. Inside the forms though the entities still differ, node keeps objects while comment converts to an array. This is also a simple followup issue then, because now $form_state['ENTITY_TYPE'] simply is always an object, as is the $node or $comment argument in the form callback. DX++.

I tested it extensively for node (previews and add more work fine with and without js), taxonomy term add more buttons work without JS but not with JS but that didn't work before either. Comment form incl. preview worked without fields, didn't test field add more there yet. So this is likely still CNW and I might not have much time to work on this. But I quite like my patch, and I really love how field API forces us to finally standardize some of our APIs..

+++ includes/entity.inc
@@ -291,4 +310,18 @@ class DrupalDefaultEntityController implements DrupalEntityControllerInterface {
+  public function submitFormValues($form, &$form_state) {
+    // Actually submit the form values.
+    $entity = $this->submit($form_state['values']);
+
+    field_attach_submit($this->entityType, $entity, $form, $form_state);
+
+    return $entity;
+  }
+
+  public function submit($values) {
+    $entity = (object) $values;
+    return $entity;
+  }

submitFormValues() and submit() somehow look "replaced" here...

submitFormValues() is actually what submit() should be.

And submit() looks more like a convertFormValuesToObject() or whatever to me.

I may be wrong.

But let's make sure we keep "submit" as the "submit" we all know...

+++ modules/comment/comment.module
@@ -2000,51 +2030,11 @@ function comment_form_validate($form, &$form_state) {
+function comment_submit_form_values($form, &$form_state) {
+  $node = entity_get_controller('comment')->submitFormValues($form, $form_state);
+  return $node;
 }
@@ -2052,7 +2042,7 @@ function comment_form_submit_build_comment($form, &$form_state) {
 function comment_form_submit($form, &$form_state) {
   $node = node_load($form_state['values']['nid']);
-  $comment = comment_form_submit_build_comment($form, $form_state);
+  $comment = comment_submit_form_values($form, $form_state);

We can and should directly invoke the entity controller here.

+++ modules/node/node.module
@@ -3073,4 +3057,38 @@ class NodeController extends DrupalDefaultEntityController {
+  public function submitFormValues($form, &$form_state) {
+    // Unset any button-level handlers, execute all the form-level submit
+    // functions to process the form values into an updated node.
+    // TODO: This is ugly to have here. To be able to remove these two lines,
+    // we have to standaridze the various entity types to all either use
+    // form-level or button-level submitting to actually save the entity.
+    // Currently, only node.module uses button level submitting to save the
+    // entity while form-level submit handlers are used by other modules to
+    // expand their values in $form_state. The latter could be moved to a
+    // to-be-created hook_ENTITY_TYPE_submit() that is invoked from here.
+    // Alternatively, all other entity types could adopt node.module's way and
+    // use button-level submit handlers to save the entity.
+    unset($form_state['submit_handlers']);
+    form_execute_handlers('submit', $form, $form_state);
+    return parent::submitFormValues($form, $form_state);
+  }

Form submit handlers should be used for entity saving. If I read right in the past days, then I think I saw plenty of code in form.inc that assumes that a form is only ever really submitted when the primary submit button/handler of a form is triggered.

This review is powered by Dreditor.

Wrong variable name?

+function comment_submit_form_values($form, &$form_state) {
+  $node = entity_get_controller('comment')->submitFormValues($form, $form_state);
+  return $node;
 }

What about using $form['#'. $entity_type ] as described in #9?
The workflow would look like:
1. Initialize $form['#'. $entity_type ]
2. On submit pass on $form['#'. $entity_type ] to the next step in $form_state['#'. $entity_type ]
3. On rebuild, initialize $form['#'. $entity_type ] with $form_state['#'. $entity_type ]
4. On final submit, save $form['#'. $entity_type ].
-> I implemented that in the patch from http://drupal.org/node/301071#comment-2091868

Thus we
- never loose any changes even if we have multiple totally different steps
- don't need to load the entity again on rebuild
- save the "data merging" step

@$term = (object) $form_state['values'];
Do we still need that? The field API does already extract it's form values properly, thus we would just have to pull out any non-fields on our own. That way unwanted stuff like form_ids and so on doesn't end up in the entity...

@Controller:
Conceptually I wonder if this belongs into the controller or should sit directly in the entity's class? Thus we could start creating a DrupalEntity base class and derive DrupalNode, DrupalComment and so on from it. I think it would be much more intuitive to just write $node->submit($form, $form_state);

Tagging absolutely critical clean-ups for D7. Do not touch this tag. Please either help with this or one of the other patches having this tag.

I might be talking nonsense as I've not had breakfast and only skimread, but what about using the same pattern as system_settings($form) ?

0) It's very unlikely that we find a totally revamped solution within the next hours. So we have to think to the point here.

1) #594650: Provide central $form_state['values'] clearance will also help this issue, at least for button elements and internal form API values (which normally should be everything to cater for).

2) True is, we currently store entities in the top-level form element, as in $form['#node']. The question is, whether we need to support multiple entities in the same form. IIRC, the current code doesn't allow for more, so that's basically out for D7.

2a) This is the current pattern, and we also use it for other renderable arrays besides forms now. Elsewhere, it's $elements['#block'] (a block object), for example.

3) If we can presume that there is only one entity per form, then we can add another property that gives us a pointer about the current entity being processed. That would be $form['#entity'] = 'node';

4) Given #entity and #{$entity}, it looks to me as if we could do this in a semi-abstracted way, by just adding a #submit = 'field_attach_submit' at the very beginning of the form builder function (where ideally also #entity etc. are defined).

5) It looks like field_attach_submit() could just be a little bit smarter and accept both an object or array, and just convert that into an object, if it's an array.

6) I have little ideas about what to do about the $form_state thingy. To start with -- if that is a requirement from the beginning, then we should introduce a drupal_get_entity_form(), which is a clone of drupal_get_form(), but additionally massages ... ugh, no, it's drupal_build_form() that needs massaging. Point is: We pass an initial object, build a form out of that, submit that form, and if we get back to the form, we want to use the last state of the object. That's what $form_state is for, and with aforementioned points, field_attach_submit() was able to update.... oh, now I get it!

7) #571086: Allow to specify a 'wrapper callback' before executing a form builder function (don't get confused by this issue's status) already introduced CTools' concept of a "wrapper_callback", and that's why all form builder functions take $form as first argument now. The idea is to replace "drupal_get_form" in hook_menu with "field_get_form" (or "entity_get_form"). That callback does the same as drupal_get_form(), but additionally assigns $form_state['wrapper_callback']. This wrapper callback is invoked before the actual form builder is invoked and (with that patch being committed), it gets the same arguments as the actual form builder. Hence, we get the initial $node object and are able to put that into $form_state. We can also auto-add the #submit handler. If the form is rebuilt, then we come back again, identify that we already have a populated $form_state and use that instead of the passed in $node.

The more I think about it, this actually means that Field API has the same requirements as CTools/Panels/Views' object caching.

Funky stuff! :)

And, yeah, I also have a ready patch in #367567: Use AJAX framework for "Add more" / load include file containing form / introduce $form_state['build_info'] to introduce a $form_state['build_info'] that is cached separately from $form_state['storage'], which was one of the key problems mentioned earlier in this issue, and nicely goes along with the solution I outlined in 7).

subscribe

So the following two patches are what hold up a sane solution for this issue:

#367567: Use AJAX framework for "Add more" / load include file containing form / introduce $form_state['build_info']
#571086: Allow to specify a 'wrapper callback' before executing a form builder function

With those in place, we are able to properly leverage the 'wrapper_callback' and 'build_info' facilities, which allow us to prepare and store non-volatile entity + field information in $form_state and cache that info accordingly with the form.

The major problem of the current flow is that any custom #validate or #submit handler has absolutely no idea of whether to work on $form_state['node'], $form['#node'], or the submitted form values. It entirely depends on the function invocation order, and that is a horrible problem.

Two very closely connected issues need to be resolved here:

1) Kill #builder_function.

2) Make entity/field caching properly work without JS.

I'm not sold on the idea of extending the entity controller. This clearly boils down to having a properly workflow in Form API: We want reliable data in $form_state, nothing more and nothing less.

Subscribe. I'm still trying to wrap my head around this issue and a related one: #622922: User input keeping during multistep forms. This is purely a naive gut feeling on my part, I'm by no means a FAPI guru, but it seems to me like we're fighting against an architectural limitation here. Seems like FAPI is designed with the idea that multi-step forms have form constructor functions that know it's a multi-step form and use $form_state['storage'] to build $form, and submit handlers that also know it's a multi-step form and set $form_state['storage'] as needed. But with AJAX enabled forms in general, which is what we're trying to figure out on that issue, and "add more" field items forms specifically, both js and non-js, we're trying to use drupal_rebuild_form(), and therefore all the assumptions that go with multi-step forms, in a context where neither the form constructor function nor the submit handlers know that it's a multi-step form.

I think it would be awesome if we could solve this at the FAPI level, by making a distinction between multi-step wizard-style forms (where the form constructor and submit handlers know that they are part of a multi-step form and use $form_state['storage']), and forms that have components that can be updated a few interim times before the "real" submit. And either come up with an alternate drupal_rebuild_form() function more appropriate to the latter case, or make the existing drupal_rebuild_form() function handle both types of multi-step forms.

Maybe I'm misreading the above discussion in this issue, but it seems like it's focusing on creating wrapper functions to get around what is undesirable in drupal_rebuild_form() rather than trying to fix or create an alternate drupal_rebuild_form(). Maybe there's a really good reason for that, but it makes it hard for me, and I imagine other non-FAPI-gurus, to make any sense of it.

It is possible that #622922: User input keeping during multistep forms will help to fix the original issue with "Add more" not working without JS on the user account edit page.

Here, however, we are trying to fully understand the whole Form API and Field API integration as well as the data and storage flow that's involved, which is a big mess currently.

Since the blockers to streamline this workflow have landed in the meantime, I'll try to wrap up the streamlined solution I have in mind on this weekend.

I just cleaned up the form workflow for the profile2 module. See http://github.com/fago/profile/commits/master.

It's now using the form storage:

* If the storage isn't set, populate it with the initial entity (loaded or created from scratch)
* Populate the form set the and initialize the form values with the entity from storage
* Once validated submit the values by updating the entity in the form storage.
* If the form is rebuilt, everything works fine as the udpated entity from the storage is used.
* On the final submit, save the updated entity.

That way we always have one single copy of the entity.

Compared to my proposed solution in #9 we even save copying it around between $form and $form_state, but we introduce the problem with form storage automatically turning on rebuilds. While it's usually handy, it doesn't make much sense that you cannot avoid that. Even redirects get ignored. I think we should address that and allow redirects even when the storage is set, then everything is fine.

For fixing redirects when using form storage see #634440: Remove auto-rebuilding magic for $form_state['storage'].

>I think that's the reason $form_state['build_info'] was introduced ?

Yes, but still this is "build_info". Thus using it for our data is imho 'misuse' - instead we should fix 'storage' so we can use it as it should be.

There is no real guideline for 'build_info' and 'storage'. I mean, yes, it was designed to store internal form builder information, but at the end of the day, everyone is free to use either one, and I would say that all data that does not map 100% to the actual form implementation could and perhaps should be stored in 'build_info'.

For example, private data for multi-step form logic in the form implementation belongs into 'storage', because that's your private form storage bin. So if your multi-step form logic wants to store the amount of foobars for the next step, that goes there.

OTOH, all data that is required to build or rebuild a form belongs into 'build_info'. For example, the $node object with the form context or Field API's 'fields' reference for attached fields in the form.

Thus, we want to use 'build_info' for this challenge. I agree, however, that we also want to streamline 'storage' and form caching, but these are separate issues:

#583730: How many ways are there to cache a form?
#634440: Remove auto-rebuilding magic for $form_state['storage']

I will look into your implementation in profile.git ASAP.

>OTOH, all data that is required to build or rebuild a form belongs into 'build_info'. For example, the $node object with the form context or Field API's 'fields' reference for attached fields in the form.

I think you are mixing up "form construction" and "form building". I know the terminology was already unclear, but we need to distinct that. Let's use "form construction" when the users's functions constructs $form and "form building" when the form API is dealing with it (->drupal_build_form). I just read through some docs, they are now following this terminology too.

Thus 'build info' should be form API info regarding it's build process. -> The $node object doesn't belong there.
Also nobody says 'storage' is private, everyone is free to use. I'd even say it's the other way round, 'build info' belongs the form API, 'storage' is free to use by anybody (constructors or alter implementations).

So I think having storage carrying any user-data is a good distinction to build-info. Yes 'storage' needs some more fixes to be really useful - but if we stop using it we don't need to fix it any more ;)

related bug: #634984: Registration form breaks with add-more buttons

I fear we're not getting anywhere. Let's do a general discussion in #635552: [meta issue] Major Form API/Field API problems and go back to individual issues afterwards. Thanks.

.

And here's the killing concept.

As explained in #18, including example conversion for node form.

A couple of tests will fail, because quite some code relies on $form['#node']. However, manually adding, editing, and previewing nodes works.

I quite like #35. It definitely goes into the right direction. I would propose to move submission (i.e. node_submit) and the creation of empty stub entities into the entity controllers (this would be an API addition, but no change). Also, several node related modules currently form_alter the node form and add custom #submit callbacks on the form (e.g. menu.module). This is a problem because those submit callbacks do not run if a button with a button-level submit callback is clicked (e.g. add more). So these parts just have to move to the new hook_ENTITY_TYPE_submit(). This is much cleaner then.

Here's an initial review of the first part of the patch, ran out of time right now but will go on later hopefully.

+++ includes/form.inc	29 Nov 2009 03:42:39 -0000
@@ -78,6 +78,170 @@ function drupal_get_form($form_id) {
+ * @param $obj_type
+ *   The type of $object; e.g. 'node' or 'user'.
+ * @param $form_id
+ *   The unique string identifying the desired form. See drupal_get_form() and
+ *   drupal_build_form() for details.
+ * @param $object
+ *   The object for which to build a form.

I suggest $entity_type for $obj_type and $entity for $object. I think that's what was generally agreed upon with regard to the entity API.

+++ includes/form.inc	29 Nov 2009 03:42:39 -0000
@@ -78,6 +78,170 @@ function drupal_get_form($form_id) {
+    $form_state['storage']['entity'] = $obj_type;

Same here, this is confusing. Let's do $form_state['storage']['entity_type'].

+++ includes/form.inc	29 Nov 2009 03:42:39 -0000
@@ -78,6 +78,170 @@ function drupal_get_form($form_id) {
+    // @todo Can we build an empty object for entities? Or does this always
+    //   need to be prepared by a custom page callback?
+    //   @see node_add()
+    if (!isset($object)) {
+      $object = new stdClass;
+    }

No we cannot do this yet generally yet but we ought to. Proper way would be to add a createEmptyEntity() method to the entity controller.

+++ includes/form.inc	29 Nov 2009 03:42:39 -0000
@@ -78,6 +78,170 @@ function drupal_get_form($form_id) {
+    $form_state['storage']['object'] = &$object;

Again, I'd replace 'object' with 'entity'

I'm on crack. Are you, too?

+  $form_state['storage']['object'] = (object) $form_state['values'];

This seems wrong to me. Why do we have to recreate a fresh object? That way we quickly end up with a broken object, either some form values are set to #access FALSE so their values are missing, or someone hooks into the form and adds something which doesn't belong into the object. Why not just extract the things we really need and put it into the object?

A lot of bugs stem from the fact that the $node after submitting a form looks different - just have a look at the token code for dealing with terms if you need an example ;) So let's learn of that and make sure we always have a full and clean object that never looks different in any stage of the workflow.

For the profile2 module I just did:

  foreach ($form_state['storage']['profiles'] as $name => $profile) {
    field_attach_submit('profile', $profile, $form, $form_state);
  }

The field API already extracts the values for us - so nothing more todo here. For the node form or others we just have to make sure any additional things in the form that are not fields are also extracted and are added to the object.

Also the above code assumes the entity is just a stdClass() object, which isn't true in general. E.g. entities of the entity CRUD API I implemented, aren't. See http://drupal.org/project/entity

Some good suggestions, fago, thanks!

However, we really want to use $form_state['storage']['object'] as stateful storage of the object that is being edited. And that's not limited to field_attach_submit(). As you can see in the patch, I changed node_form_submit() to also work from the latest $node object that is contained in the form storage (and save that).

The issue you're raising regarding #access and invisible properties/fields makes totally sense. So the only way to account for this would IMHO be to walk (perhaps even recurse) over the submitted form values and granularly update $form_state['storage']['object'] with it.

Would that make sense?

Yep, I was talking about updating the object like that.

The question is if we want to automatically pull everything of the form values into it or not. There might be data in it that doesn't belong in the object anyway, so perhaps it would be cleaner to just define an array of fields to copy over. Of course then modules would have to do that too in case they want to add something to the form that has to go into $node. But this would ensure that $node is always clean and doesn't look somehow different because of form values looking different than the structure loaded from db.

As of now, this is what we currently do in such forms anyway. Form element value callbacks and element validation handlers ensure that submitted form values in $form_state['values'] are in the structure we do expect for the object. In addition to that, some elements may even use hard-coded #parents to ensure value proper placement on submission. Hence, plenty of ways to do this. Not sure, when exactly this started, but I already know this from Drupal 5.

I don't think we need another facility to do this. The current code already is already prepared for that.

One big question with walking/recursing over submitted form values is, however, the granularity in which we apply the values to the object. Or in other words: How do we know from which key we need to update $object in the following?

$form_state['values']['field_foo']['en'][0]['value']
$form_state['values']['menu']['link_title']

Sorry, I forgot. The current code for Field API and other things does stuff like #629252: field_attach_form() should make available all field translations on submit, i.e. ALL values that should end up in the object that's derived from the form values are contained invisibly in the $form as #type => value.

So, erm... not sure whether we need this granularity at all. The current practice is to define such properties as #type => hidden/value in the form anyway, so the casted $object based on $form_state['values'] really contains everything it should.

@granularity:
Perhaps it would be best if we recursively merge the values - so we ensure nothing is getting lost?

@#type => value
That is something really unnecessary. The values are already in the object in form storage, so why pass them around another time?

fago and me discussed in IRC.

1. The issue with form values for #access => FALSE elements was a mistake and does not exist.
2. All of the current forms already use #type => 'value|hidden' to ensure that certain/internal object properties are contained in the form values and this is how it works for a long time already. It would definitely be worth to explore whether we can find a better process for not having all object properties invisible in a form, but that's definitely D8 material.

   What's important for now is that we can safely cast $form_state['values'] into an $object, because that is exactly what the current code in HEAD does everywhere. So this is nothing new and not invented here.

3. Of course, this code doesn't really belong into form.inc. However, entity.inc is not loaded by default in full bootstrap. And the menu system does not support calling class methods via 'page callback' => array('entity', 'form')yet.

   Hence, to get this done, we will simply load entity.inc in full bootstrap.

   Step 2 is to make the menu system support calling class methods for router item callbacks (separate issue).
   Optional step 3 is to convert entity_get_form() into DrupalDefaultEntityController::form(), but that may as well be D8 material.

@yched:

1. Interesting, I didn't know about http://api.drupal.org/api/function/entity_create_stub_entity/7 - however, when looking at http://api.drupal.org/api/function/node_add/7, I'm not sure whether we can inline all of that? Perhaps we can. Not sure. Will Field API have problems with that? I guess not, because those properties aren't remotely related to fields. Likewise, all existing ENTITY_save() functions should already be smart enough to only store object properties they care about, so it possibly doesn't matter whether there is an additional, unused $object->uid or $object->language property.
2. $form_state['storage']['object'] always contains the latest state of the $object and is identical to $object, because $object is extracted by reference. The special form submit handler entity_form_submit() is invoked first, so any further submit handlers, including the main entity submit handler that will save the entity can safely base their work on it.
3. I don't think we need the original $object anywhere - at least I'm not sure why we ought to need it. The entire concept roughly follows the idea of CTools/Views/Panels' object caching, which means:
   1. We build a "form representation" for an object and cache it.
   2. We update this object in cache all over again.
   3. On final submit, we save the object from cache.
4. Your suggestion about $form_state['rebuild'] perhaps makes sense. I was equally unsure about it, which is why I didn't add an inline comment for that line. (should have added a @todo though ;)

Incorporated some of the suggestions.

>Bottomline is: with TF, no 'node' built solely from form submitted value will ever be able to match a $node loaded from the db, because of all the field values for other languages, that are missing in the form.

Yep and as we have already the values in the storage object, why should we bother with putting them in the form values?
So as of know most is still in the form_values, but we have already the translations being not in there. I still think it's time to clean that up now and don't go with both ways being partly used.. The question is whether we should
a) automatically pull everything out of the form and update the object using a recursive function, or
b) change it only pull a list of fixed element names into the object and let modules extract their own data

b) changes probably to most in the current workflow, so it's no real option to go. Still a possible problem is when a module wants just create something related to the currently edited object, then still it's data would go into $object what isn't ideal. Ideally we would should only $form_state['values']['node'], but this change wouldn't be trivial.

* Probably we have to rename the existing $form_state['storage']['object'] to something like $form_state['storage']['source_object']

@object reference:
We are coding for PHP5 now, so we don't need to set the reference.

* field_attach_form_validate() reads the form values out of $form_state itself, so we can just pass the source object from storage to it.

* @stdClass: It would be easy to enhance entity_create_stub_api() to make use of an optional 'entity class' entity-info property. That's all we need to do to don't limit it to stdClass when we go with the updating values approach.

errr... so the latest patch in #629252: field_attach_form() should make available all field translations on submit removes the code that puts the field values for inaccessible fields/translations and stuff into #type => value elements. Hence, it looks like we could still go with the proposed approach here, if we would not do that change.

#629252: field_attach_form() should make available all field translations on submit does require the source object, and won't work with the 'current state of the edited $object' if I'm not mistaken. Bottomline is: with TF, no 'node' built solely from form submitted value will ever be able to match a $node loaded from the db, because of all the field values for other languages, that are missing in the form.

Exactly. In the latest version we automatically pull out field values: we use the submitted field values if present, otherwise we use the original ones. This way the new $object is a merged version between a simple cast of $form_state['values'] and the original data structure. The advantage of this approach is we don't get unnecessary/unwanted form submitted values in $object.

the latest patch in #629252: field_attach_form() should make available all field translations on submit removes the code that puts the field values for inaccessible fields/translations and stuff into #type => value elements. Hence, it looks like we could still go with the proposed approach here, if we would not do that change.

I think both approaches could work in #629252: field_attach_form() should make available all field translations on submit. Probably the reason we went for the automatic handling is Field Form API already took this way. I think it would easy to come up with a new patch using 'value' form elements instead of using $form_state['storage']['object'].

So what about this one:
* I took sun's patch and modified it to don't cast the form values to an object, but use the form values to update $object in a recursive way. So values not touched by the form will stay, eg. the other translations. Thus with that approach there should be no special fix necessary in #629252.
* I also fixed it be able to work with classes.

When updating the code I noted a problem. Field API extracts the values during build and as we have PHP5 the object in storage gets updated. Thus extracting the values again in 'submit' is a duplication. Furthermore I think the cache will be written although we have validation errors, thus the changes persist. Thus the object used in the form constructor is suddenly not properly validated any more!

Is there a need to update the cache when validation fails? I tend to think we ran into another FAPI issue.
update: Well it might make sense to not cache storage when validation fails, but probably it's simpler staying without that magic.

Instead we could clone the object for validation and update the object in storage only in the submit handler.

+++ includes/common.inc
@@ -3475,7 +3475,7 @@ function drupal_load_stylesheet_content($contents, $optimize = FALSE) {
-      }x', '\1\\\\\2', $contents);    
+      }x', '\1\\\\\2', $contents);

Please fix your IDE.

+++ includes/entity.inc
@@ -298,3 +298,195 @@ class DrupalDefaultEntityController implements DrupalEntityControllerInterface {
+  // Extract values.
+  foreach ($form_state['values'] as $key => $value) {
+    if (isset($object->$key) && is_array($object->$key) && is_array($value)) {
+      _entity_set_value_rec($object->$key, $value);
+    }
+    else {
+      $object->$key = $value;
+    }
+  }

The problem with this approach is that I think we also need to account for removed values, especially when considering multiple value fields.

I'm on crack. Are you, too?

>Please fix your IDE.
:D The line needs to be fixed ;)
I'll turn it off.

>The problem with this approach is that I think we also need to account for removed values, especially when considering multiple value fields.
Indeed, so the recursive approach is a no-go. So let's do it like previously and update the whole value if it is in the form.

errrr, again.

There is absolutely no way to get an entire revamp of form value => object => form value handling into D7.

I don't think there is a solid middle ground between total array-object casting and selective object property updating (requiring some yet unknown facility). I already tried to explain with trivial form value snippets above why I think it's not possible to do anything in between - because we simply miss the required information for entirely removed values and more complex form value data structures.

The selective object property updating seems out of the loop for me. Which means we need to go with the already existing array-object casting.

Hence. Yes. The approach taken to solve this issue could be seen as manifestation of the "$form structure needs to map to $object structure" design, having the slight difference that we always work from a stateful $object to base a $form representation on it. But that's what we have since D6, and I don't see how we could squeeze in a major API change that does not follow this paradigm into D7 right now.

In addition to that, I'm not even sure whether it's a wrong paradigm. Ideally, merlinofchaos should chime in here, but I fear he's far away from core HEAD currently. IIRC, he once mentioned the concept and idea of forms being just a representation of an object. Just like node_view() is the rendered view of an object, node_edit() should be the form representation of an object. This, at least, allows for some clever and bad-ass smart workflows when working with the form representation.

I fear we need to get our ideas, considerations, and goals in line before re-rolling the patch any further. But, please, let's do that quickly.

Ok, so let's resemble the previous way of $node = $form_state['node'] + (array)$node; by doing a simple

  // Extract values.
  foreach ($form_state['values'] as $key => $value) {
    $object->$key = $value;
  }

That way this works for non-stdClass objects too. Re-rolled patch with that.

Better abstracting out the 'extract values' part sounds a good thing to do. It would make sense to first extract all values and then start with the validation, so each stage has all extracted values. However for that we'd need to refactor field API to have an extra 'extract values' attacher which is currently built into validate+submit. hook_entity_extract_values?

Also as field API already can pull the values intelligently out of the form it would be nice to have the field API doing it that way. We could copy the form-values into a temporary data array, let field API extract it's values + unset the extracted values. Afterwards the remaining values are set in the object like above.

@field_attach_form_validate
>Additionally - we explicitly do not want field_attach_form_validate() to run on those 'values present in the source object but not in the form'.

Agreed, the validation handler should of course get the updated object. But when only looking at field API the comment of the function states it pulls out the values of the form nevertheless, and it does. I think this is dangerous as it's not documented and could lead to not validated values going into the object, like it does with the current patch. Don't forget PHP5 passes objects always reference-like.

and the patch..

@yched: That's because http://api.drupal.org/api/function/node_add/7 is a fairly special function. When I wrote that part, my presumption was that not every entity may be a nightmare like nodes. Of course, to simplify this patch and what we do for D7, we could simply require entity add forms to implement a custom menu page callback to prepare an empty object.

Well, starting to use entity_create_stub_entity() probably makes sense and might help us saving the extra add forms. Anyway I didn't get the comment. We could do it by just alter the args in $form_state which is passed to the constructor by reference. But why do we have to update $args?

@#63:
Thanks, I understand the problem now. Is validation skipped when the values aren't there automatically? What if required values aren't edited currently? Having a half-baked object doesn't look that good to me. For proper entity_validation() we would still need a full object anyway, so one can validation dependent on another value.

So what about changing 'field_default_form_errors' to only mark errors represent in the current form? So we could go with a full object and the user has the possibility to fix it. I think that's the best option we have when have suddenly not more valid data in the db.

Regarding the last follow-ups on partial form validation: Please take into account #370537: Allow suppress of form errors (hack to make "more" buttons work properly) in your considerations, which seems to be RTBC by now (and quite awesome on that note).

Forces modules to define functions outside of their namespace if 'module name' != 'entity name', e.g field_test.module has test_entity_submit()

This is an absolute show-blocker. Not sure how to resolve this other than requiring to pass the module name as first argument to entity_get_form().

Why not specify the module in hook_entity_info() or gather the information automatically? It's for sure useful in more cases to know which module defines the entity.

@validation: Ok, then let's pass the half-baked thing to the field API, however I think this should be documented in the validation related field-API functions. I fear modules passing around this half-baked object to API functions triggering obscure bugs. As field_attach_form_validate() reads out the form values itself we could even pass a new stub entity to it.

>>- "For proper entity_validation() we would still need a full object anyway, so one can validation dependent on another value."
>Use cases are unclear to me. That seems like custom code in hook_node_validate().

E.g. validate a field dependent on the node author.

>And if done on the entire $object it would carry the same danger of reporting errors the current user is not guilty of and can not fix.
We don't have a distinction of doing form validation and validating the object yet - except for fieldAPI. However hook_node_validate() is already tied to the form, which is good as having the form enables one to only validate what's in the actual form. But also passing $form_state as this patch does is important.

@#75
Agreed, that makes sense. I'd prefer much the raw form values approach to make clear this is no entity-level validation, however no way to change this now for d7. But also we need clearly documented the this object is not fully baken. It might have not more than a single field in it!

update: Not directly, but as a side effect it should probably fix it for that case. Currently is probably only broken for ajax requests because the bypass entity_get_form, but once the wrapper_callback property is cached it should work in that case too.

Subscribing. I'm trying to develop an entity module right now and running head first into FAPI WTF land. There's stuff going on here that I can't even begin to comprehend, mostly the workflow of the $form['#builder_function'] stuff.

I'll try to have another stab at this, if no one beats me to it. Core maintainers know about this issue, but we have a hard deadline now - this time for real.

Discussed this patch with frando in IRC last night... #370537: Allow suppress of form errors (hack to make "more" buttons work properly) basically made this entire approach impossible, because there can be unvalidated values in the submitted form values, which we must not merge into the entity. That's insane.

As far as I can see, the only way out would be to make field validation not based on the entire form values/object, but on a per-field level, so only validated form values are taken into account for field validation and submission.

So you think pulling the unvalidated values into the object, which is used to rebuild the form is dangerous? Well I think if that feature is used on the node form as it is now, it would already work that way (or just forget the changed values). But I can see that this might become dangerous as the form would be built with an object containing unvalidated values and all code using that object would have to respect that - but that becomes impossible once the object is passed to any API functions.

So it would be better when the object wouldn't change at all in the case of a button bypassing validation. We could just use the old object for form rebuilding again + the new form API feature of keeping the input values during a form rebuild. That should work, shouldn't it?

Also we may not forget what yched pointed out in #74:

What I'm pointing is that the patch as it currently stands brings a major change in the validation workflow of core entities: entity validation (and related hook_{entity}_validate for 3rd party checks) runs on the fully stored object instead of only on actual form values. This brings a chance of blocking users on validation errors they cannot fix since they lie in the object as it currently exists. This is not only true for the specific case of field validation.

IMO all (form) validation should run on a stub object built only with the forms values. Or perhaps even just on raw form values without building a stub object. Entity validation is not untied from form submission, then lets draw the consequence.
Then field validation is a little specific because it happens that the _field_invoke() iterator mechanism and the structure of the various hook_field_[op]() require an $object (even an incomplete one) holding field values. But that's strictly the business of field validation. The $object can be built specifically for Field API validation purposes and ditched afterwards.

Subscribing because FAPI WTF is fun... sometimes :)

Well currently the node form as well as the field API itself disables this feature from FAPI - have you enabled it for your tests?

For that we have to remove

  // This form has its own multistep persistance.
  if ($form_state['rebuild']) {
    $form_state['input'] = array();
  }

from field_multiple_value_form(). Doing would also be the best way to fix #634984: Registration form breaks with add-more buttons - as it would fix that problem for any form. Also this feature is built to serve exactly that cases so I really think we should try to make use of it. However I fear the we might have troubles with sorting multiple values once activated.

.

It seems we have to live with "what no one understands" until D8.

No.

Bugs in D7 are not fixable until this is at the very least explained in documentation.

Eg #641314: Taxonomy term form being rebuilt even after final submit.

The problem is we cannot actually fix this without breaking major APIs. I thought about this for quite some while and have several early half-baked patches lying around here that tried to solve this. But the thing is the way the interaction between FAPI validation and submit handling on the one hand and entity submission and saving on the other is just not made for being generalized. It is of course possible and it will definitely have to be done for D8, hopefully quite at the beginning of the cycle together with some other needed FAPI/Field API/Entity API generalizations and cleanups.

For D7, I fear there is no actual clean approach, and certainly not without not breaking at least some APIs.

Anyway:

Here's a summary of the issue

(Note: I wanted to write this down for quite some while now, so it got rather detailed. This can likely serve as a starting point to document the #builder_function part of the field attach API, should it stay in D7).

Entity submission prior to Field API worked like this (based on node module):

• - Form was submitted, form validation handler is invoked, takes form values, converts them into a stdClass and passes them to node_validate(), together with the $form array (for form error flagging). node_validate() calls hook_node_validate(), again passing the form-values-as-stdClass and $form around.
• - If no validation errors occur, the form submit handler is invoked. This calls node_submit() which, again, simply converts $form_state['values'] into an stdClass and then calls node_save() on it.

  This means that all validation handling happens purely in FAPI. On multistep forms, the form is rebuilt with the form-values-as-std-class in place of the original $node. On each multistep submit, the form values are again converted into a stdClass and completely replace the previous one. This is why it's e.g. so awefully difficult to create a node form as wizard module (because each time the form is submitted, the form values are validated as if they were a whole node. This means that on each submit, all values of the node have to be in $_POST or somehow inserted into the form values using the uglier parts of FAPI).

Then, Field API came. Field API has an advanced submission design, as it can valdiate on actual entities, and not on form values, and even has a seperate step to then flag form errors based on validation errors. This is how things are meant to be handled, and this is what we have to do for entities in D8.

Now, the basic problem is that, as explained above, there is no entity available for validation, just a form-values-as-stdclass with some entity-specific alterations, which is created by the entity's form validation handler and then again by the form's form level submit function.

So what Field API did is it made entity forms add a custom #builder_function property to their forms. In that callback, e.g. node_form_submit_build_node(), the entity module is expected to construct an entity object out of the form values. Field API calls that callback in their multistep submit handler (the "add more" button's button-level submit handler) to receive an entity object to pass on to the field validation handlers. The #builder_function can be re-used by the entity's own submit or validation handler, however, this is not made very clear nor is it applied in any automatic way.

The problem is that all this is not properly reconciled with the original entity forms.

The proper solution to all this will likely look like this:

1. All entity forms set their form level submit handler to something like entity_submit() and their validation handler to entity_validate().
2. FAPI gets a new, general, public property called something like #builder_function or whatever. This callback is called *prior* to validation. For an entity form, this would be set to entity_build_submit(). That function would invoke the entity's submit() handler, which will likely sit in the EntityController class, passing on the current form values and the previous state of the entity (stored in $form_state['entity']). The entity build submit handler can then look if it's interested in the form values and if so set them accordingly on the entity object. It also calls hook_entity_submit().
   (Sidenote: I don't like the name "build submit" too much. I did not come up with a better one, though. Alternatives might be build_from_form_values, construct_from_form_values, build_object, dunno).
3. Then, this object is passed on to validation, together with the current form values. entity_validate will call the entity's validate handler (so we'll validate the entity, not the form values). If there are exceptions, the form_error handler will be called (so we take Field APIs approach and use it for the whole entity).
4. If there are validation errors, they are displayed or hidden based on the #limit_validation_errors property for button level submit handlers. So this part will happen on FAPI level again, which is fine.
5. If there are no validation errors displayed, the submit handler is called.
6. If it is a button level submit handler in a multistep scenario, it can do whatever it want (apart from saving the entity). The form is rebuilt with the entity as constructed prior to validation, the button level submit handler has to do nothing special (so e.g. the field API add more handler does not have to do anyhting with a #builder_function or whatever). The current entity object is stored in $form_state and cached. On the next button click, the submitted form values are copied over to the entity object. Therefore, a wizard-like node form will be easy. A stripped down node form containing only one field will be easy as well, because we do not have to receive all values in $_POST as we do now. Yes, this will bring as a lot closer to edit in-place.
7. Finally, the form level submit handler will be called, which is entity_submit(). It doesn't support #limit_validation_errors, so it means that validation must have passed completely. The entity constructed in entity_build_submit prior to the last validation is still stored in $form_state and can then be saved without further processing.

So the main change will be that the entity object that's gonna be saved will be constructed before validation happens, so that validation can act upon the as-to-be-saved entity and not just on form values. This, however, is probably not doable in D7 because it involves several API breaks:

1. Contrib modules that currently do things in a submit handler that they form_altered onto the node form will have to do their stuff in hook_node_validate() instead
2. All entity forms will have to set their validation and submit handler to entity_* functions.
3. The entity module's submit and validation processing will have to move to place where it's findable by the entity functions - either strong namespacing, or (better and easier) methods in the EntityController.
4. And, the most difficult API change: Instead of converting the form values into an stdClass, the submit handler has to iterate over the form values, take the parts it's interested in and set them on the entity object which is stored in $form_state['entity'].
5. Also, currently we still change values sometimes in the validation handlers. This can easily lead to problems here I think and should happen in #value_callback.

Also, all this is quite some work, because we will have to update node, user, comment and taxonomy modules to this scheme. Comment.module's form handling for example looks in parts quite ancient..

I thought about doing this partially and doing a part still in D7, but I never found a solution that would be notably cleaner than what we have now without getting back to doing it more or less as outlined above.

What we can still do in D7 is cleaning up the individual entity forms, generalizing on common principles.

Maybe we can also still do API change A) as outlined above. This would kill some ugliness in node module, where the #builder_function currently calls the node form's entity level submit handlers (to e.g. make menu.module work in multistep scenarios). However, it doesn't make a lot of sense without generalizing it to the other entities and basically introducing a hook_entity_validate() and hook_enitity_submit().

Easier ones to still tackle for D7: We can for example always store the entity object in $form_state['entity'] after it has been constructed by the #builder_function.
And we can make it obligatory for all button level submit handlers on an entity form to call the form's #builder_function callback. When rebuilding the form, we just check in the main form callback whether $form_state['entity'] is present and if so use that instead of the entity passed as an argument.

This is basically how node.module does it already. Doing that for all other core entities would definitely make things easier for contrib (e.g. to form_alter a core entity form into a multistep form).

thanks frando for the detailed write-up - makes totally sense.

However I don't think a hook_enitity_submit() is the way to go, as extracting form values into the entity depends on the form. There might multiple, different forms that edit the entity, thus a single hook isn't suitable. Instead it probably would make more sense for modules to register their callbacks for building the entity just like they are able to register validation and submit handlers.

As of now http://api.drupal.org/api/function/hook_node_validate/7 is imho kind of broken as the node object is not properly built by node_form_submit_build_node() yet. So I really think we should fix it - which means we have to do that API change nevertheless.

One thing not considered in #91 is the problem of marking validation failures that can't be fixed by the user, see #74 & #75. This is a problem with entity level validation and a possible way out would be to just not do it as suggested in #75. However that way it's hard for a module to do entity validation as it cannot know all entity forms beforehand, thus a hook is necessary.

So what about something like hook_node_validate() is now, but
* pass in the previous node object, which has already passed validation but doesn't contain the current changes
* pass $form and $form_state
-> That way modules doing validation have to make use of the form values for validation and so they can only do so if the current form really changed the value to be validated. Also that way we can avoid half-baken objects. It looks to me as even the field API doesn't build half-baken objects for validation any more.

When we do so also the necessity of a #builder_function is gone and we can stick with the usual #validation for validation and #submit for extracting values into the entity, thus we can call hook_entity_submit() just on #submit which is likely that what developers would expect to happen.

I think the only way we can move forward, alas, is to tackle this in 8.x and write some docs for 7.x to make groking the state of things easier. Also writing multistep node forms is trivial in D7, if ppl think it's hard then that's a doc that needs to be written too.

>Also writing multistep node forms is trivial in D7

Well writing your *own* multistep form isn't hard, yep. But making the node_form() in multiple steps is as it requires you to have all form values there, else values not in there won't be in $node and are lost once the node submit handler calls node_save() on that.

Anyway that's not hard to fix: Just make $form_state['node'] a full node object that gets the form values changes merged in. So at least a little cleanup of the workflow makes still sense for sure.

Apart from that I'd still argue that node_validate() is kind of broken as it passes around a node object that's just an array to object conversion of the form values - thus we have a not fully built object being passed around which might lead weird bugs as we had it already in D5 & D6 - e.g. think of token trying to support multiple, different looking node objects which stem from different form-stages. That's just bad API design I'd prefer to be fixed for d7 instead of being documented ;)

@limit-validation-errors problem noted by sun in #83
-> It's in poll.module: #735800: node form triggers form level submit functions on button level submits, without validation.

The node form multistep tutorial is at http://drupal4hu.com/node/246 .

And while fixing that would be marvelous I do not believe such a miracle can happen in time given the amount of change necessary as described in #91

Your code has the same problem as the poll module issue mentioned above: On previous it skips validation, but still updates $node with the form level submit handlers. But yes setting #access == FALSE on all other elements does it.

After talking briefly with chx in IRC, I filed this issue in the Documentation project issue queue about adding comment #91 as a page to the Handbook:

#738750: Addition to field API pages

I've tentatively assigned that issue to myself, but if someone else would like to take it on, that would be fine with me (since I really have only a small clue what's going on).

Presumably this is now a non-critical issue, if it's been booted to Drupal 8?

I just committed #735808: fix multiple field value form to work with form API persistence, by the way.

fago's patch over in #735800: node form triggers form level submit functions on button level submits, without validation looks very promising, and might be one of the corner-stones to achieve at least some sanity for D7. That patch, and the possibility to entirely remove #builder_function in a follow-up issue, lets me hope again.

I'm slightly reclassifying this issue into a meta issue, since it served very well for various spin offs we already managed to tackle successfully. Thanks all for your work on those issues! Let's also keep this issue critical. We most probably won't put in dedicated API functions for entity forms, but at least the summary of this and all spinned off issues will have a major impact on the documentation that needs to be written.

No patch here.

#91 is a great description, but I probably miss something big here.

I don't understand why we couldn't simply:

• Transform #builder_function into #builder_functions, allowing other modules to act at the build stage to add stuff into the built entity
• Store the built entity into $form_state
• Rely either on entity-specific validation or on standard FAPI #validate for everything else (we just need to be sure that validation occurs on the built object, not on the raw form values
• Save the built entity previously stored in $form_state on #submit

One problem with #builder_function is that it needs to be called from all button-level clicks, including ones with #limit_validation_errors. So it gets in the way of fixing #763376: Not validated form values appear in $form_state, and it means every seemingly unrelated button on an entity form (like the file field remove button: #736298: Refactor file.module to use proper button click detection, enabling FAPI to improve button click detection security) that triggers a rebuild must know to call #builder_function: will cause much grief to contrib module authors.

With any luck, we'll be able to get rid of #builder_function entirely. #735800: node form triggers form level submit functions on button level submits, without validation does it for nodes (see #38 and #39 of that issue for a summary), and if we can do it for nodes, surely we can do it for the other entities.

Other than that, I think #102 is basically right and what we're attempting in #735800: node form triggers form level submit functions on button level submits, without validation.

So if I understand all this correctly, it's our failure to move every element-level validation to #element_validate that causes all this mess in the first place.

All this because we have a dilema:

• We want to validate the entity based on a fully built object
• We don't want unvalidated data in that object, but we rely on #validate functions to validate part of this data
• #fail

I see no way out except then assuming that, when we enter the validation, every element has been sufficiently validated and that we can safely build the entity object based on the form values. After that step is done, all subsequent validation should be performed on the built object.

I see no way out except then assuming that, when we enter the validation, every element has been sufficiently validated and that we can safely build the entity object based on the form values. After that step is done, all subsequent validation should be performed on the built object.

But if validation fails, the updates would still be in the entity object - except we clone it for validation or prevent caching of the updated object. Also we may not do so in case #limit_validation_errors is turned on, as some values might not have passed validation even the form validates fine. Thus we may not introduce something like #builder_function that is automatically invoked - we really should scratch that. Instead we have that nice feature of form API value persistence when rebuilding a form, so we don't have to update $entity in order to keep the values during rebuild any more.

Also yched pointed out in #74 in regard to entity level validation on fully built objects:

This brings a chance of blocking users on validation errors they cannot fix since they lie in the object as it currently exists. This is not only true for the specific case of field validation.

To avoid that, we cannot untie entity level validation from the form workflow. Thus we could just bring entity validation back to the form level with a little helper hook like

- hook_entitytype_validate($unchanged_entity, $form, $form_state);

-> That way we can use hook_entitytype_submit($entity, $form, $form_state) to update the entity afterwards. Imho that's the way to go for D7, as we don't have the complicated "build entity before validation" step involved at all.

Whether we want to implement entity level validation or not, imo that's d8 stuff.

Friends: I think the patch in #735800-65: node form triggers form level submit functions on button level submits, without validation gets us as far as we can get with this in D7. Please review it to see if you agree. A long summary is in the comment just below the one with the patch. The patch reflects #105 that validation pipeline changes need to wait until D8. @Damien: given your experience with contrib-defined entities, your review would be much appreciated.

Finally #735800: node form triggers form level submit functions on button level submits, without validation landed! Now entity forms should be much easier :)

However for working out how they are properly extended I opened #830704: Entity forms cannot be properly extended.

Thanks for the follow-up issue, fago.

I'm not sure there's anything left here that's critical for D7. Please reset this issue's priority if I'm wrong. I'm tempted to change the version of this issue to "8.x-dev". Does this meta issue still have a purpose for D7?

Bumping to D8 and removing tags. I think this meta issue has run its course for D7, though many more improvements are needed in D8. If new individual issues are needed (whether for D7 or D8), please open them, and link to them from here. If someone wants to bump this back to D7 and assign tags you believe to still be useful, please do so.

Couple of related, targeted issues:
#1261310: uid is not available in hook_field_attach_validate()
#1220212: Provide original entity in hook_field_attach_validate()

This is me miming a productive comment so sun doesn't scold me for subscribing. ;)

I think we've improved the entity/field API integration as much as possible for D7, and for D8, the move to classed entities as well as the upcoming #1499596: Introduce a basic entity form controller will improve it further, but from the ground up. So let's close this meta issue.

FWIW, the last major summary was in #91, and was mainly about dealing with unvalidated form input being used on entity properties. If any todo is left with regard to that, then we should open a separate, focused issue for that.

Re-adding tags for lookup/statistical purposes.