Background:
#1969728: Implement Field API "field types" as TypedData Plugins is moving the "field type" API from "pseudo hooks" (hook_field_[OP]()...) to methods on EntityNG FieldItem classes.
Among those is hook_field_prepare_translation() - not done yet in the patch above.
This strictly relates to the old "translation set"-based node translation model (translation = separate node).
The hook was part of the process of pre-filling form values on the "create new translation node" form, based on the source node values. Field API automatically took care of pre-filling the original values from the source node, the hook let field types do additionnal stuff on top of that:
- A typical use case was for node_reference to point to the *translation* of the target node rather than the target node itself.
D8's entity_reference currently doesn't bother with this though (I guess it doesn't bother supporting special cases for nodes and their old translation model)
- Another example is text field *unfilling* the source values if the user creating the translation node doesn't have permissions to use the input format those source values are in (see text_field_prepare_translation()). I'm not sure how this case is dealt with in the new translation model, but that's not through text_field_prepare_translation().
So the question is:
Should we bother porting hook_prepare_translation() to the new field type API right now ?
Or: do we want to make "prepareTranslation() : do extra massaging for the special case of nodes in their old translation model" part of the new API for field types ?
Comments
Comment #1
plachNot sure whether we are dealing with this atm, honestly. However this should be done in
hook_entity_prepare()
(once we have it).IMO we shouldn't unless this actually blocks the conversion.
Comment #2
yched CreditAttribution: yched commentedtagging
Comment #3
fagoForcing contrib authors to think about that translation method when it is at least not available any more after installation, would be a rather big WTF and very confusing I think.
So, could we just drop it? If you really need to care about it you can still do it manually, e.g. loop over all the fields and take care of your fields in the regular hook of the module.
Comment #4
yched CreditAttribution: yched commentedYep, let's do this.
Worst case, if we find it's still needed, won't be too late to add it on top of the new API
Comment #5
plachYes, we should drop it. Translation should probably have leveraged
hook_node_prepare()
in first place.Comment #6
yched CreditAttribution: yched commentedHmm. Actually, doing so means that it's not only "field types don't need to bother about doing special stuff for prepare_translation", it's also that the original field values don't get prefilled in the 'create new node translation' form anymore.
Sounds like seriously hindering the feature if, god forbid, we found out we had to keep supporting it in D8 ?
Comment #7
plachWe prepulate the node form also in ET and we don't need that hook, I guess we should be able to work around it.
Comment #8
yched CreditAttribution: yched commentedOK, removing then :-).
Comment #9
yched CreditAttribution: yched commentedRemoved in #1969728-144: Implement Field API "field types" as TypedData Plugins :-)
Thanks for the feedback !
Comment #10
fagoAs noted in #3:
So is that the way forward to not introducing a regression? Also see #1969728-149: Implement Field API "field types" as TypedData Plugins
Comment #11
fagoSetting back to needs work, as it looks like it needs more feedback on what the way forward for developers is here.
Comment #12
yched CreditAttribution: yched commentedWhat I understood from the above was "let's nuke it" - but if I got that wrong, the commit that removed prepare_translation in the sandbox is http://drupalcode.org/sandbox/yched/1736366.git/commit/f1bdc14427f3744fd..., in case it needs to be reverted.
I would still drop field_attach_prepare_translation() & _field_invoke() though, and have translation_node_prepare() directly iterate on fields and call Field::prepareTranslation().
This might be doable through invokeFieldMethod() if we add the possibility for it to receive two extra $a and $b params that are passed directly to each field method (a bit like _field_invoke() did so far)
Comment #13
Gábor HojtsyI understood let's remove the hook, but that does not let us remove the feature :) It should be re-implemented in a different way then.
Comment #14
plachI said the we should be able to make Translation work also without that hook, I am surprised that just killing it doesn't make tests fail.
Comment #15
yched CreditAttribution: yched commented@plach: well, the commit also removes the corresponding tests :-)
Comment #16
fagoDo you mean for entity translation? *confused*
This is just about removing the hook|callback|method for field types, not about removing the node-level hook. That's another interesting question though I guess.
Comment #17
plachRelated issue: #2025991: Introduce hook_entity_prepare_form() to generalize hook_node_prepare().
Should be an easy one, reviews welcome :)
Comment #18
fagook, trying to summarize this:
Proposal is to remove the prepare_translation callback from field type implementations. Reasons:
The existing use-case for the hook in core is:
I don't think checking access should be done here though, it's better done by the access() implementation of the item I think. We should verify this works correctly with ET though - so we probably won't to keep the TextTranslation test case from http://drupalcode.org/sandbox/yched/1736366.git/commitdiff/f1bdc14427f37... and make sure this works without the hook.
Comment #19
yched CreditAttribution: yched commentedAs mentioned in the OP, one other use case was node_reference_field_prepare_translation. But a) that case was strictly for the old translation model where translations were separate nodes, and b) entity_reference didn't seem to bother replicating this.
Also, note that the text use case is not about field access, but access to the input format.
Comment #20
fagoYes, but that could influence field access then? I guess problem is that we do not have per item field access, but just for the whole field. We could add support for $item->access() and use that for deciding whether something can be rendered/edited ?
Comment #21
yched CreditAttribution: yched commentedMmh. Dunno, I'm afraid it might be a bit complex.
- field_access() primarily is about the "abstract" field (e.g as used in a view without any bundle or actual entity yet). Not sure how that would play with "per item" access.
- Per Entity\Field (list of items) vs per FieldItem is another complexity level - if I don't have acces to one of the items, do I still have access to the Field itself ?
- nasty stuff on edit like : do I have access to the "old" item, do I have access to the "new" item (those are two different things and might contradict). I might even edit the item in a way that access(), asked on it, might return FALSE on the new values I just assigned ?
So this would look like a new validation error, like "you don't have the right to set this value for this field" ?
- "no access to the input format" means not editable, but still viewable.
There might be good concepts in there, but it feels a bit late in the cycle to explore IMO - especially as a spin-off of the code in the old, deprecated/almost-deprecated text_field_prepare_translation()
Comment #22
andypostWhy not make it on field_instance level? Once comments become field then we unfreeze #1903138: Move global comment permissions to comment-type level so separation of bermission would go to bundle level (post comments for article, read comments per order)
Comment #23
fagoIndeed. So what would be your suggestion to handle the snippet of text_field_prepare_translation() then?
Another way I could think of, would be adding a prepareForm() method to widgets - based upon #2025991: Introduce hook_entity_prepare_form() to generalize hook_node_prepare(). That could work analogously to prepareView() for formatters and hook_entity_prepare_view(), i.e. field types can provide base classes to allow for simple re-use of the prepareForm() logic across widgets.
Comment #24
fago#1969728: Implement Field API "field types" as TypedData Plugins got committed, so raising priority to critical.
Comment #25
PanchoNo patch, therefore active.
Comment #26
tim.plunkettDo we know the answer to the title's question yet?
Comment #27
plachCurrently the hook no longer exists in the D8 code base, the old translation module is going to be removed from core and hook_entity_prepare_form allows to implement the logic outlined in #18 easily.
So I'd say the answer is "actually we don't" :)