field_attach_form() should make available all field translations on submit

The current patch adds to the form definition all the field translations not being edited as 'value' elements, this way they are available in the submitted form values, but they are not actually shown in the form.

The last submitted patch failed testing.

this should fix the tests

Hm. I'm still pondering whether this is a good idea.

At any rate, this might not play well with

at the beginning of field_default_form().
If the user has no 'edit' rights on the field, then we just don't include anything about the field in the form. At submit time, with no data for the field in the submitted form values, the field is left untouched. Similar to including the values as a '#type' = 'value', except this saves processing time of resaving the same values...

I think it's the price to pay for consistency: on load we get all translations not only the one we might need. However this should have no impact on unilingual sites.

Possibly :-). But what I mean is that even with the addition in this patch, the current way of handling non edit-accessible fields will probably still break #539110: TF #4: Translatable fields UI, so that part might need to be changed too (include the field as '#type = value' instead of just skipping it).

You're right, but then (perhaps) this is breaking plain node revisions too, I am curious to know how the HEAD is behaving when creating a new revision for a node while having no access to some fields. I'll make some test about this.

I can confirm that with the current HEAD if a user creates a new node revision without having access to a certain field, this is "left behind" and gets no new revision. I ain't sure this is by design but it seems like a bug to me.

You mean the new revision doesn't contain the field data ? Hm, that's likely, and that's a bug indeed. Nice catch.

That would be best in its own thread, though, because the fix for this does belong in field_default_form(). I'll try to post a patch later today.

For the issue at hand, I'm still not sure whether sticking all languages data in the form is a good idea. It bloats the form, and makes the function much less reusable outside of a 'regular' entity edit workflow. It could be best IMO to merge them in at the other end, in field_default_submit() or something. That would require putting them in $form_state instead. Having the original entity available at a standard place in $form_state would make sense to begin with, IMO. Crosslinking to #367006: Field attach API integration for entity forms is ungrokable (ouch for the issue title...), this is debated over there.

I don't think we want to populate all those values.

However, I think we want to make it possible to submit + store additional values - if they are set.

Hence, the Translate To Any contributed module could load + render all other languages in the form, and the values will be stored, if they exist. But normally, they don't.

Created #636834: Field revision data messed up when user has no 'edit' access on the field for the issue pointed in #8.

plach requested that failed test be re-tested.

The last submitted patch failed testing.

Here I tried a slightly different approach: as yched suggested in #9 I moved the field translation handling in field_default_extract_form_values().

Temporarily I stored the original object into $form_state['storage']['object'], suggestions on a better place are welcome.

I slightly refactored field_attach_submit() to accept a $revision parameter which is used to notify the Field API a new revision has been submitted. I ain't sure this is the right way, suggestions are welcome also here.

This last refactoring allows to avoid writing all the field translations to the storage everytime the entity is saved. However I don't agree with sun that this behavior should be relegated to contrib (if I understood his point well): field revision handling is part of the Field API internals and IMO so should be for any field translation. Failing to achieve this is a bug, exactly as #636834: Field revision data messed up when user has no 'edit' access on the field was.

The last submitted patch failed testing.

exceptions should be fixed

I'm not too hot on the $revision param for field_attach_submit(), sounds like a hack, it doesn't feel like it belongs there.

I don't think I exactly get the bug this is trying to fix, could you sum it up ?
Also, if there is a possible bug around stored translations when creating revisions, wouldn't that also be the case on programmatic saves ? The patch will only take care of form submitted saves.

I'm not too hot on the $revision param for field_attach_submit(), sounds like a hack, it doesn't feel like it belongs there.

I don't like it very much too, but I think there should be a shared way for entities to notify the Field API a new revision is being created/submitted. Any suggestion?
Otherwise all the field translations will be stored at each entity save; this is a non-issue on unilingual sites, though.

I don't think I exactly get the bug this is trying to fix, could you sum it up ?

The problem is very similar to the one solved by #636834: Field revision data messed up when user has no 'edit' access on the field: in the current workflow, due to the way field_attach_form() works, the $node object passed to field_attach_update() holds only the field translations corresponding to $node->language.
When creating a new node revision only those ones get a new revision, all the other field translations don't contain the field data.

Also, if there is a possible bug around stored translations when creating revisions, wouldn't that also be the case on programmatic saves ? The patch will only take care of form submitted saves.

Programmatic saves already work, provided that the object to be saved has all the field translations in place.

The latest patch is now almost 100% touching the logic that needs to be revamped in #367006: Field attach API integration for entity forms is ungrokable.

