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.
Comment | File | Size | Author |
---|---|---|---|
#62 | field_form_fix_revisions-629252-62.patch | 8.69 KB | plach |
#60 | field_form_fix_revisions-629252-60.patch | 8.57 KB | plach |
#50 | field_form_fix_revisions-629252-50.patch | 8.5 KB | plach |
#45 | field_form_fix_revisions-629252-43.patch | 8.9 KB | yched |
#43 | field_form_fix_revisions-629252-43.patch | 8.9 KB | plach |
Comments
Comment #1
plachThe 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.
Comment #3
plachthis should fix the tests
Comment #4
yched CreditAttribution: yched commentedHm. 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...Comment #5
plachI 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.
Comment #6
yched CreditAttribution: yched commentedPossibly :-). 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).
Comment #7
plachYou'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.
Comment #8
plachI 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.
Comment #9
yched CreditAttribution: yched commentedYou 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.
Comment #10
sunI 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.
Comment #11
yched CreditAttribution: yched commentedCreated #636834: Field revision data messed up when user has no 'edit' access on the field for the issue pointed in #8.
Comment #14
plachHere 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.
Comment #16
plachexceptions should be fixed
Comment #17
yched CreditAttribution: yched commentedI'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.
Comment #18
plachI 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.
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 tofield_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.
Programmatic saves already work, provided that the object to be saved has all the field translations in place.
Comment #19
sunThe 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'].
Comment #20
plachThe 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:
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.
Comment #21
plachnewlines :(
Comment #22
yched CreditAttribution: yched commentedFigured 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.
Comment #23
yched CreditAttribution: yched commentedHm. Unless we explicitly make field_default_form_errors() *not* report errors on fields that are not actually present in the form.
Ach. Tricky case.
Comment #24
sunAs 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
Comment #25
yched CreditAttribution: yched commented@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().
Comment #26
plachOne 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.
Comment #27
yched CreditAttribution: yched commentedYep, 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'.
Comment #29
sunUnfortunately, 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'] ?
Comment #30
plachThis 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:
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']
.Comment #31
yched CreditAttribution: yched commentedMinor: 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.
Comment #32
yched CreditAttribution: yched commented[slightly edited the code above for readability]
Comment #33
plachI 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'.Comment #34
sun.
Comment #35
yched CreditAttribution: yched commentedHardcoding $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.
Comment #36
yched CreditAttribution: yched commentedOpened
#644332: Do we need to abstract $object->revision ('new revision') property
#644338: file_field_update() triggers a full entity load durung entity_save()
Comment #37
sun(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.
Comment #38
plachThis one incorporates suggestions from #31 and #37.
Comment #39
plachTrailing whitespace :(
I'm on crack. Are you, too?
Comment #40
sun(very minor) We don't use this syntax in Drupal. Keep it on separate lines.
I'm on crack. Are you, too?
Comment #42
yched CreditAttribution: yched commented#628642-9: Taxonomy field 'column' should be 'tid' instead of 'value' - let's keep the bot out for now.
Comment #43
plachOk, the attached patch takes care of #40.
Comment #44
yched CreditAttribution: yched commentedBot is disabled now, so back to CNR, tests will resume when HEAD is fixed.
Comment #45
yched CreditAttribution: yched commentedRepost just to make sure the bot catches it.
Comment #46
yched CreditAttribution: yched commentedGreen.
Comment #47
sunHold on. Let's discuss the removal of #type value elements in #367006: [meta] Field attach API integration for entity forms is ungrokable first.
Comment #48
plach#45: field_form_fix_revisions-629252-43.patch queued for re-testing.
Comment #50
plachrerolled
Comment #51
plachAs #367006: [meta] Field attach API integration for entity forms is ungrokable has been moved do D8, I'd set this back to RTBC.
Comment #52
plachComment #53
yched CreditAttribution: yched commentedNot sure how this now plays with the refactorings in #735800: node form triggers form level submit functions on button level submits, without validation
Comment #54
plachdowngrading again :(
Comment #55
sunI thought you told me that this patch would not really touch Form API, but rather Field API only?
Comment #56
plachAFAICT it only touches Field's Form API integration. Did I misunderstand your question?
Comment #57
fagoHm, 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.
Comment #58
sunIf 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.
Comment #59
plach@fago:
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.
Comment #60
plachrerolled
Comment #62
plachrerolled better :)
Comment #63
plachBack to RTBC
Comment #64
effulgentsia CreditAttribution: effulgentsia commentedI 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).
Comment #65
Dries CreditAttribution: Dries commentedOK, reviewed it, and committed to CVS HEAD. Obviously a small but important change.