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

plach’s picture

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

Not sure whether we are dealing with this atm, honestly. However this should be done in hook_entity_prepare() (once we have it).

Should we bother porting hook_prepare_translation() to the new field type API right now ?

IMO we shouldn't unless this actually blocks the conversion.

yched’s picture

tagging

fago’s picture

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

yched’s picture

Yep, 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

plach’s picture

Yes, we should drop it. Translation should probably have leveraged hook_node_prepare() in first place.

yched’s picture

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

plach’s picture

We prepulate the node form also in ET and we don't need that hook, I guess we should be able to work around it.

yched’s picture

OK, removing then :-).

yched’s picture

Status: Active » Fixed
fago’s picture

As noted in #3:

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.

So is that the way forward to not introducing a regression? Also see #1969728-149: Implement Field API "field types" as TypedData Plugins

fago’s picture

Status: Fixed » Needs work

Setting back to needs work, as it looks like it needs more feedback on what the way forward for developers is here.

yched’s picture

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

Gábor Hojtsy’s picture

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

plach’s picture

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

yched’s picture

@plach: well, the commit also removes the corresponding tests :-)

fago’s picture

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

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

plach’s picture

Related issue: #2025991: Introduce hook_entity_prepare_form() to generalize hook_node_prepare().
Should be an easy one, reviews welcome :)

fago’s picture

ok, trying to summarize this:
Proposal is to remove the prepare_translation callback from field type implementations. Reasons:

  • Different to all other field type callbacks there is no equivalent entity hook and it's invoked by translation module only.
  • We had one implementation in drupal core, which seems to be obsolete now (yched removed it already). I'm not sure how this use case is solved with entity translation though, maybe plach can comment on this?
  • The hook has seen little use in d7 - I've done a short look at drupalcontrib.org and the only contrib implementation I found there was i18n_taxonomy_field_prepare_translation() - actually not a field type callback but a general-hook implementation (via the alter).
  • If there exists any implementation, it can move over to general hook implementation of hook_node_prepare() + checking for the translation case + invoking it for the given field.
  • #2025991: Introduce hook_entity_prepare_form() to generalize hook_node_prepare() is about to generalize the until-now node specific feature to entities, which is needed to support the same feature in entity-translation, but that's something out of the scope of the field API conversion and not influenced by the removal of the field-type callback.

The existing use-case for the hook in core is:

/**
 * Implements hook_field_prepare_translation().
 */
function text_field_prepare_translation(EntityInterface $entity, $field, $instance, $langcode, &$items, EntityInterface $source_entity, $source_langcode) {
  // If the translating user is not permitted to use the assigned text format,
  // we must not expose the source values.
  $field_name = $field['field_name'];
  if (!empty($source_entity->{$field_name}[$source_langcode])) {
    $formats = filter_formats();
    foreach ($source_entity->{$field_name}[$source_langcode] as $delta => $item) {
      $format_id = $item['format'];
      if (!empty($format_id) && !filter_access($formats[$format_id])) {
        unset($items[$delta]);
      }
    }
  }
}

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.

yched’s picture

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

fago’s picture

Also, note that the text use case is not about field access, but access to the input format.

Yes, 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 ?

yched’s picture

We could add support for $item->access() and use that for deciding whether something can be rendered/edited ?

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

andypost’s picture

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

fago’s picture

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

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

fago’s picture

Priority: Normal » Critical

#1969728: Implement Field API "field types" as TypedData Plugins got committed, so raising priority to critical.

Pancho’s picture

Status: Needs work » Active

No patch, therefore active.

tim.plunkett’s picture

Do we know the answer to the title's question yet?

plach’s picture

Category: task » support
Priority: Critical » Normal
Status: Active » Fixed

Currently 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" :)

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