As already mentioned elsewhere (either in aforementioned issue or possibly #635552: [meta issue] Major Form API/Field API problems), we want to populate $form_state['storage'] with 'obj_type', 'object', 'fields', and all the other properties - and - very potentially remove all the arguments (except $form and &$form_state) from field_attach_form*() functions, making them always rely on the information contained in $form_state['storage'].

The current patch introduces a test that shows the bug and fixes field_test_entity_test_load() which didn't properly load old revisions.

It also suppresses the explicit check on the revision creation and removes the changes to field_attach_form()'s signature. We still need to find a way to understand if a new revision has been submitted.

@sun:

The latest patch is now almost 100% touching the logic that needs to be revamped in #367006: Field attach API integration for entity forms is ungrokable. [...]

I wasn't able to follow all the issues involved in the discussion going on in the metaissue. As I said in #14, the changes to field_attach_form() are absolutely temporary, I'm just waiting for the right solution to come up from the other issues.

Leaving to CNR for the testbot but actually it's CNW.

newlines :(

Figured one serious issue with either approach (merge in field_default_extract_form_values() or include '#type = value' in the form) : It validates values that are already in the base and are not entered by the user.

Imagine an integer field, with a 'min value' setting of '0'.
Create a node and some translations, with value '1' for the field.
Then edit the field and set the min value to '2'.
Any attempt at editing *any* translation will now fail validation, because there are some values for some language that are invalid.
And you cannot do anything about it, because the values are not editable in the form... You'd need to change all values at once.

That's also an issue with #636834: Field revision data messed up when user has no 'edit' access on the field, BTW.

Hm. Unless we explicitly make field_default_form_errors() *not* report errors on fields that are not actually present in the form.
Ach. Tricky case.

As far as Form API is concerned, only submitted values (in #value) of element #types are validated, which have the special #input => TRUE property.

uhm. btw, why do we have a duplicate form validation layer here? :P

@sun: "why do we have a duplicate form validation layer here?"
Because hook_field_validate() happens independantly of FAPI and has no notion of what widget is used or how it is structured.
See the beginning of #369964-1: Refactor field validation and error reporting for details.

Workflow is:
- FAPI #element_validate: widgets perform their own custom validation, that makes sense for them but not for other widgets (i.e : noderef autocomplete needs specific validation on its own "node title [nid:xx]" input format)
- FAPI #form validate: usually the entity validate callback will call field_attach_form_validate()
- field_attach_form_validate() -> field_attach_validate() -> hook_field_validate() : generic validation and reports an array of errors on the values it finds in $object (that have been put there by field_default_extract_form_values()
- the errors are then passed to the *widget's* hook_field_widget_error(), which knows on which actual sub element to report each error (e.g date widgets can have different sub fields, only the widget know which error goes where). This is done by field_default_widget_error().

One possible solution is moving the following code (or equivalent) into field_default_submit():

This should fix the bug reported here and, if we revert back the changes introduced in #636834: Field revision data messed up when user has no 'edit' access on the field, also the field access bug without messing validation.

Yep, should work I guess :-).

Attached patch :
- reverts the #type => 'value' element added in #636834: Field revision data messed up when user has no 'edit' access on the field for non-accessible fields,
- includes #21 but merges values in field_default_submit() as per #26
- only does so when the entity type is actually revisionable - since this adds more writes to the storage backend.

I'd have no real problem in harcoding a check on $object->revision == TRUE to further limit the merge to the actual case where a new revision is created.
Nodes are the only entity with revisions currently (er, with test_entity), we have the liberty to set a standard on $entity->revision == TRUE for 'new revision of then entity'.

The last submitted patch failed testing.

Unfortunately, some exceptions here. Otherwise, this probably is ok.

Storing $object in $form_state['storage']['object'] and re-using that information during form submission is a first step towards the required Field/Form API change.

Off-topic: I'm no longer sure whether it's a good idea to always include the $langcode for field widgets in forms. I especially have problems to wrap my head around the question of how hook_form_alter(), #process, #after_build, #element_validate, #validate, and #submit implementations can safely rely on the form elements and submitted values in a form, if their location in the form changes from language to language...? Especially #validate and #submit handlers mainly work on $form_state['values'], but the submitted values do not contain any language pointer -- i.e. how is my validate/submit handler supposed to access $form_state['values']['field_foo'][$always-different-key-PER-FIELD-here][0]['value'] ?

This should fix the exceptions in #27.
I introduced a check on the hardcoded $object->revision property as I agree with yched that this should make no harm and save unnecessary storage queries.
A slightly expanded comment is also there.

@sun:

I'm no longer sure whether it's a good idea to always include the $langcode for field widgets in forms.

This was done because Node simply cast $form_state['values'] to object to obtain the submitted $node data structure. Probably this is not actually necessary, and we might want to open a new issue to remove the language level.
However the actual $langcode to be used is stored in $form[$field_name]['#language'].

Minor: given the new check on $object->revision, I'd suggest the following code in field_default_submit():
(typed in the 'comment' input here, so 80 chars wrapping might be off)

We also need to document that $object->revision requirement in hook_entity_info(), and in field_attach_submit().

Other than that, RTBC.

[slightly edited the code above for readability]

I ain't sure on what should be the right way to document $object->revision. However perhaps I'm more in favor of a new object key: currently we have 'revision key' which seems off wrt 'id key'; we might want to introduce a new 'revision id key' which would correspond to 'vid' and use the current 'revison key' for 'revision'.

.

Hardcoding $object->revision vs. adding a new 'object key' in hook_entity_info():
I suggest we go with the current patch and discuss / adjust / document that point in a separate issue.

Note that file.field.inc already hardcodes some behavior on $object->revision in file_field_update(). So, 'Not invented here', in a way.
Hm, looking a this function a bit closer, it also triggers an entity_load during entity_save - and in a not too clean way, it seems.

Opened
#644332: Do we need to abstract $object->revision ('new revision') property
#644338: fragile code in file_field_update()

(minor) Trailing white-space and wrong "elseif" here.

Also aren't the first two conditions the same as !empty() ?

This review is powered by Dreditor.

This one incorporates suggestions from #31 and #37.

Trailing whitespace :(

I'm on crack. Are you, too?

(very minor) We don't use this syntax in Drupal. Keep it on separate lines.

I'm on crack. Are you, too?

plach requested that failed test be re-tested.

#628642-9: Taxonomy field 'column' should be 'tid' instead of 'value' - let's keep the bot out for now.

Ok, the attached patch takes care of #40.

Bot is disabled now, so back to CNR, tests will resume when HEAD is fixed.

Repost just to make sure the bot catches it.

Green.

Hold on. Let's discuss the removal of #type value elements in #367006: Field attach API integration for entity forms is ungrokable first.