Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
We are not handling field translations directly, so it will be up to the object types (e.g. nodes). We do not currently know whether we should expose a prepare_translation operation or leave it up the calling fieldable type to do completely. We will ask the translation gurus about this.
Comment | File | Size | Author |
---|---|---|---|
#43 | field_prepare_translation-43.png | 14.89 KB | plach |
#37 | field_prepare_translation-362021-37.patch | 4.59 KB | plach |
#30 | field_prepare_translation-362021-30.patch | 13.81 KB | plach |
#29 | field_prepare_translation-362021-29.patch | 14.13 KB | plach |
#28 | field_prepare_translation-362021-28.patch | 13.68 KB | plach |
Comments
Comment #1
yched CreditAttribution: yched commentedRegardless of what happens with #367595: Translatable fields, we probably need this now. Body now being a field, the 'prefill body field on new node translation form' is currently broken.
The current code contains unfinished code for field_attach_prepare_translation(), field_default_prepare_translation() (directly taken from CCK for nodes).
They simply need to be updated to the D7 Field API style - and then node.module needs to call field_attach_prepare_translation() appropriately.
Comment #2
yched CreditAttribution: yched commentedbumping to critical. Just needs to be done.
Comment #3
bjaspan CreditAttribution: bjaspan commentedWe still have field_attach_prepare_translation() and hook_field_prepare_translation(). What is their status?
Comment #4
yched CreditAttribution: yched commentedIf D6-style translation (e.g different nodes) remains in core (which is still not sure, if I read webchick's comments on one of the TF followup threads, I think it was 'TF UI'), then we need the field_attach_prepare_translation() func and hook to support "body prefilled with source language in 'new translated node' form".
The functions are nothing more than stub, not to-date code for now.
Comment #5
sunsubscribing
Comment #6
plachThis one depends on the destiny of #539110: TF #4: Translatable fields UI. If the current node translation stays in core I'll work on this.
Comment #7
sun.core CreditAttribution: sun.core commented@plach: What's the future of this issue?
Comment #8
plach@sun: Simply didn't have time to work on it, yet. If someone else wishes to take it, feel free :)
Comment #9
sun.core CreditAttribution: sun.core commentedComment #10
yched CreditAttribution: yched commentedNope, this is for D7. Although, not critical.
Comment #11
plachOh, that's why I couldn't find this anymore.
Comment #12
plachHere's an initial draft, definitely needs work. Let's see if the bot is happy.
Comment #13
sunDiscussed this patch with plach. To solve Translation module's requirements, we need to introduce a new hook_field_prepare_translation() for every field module. The current logic in Translation module is based on D6 and does not account for field API at all.
I'm also inclined to bump this issue to critical, but plach just tells me that Translation module currently doesn't work like it should anyway. A quick fix without API change would lead to a critical security issue. The API change (addition) is required to not introduce a security issue. But leaving as normal for now.
Comment #14
yched CreditAttribution: yched commentedFrom CCK D6 :
First line called the '"default" behavior : content_field($op = 'prepare translation') - in D7, that would be a field_default_prepare_translation() func.
Second line called the field type's hook_field($op = 'prepare translation') - in D7, that would indeed be hook_field_prepare_translation()
Nodereference D6 indeed implemented this to replace do some advanced replacement (trying to find the translation of the referenced node if it exists - code was by nedjo, IIRC :-p)
So true, we do need this field-type hook, in addition to the default method.
Comment #15
plachMy bad: I wrote the patch some time ago and I thought I introduced hook_field_prepare_translation() in #12. Perhaps we are missing hook_field_attach_prepare_translation(), though.
Comment #16
plachThis should fix the
format_access()
issue by implementinghook_field_prepare_translation()
in the Text module; I don't think the other core fields need implementing it.Docs are stil needs work but code should be ok. Again let's see if the bot is happy.
Comment #17
sunI'm not sure whether other field_attach functions are following a certain argument order/pattern, but if they do, it would be good to make these arguments follow those.
Without looking up other functions, I guess that the following order would be more natural:
field_attach_prepare_translation($entity_type, $entity, $langcode, $source_entity, $source_langcode)
If this change is required for this patch, then I guess we need a short inline comment to explain it.
Do we still need this?
Powered by Dreditor.
Comment #18
plachFixed #17 and the documentation.
Actually this is not needed: it just enables the admin theme for the translation page. I was unsure this deserved a dedicated issue.
Probably if we go on introducing
hook_field_attach_prepare_translation_alter()
,hook_node_prepare_translation()
won't be needed anymore. We just need to understand if this is still feasible.Comment #19
yched CreditAttribution: yched commentedCopy the code from text_field_p_t() ?
Should be 'prepares' (3rd person)
That's a bit too much. The scope of this function is only to fill-in the form default values for Field API fields when displaying the 'add new (translated) node' form.
'allow' should be 'allows' - the sentence feels awkward even with this correction, though.
Proposal :
"This function is used as part of the 'per node' translation pattern implemented by translation.module."
?
Perhaps we should also state that it only makes sense for nodes, and that (unlike other field_attach_X() functions), entity-type modules probably don't need to call it ?
I'd suggest : "Let field types handle their own advanced translation pattern if needed." ?
Not sure either. Since this is limited to nodes, the
module_invoke_all('node_prepare_translation', $node);
in translation_node_prepare() could be enough (and that hook is still needed for non-field node components)3rd person
I'd say this deserves a separate patch.
Also, some tests around this in translation.test could be worth it ?
109 critical left. Go review some!
Comment #20
sunThe addition of drupal_alter() was my suggestion, having the idea in mind that contributed modules might want to "really" prepare field values for translation. For example, consider machine-based translation web services or simple value conversions and stuff like that.
Comment #21
plachThis addresses #19-#20.
IMO
field_attach_prepare_translation()
makes sense for any entity (above all new ones we never thought about) implementing a per-entity translation pattern, that's why it replaceshook_node_prepare_translation()
withhook_field_attach_prepare_translation_alter()
: the latter lets achieve the same tasks as the former but in a more general way.Comment #22
plachI forgot to update poll and node.api.php.
Comment #23
plachRelated issue: #771082: Enable the admin theme for the translation overview page.
Comment #24
sun"If the translating user is not permitted to use the assigned text format, we must not expose the source values."
That ->translation_source seems to be duplicate information now.
114 critical left. Go review some!
Comment #25
plachFixed #24
It's used across the whole Translation.module, it might be problematic to suppress it. I fixed
poll_field_attach_prepare_translation_alter()
to avoid using it though.Comment #26
plachDo we need yched for a final pass here?
Comment #27
sunyched is short on time currently. But this patch looks good anyway, thanks!
Comment #28
plachRerolled.
Upgrading to critical as we can't release D7 without this one.
Comment #29
plachAdded function body to
hook_field_attach_prepare_translation_alter()
infield.api.php
.Comment #30
plachwindows newlines...
Comment #31
YesCT CreditAttribution: YesCT commented#30: field_prepare_translation-362021-30.patch queued for re-testing.
Comment #32
webchickThis looks sane to me. Basically makes the existing node-centric translation stuff entity-centric instead. There are API changes here... I'll do my best to summarize to my current understanding, but it'd be great for one of the folks here to confirm:
1. hook_field_prepare_translation now takes a source_entity and source_langcode param.
2. There's a new hook_field_prepare_translation_alter()
3. field_attach_prepare_translation() now takes entity details rather than just a node object
4. Fields that deal with translatable data (e.g. "text" field) need to implement hook_field_prepare_translation().
5. hook_node_prepare_translation() is removed. Use hook_field_attach_prepare_translation() now.
6. If your entity module used to invoke hook_node_prepare_translation:
you now do call field_attach_prepare_translation:
And... committed to HEAD!
Comment #33
yched CreditAttribution: yched commentedNot exactly. For simple data (text, number...), the default behavior in field_default_prepare_translation() (just copy over the values from the source entity as default values in the translated entity form) is enough.
This hook is for stuff like nodereference fields, that want to do advanced logic like 'if source node references node A, and node B is a translation of node A, then prepoluate node B as a default value in the translated node' (e.g transitivity - easier with a drawing :-p)
Comment #34
plachJust a specification: Text is implementing hook_field_prepare_translation() because it needs to skip copying a filtered text field value if filter_access() reports that the user cannot view it. Just like we did in translation_node_prepare() only for the node body.
Comment #35
plachGiving another look to this code, I'm afraid it does not handle multiple values properly :(
Powered by Dreditor.
Comment #36
plachWorking on a followup patch with some tests, hope to post it soon.
Comment #37
plachThe change is pretty straightforward: hide just the items the user has no access to, instead of all items based on the first one. The patch ships a test that shows that the current behavior is broken.
Comment #38
plachRelated issue: #822128: "Textarea + summary" widget broken when field allows multiple values (followup) and associated JavaScript uses fragile selectors.
Comment #39
yched CreditAttribution: yched commentedI'm wondering if there's something we should or could do to signal that the 'translate node' form is not populated with all source values. Would be nice to have sun's feedback on this.
Meanwhile, code looks good and is obviously more correct than the current. RTBC.
Comment #40
YesCT CreditAttribution: YesCT commented#37: field_prepare_translation-362021-37.patch queued for re-testing.
Comment #41
sununset? Shouldn't we empty the value and format instead?
I'm not 100% sure, but when considering that I would not know of an originally existing source value in between of other values, then I'd likely prefer to see an empty widget/value among others to at least have a clue that something must have been there...?
57 critical left. Go review some!
Comment #42
sunplach just tells me that this is actually the case and even covered by the added tests.
Comment #43
plachHere is a screenshot taken from the latest simpletest debug message.
Comment #44
marcingy CreditAttribution: marcingy commented#37: field_prepare_translation-362021-37.patch queued for re-testing.
Comment #45
Dries CreditAttribution: Dries commentedCommitted to CVS HEAD. Thanks!
Comment #47
plachRelated issue: #879910: text_field_prepare_translation() broken
Comment #48
jhodgdonThis was apparently never updated on the 6/7 module update page. Needs to be.
EDIT: See comment #32 above for a summary.
Comment #49
yched CreditAttribution: yched commentedThis hook has no equivalent in D6 core, so this doesn't belong to the 6/7 module update page.
Comment #50
jhodgdonDuh. Sorry about that.