field_attach_form() operates in single language mode, i.e. the field form elements are shown for just one language.

This behavior should not prevent from having all the field translations available to code being passed the array of submitted form values, which is exactly what happens now: we get an array containing only a single field translation for each object field.

This may cause tricky consequences, as yched pointed out in #539110-70: TF #4: Translatable fields UI/#2.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

plach’s picture

Status: Active » Needs review
FileSize
1.32 KB

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.

Status: Needs review » Needs work

The last submitted patch failed testing.

plach’s picture

Status: Needs work » Needs review
Issue tags: +translatable fields
FileSize
993 bytes

this should fix the tests

yched’s picture

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

At any rate, this might not play well with

  if (!field_access('edit', $field, $obj_type, $object)) {
    return $addition;
  }

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...

plach’s picture

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.

yched’s picture

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).

plach’s picture

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.

plach’s picture

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.

yched’s picture

Category: task » bug

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: [meta] Field attach API integration for entity forms is ungrokable (ouch for the issue title...), this is debated over there.

sun’s picture

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.

yched’s picture

plach requested that failed test be re-tested.

Status: Needs review » Needs work
Issue tags: +translatable fields, +API clean-up

The last submitted patch failed testing.

plach’s picture

Status: Needs work » Needs review
FileSize
3.25 KB

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.

Status: Needs review » Needs work

The last submitted patch failed testing.

plach’s picture

Status: Needs work » Needs review
FileSize
3.34 KB

exceptions should be fixed

yched’s picture

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.

plach’s picture

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.

sun’s picture

The latest patch is now almost 100% touching the logic that needs to be revamped in #367006: [meta] 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'].

plach’s picture

FileSize
5.62 KB

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.

plach’s picture

FileSize
5.49 KB

newlines :(

yched’s picture

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.

yched’s picture

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.

sun’s picture

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

yched’s picture

@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().

plach’s picture

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

<?php
  if (!isset($form_state['values'][$field_name][$langcode]) && isset($form_state['storage']['object']->{$field_name}[$langcode])) {
    $items = $form_state['storage']['object']->{$field_name}[$langcode];
  }
?>

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.

yched’s picture

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'.

Status: Needs review » Needs work

The last submitted patch failed testing.

sun’s picture

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'] ?

plach’s picture

Status: Needs work » Needs review
FileSize
8.9 KB

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'].

yched’s picture

Status: Needs review » Needs work

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)

  elseif (!empty($object->revision) && isset($form_state['storage']['object'])) {
    // To ensure new revisions are created with all field values in all languages, 
    // populate values not included in the form with the ones from the original object.
    // This covers:
    // - partial forms including only a subset of the fields,
    // - fields for which the user has no edit access,
    // - languages not involved in the form.
    $object_source = $form_state['storage']['object'];
    $items =  isset($object_source->{$field_name}[$langcode]) ? $object_source->{$field_name}[$langcode] : $items;
  }

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

Other than that, RTBC.

yched’s picture

[slightly edited the code above for readability]

plach’s picture

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'.

sun’s picture

Issue tags: +D7 Form API challenge

.

yched’s picture

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.

yched’s picture

sun’s picture

+++ modules/field/field.default.inc	26 Nov 2009 12:33:21 -0000
@@ -24,13 +24,24 @@ function field_default_extract_form_valu
+  // To ensure new revisions receive all field values, merge in values from the
+  // original object if the field translation corresponding to $langcode was not 
+  // included in the form.
+  // This might happen in two cases: when the user has no access to the field 
+  // and when editing a field value that actually has translations.
+  else if (isset($object->revision) && $object->revision && isset($form_state['storage']['object']->{$field_name}[$langcode])) {

(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.

plach’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
8.9 KB

This one incorporates suggestions from #31 and #37.

plach’s picture

+++ modules/field/field.default.inc	27 Nov 2009 10:00:57 -0000
@@ -24,13 +24,25 @@ function field_default_extract_form_valu
+    // languages, populate values not included in the form with the ones from 

Trailing whitespace :(

I'm on crack. Are you, too?

sun’s picture

Status: Needs review » Reviewed & tested by the community
+++ modules/field/tests/field.test	27 Nov 2009 10:12:36 -0000
@@ -2521,6 +2521,51 @@ class FieldTranslationsTestCase extends 
+    $eid = $evid = 1;

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

I'm on crack. Are you, too?

Status: Reviewed & tested by the community » Needs review

plach requested that failed test be re-tested.

yched’s picture

Status: Needs review » Active
plach’s picture

Ok, the attached patch takes care of #40.

yched’s picture

Status: Active » Needs review

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

yched’s picture

Repost just to make sure the bot catches it.

yched’s picture

Status: Needs review » Reviewed & tested by the community

Green.

sun’s picture

Status: Reviewed & tested by the community » Needs review

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

plach’s picture

Status: Needs review » Needs work
Issue tags: +translatable fields, +API clean-up, +D7 Form API challenge

The last submitted patch, field_form_fix_revisions-629252-43.patch, failed testing.

plach’s picture

Status: Needs work » Needs review
FileSize
8.5 KB

rerolled

plach’s picture

As #367006: [meta] Field attach API integration for entity forms is ungrokable has been moved do D8, I'd set this back to RTBC.

plach’s picture

Status: Needs review » Reviewed & tested by the community
yched’s picture

plach’s picture

Status: Reviewed & tested by the community » Needs review

downgrading again :(

sun’s picture

I thought you told me that this patch would not really touch Form API, but rather Field API only?

plach’s picture

Status: Reviewed & tested by the community » Needs review

AFAICT it only touches Field's Form API integration. Did I misunderstand your question?

fago’s picture

Hm, I don't think this approach would intercept with the mentioned refactorings. However I think it might be confusing to have a $form_state['entity'] beside $form_state['node'] and $form['#node'] - where are the differences?

$form_state['node'] contains the updated, edited entity that is being worked on. $form['#node'] is there for compatibility reasons and contains the same. $form_state['entity'] would contain the unchanged, source entity - but the name doesn't imply that.

So do we need to have the unchanged source object? Couldn't we just use $form_state['node'] with the updated workflow? Of course in general we would have to use $form_state[$entity_type] and just add it if it's not yet there.

Also I'd feel much better having the entity not another time more in the form cache. If we can't use use $form_state['node'], why don't just use entity_load()? Usually the unchanged object should be already in its static cache.

sun’s picture

Status: Needs review » Reviewed & tested by the community

If we are able to get both patches in independently of each other, then I'd say we should do so - even if that would lead to duplicate cached data in $form_state['node'] and $form_state['entity']. We can clean up the resulting handling in a separate issue + most likely, we want to standardize on $form_state['entity']. However, both issues shouldn't be hold off by that discussion.

plach’s picture

@fago:

why don't just use entity_load()?

One of the main purposes of this patch is avoiding tricky entity_load()s during an entity saving process.
See #539110-70: TF #4: Translatable fields UI/#2 for details.

plach’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
8.57 KB

rerolled

Status: Reviewed & tested by the community » Needs work

The last submitted patch, field_form_fix_revisions-629252-60.patch, failed testing.

plach’s picture

Status: Needs work » Needs review
FileSize
8.69 KB

rerolled better :)

plach’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC

effulgentsia’s picture

+++ modules/field/field.attach.inc	11 Apr 2010 14:32:55 -0000
@@ -555,6 +555,9 @@ function field_attach_form($entity_type,
+  // Save the original entity to allow later re-use.
+  $form_state['entity'] = $entity;
+

I just now discovered this issue, haven't reviewed it (it's RTBC, so I don't need to), but I saw the above line and thought it would be worth cross linking to #735800: node form triggers form level submit functions on button level submits, without validation (you may want to either skip to the latest posted patch or start reading that issue at #66).

Dries’s picture

Status: Reviewed & tested by the community » Fixed

OK, reviewed it, and committed to CVS HEAD. Obviously a small but important change.

Status: Fixed » Closed (fixed)
Issue tags: -translatable fields, -API clean-up, -D7 Form API challenge

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