Current code is:

function field_is_translatable($entity_type, $field) {
  return $field['translatable'] && field_has_translation_handler($entity_type);
}

function field_has_translation_handler($entity_type, $handler = NULL) {
  $info = entity_get_info($entity_type);
  return !empty($info['translatable']);
}

Those two should most probably be methods somewhere - but where ? :-)

For field_is_translatable(), this could be:
- on the FieldItem class:
When those objects exist, there is an $entity to work with
OTOH, since those objects are the *values* themselves (possibly already "translated") - does it make sense for them to say "I'm translatable" ?
- on the Field config entity - then, works only for configurable fields
- on the FieldDefinitionInterface being introduced in #1950632: Create a FieldDefinitionInterface and use it for formatters and widgets
That one already has an isTranslatable() method, but it only reflects the "$field['translatable']" part (it specifies it should be translatable), not the "&& field_has_translation_handler($entity_type)" part ("it actually *is* translatable according to the rest of the site i18n setup")

Also, it seems that EntityNG::getTranslationLanguages() has some related / similar-but-not-quite-(?) logic to determine the list of languages that appear in an entity.

So yep, some cleanup / unification might be helpful here.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

plach’s picture

yched’s picture

According to @plach in #2004626: Make non-configurable field translation settings available in the content language settings, the plan is to get rid of the $field->translatable bool flag, and store translatability of both base and config fields in a separate CMI entry owned by translation_entity.module.

Not sure where that would put field_is_translatable(), but I guess that means not in the Field config entity, nor in the FieldDefinitionInterface.

plach’s picture

We have an issue to make translatability an instance property (#1893568: Make each field instance have its own translatable property, instead of sharing translatable property with others.). Is there any plan to support per-bundle settings for field definitions?

plach’s picture

Well, field_is_translatable() (or any replacement) would just need to check field definitions, ET is using its settings to alter them and mark fields untranslatable if they are configured as such.

In this scenario the field definition would be responsible to say whether the field supports translation, while the ET settings would determine if it has translation enabled.

yched’s picture

Is there any plan to support per-bundle settings for field definitions ?

You mean field definitions as in #1950632: Create a FieldDefinitionInterface and use it for formatters and widgets ?
Well, in that patch, Field and FieldInstance both implement FieldDefinitionInterface. So a $field and an $instance both are valid "field definitions".
- In case of $instance, it acts roughly as "merge($field, $instance)"
- In case of $field, it provides "sensible defaults" for the properties it doesn't know about (because they would be defined by an $instance). So if translatability is per instance, $field->isTranslatable() would have to return always TRUE or always FALSE.

But from #2004626: Make non-configurable field translation settings available in the content language settings, I'd infer that isTranslatable() would go out of FieldDefinitionInterface completely ?
It's another system (translation_entity) that knows whether a field / instance is translatable (and that decides whether this is by field or by instance). So if you want to know about translatability, you ask that system, not the FieldDefinition ?

plach’s picture

Well, in that patch, Field and FieldInstance both implement FieldDefinitionInterface. So a $field and an $instance both are valid "field definitions". [...]

Aaah, very cool :)

I'd infer that isTranslatable() would go out of FieldDefinitionInterface completely ? It's another system (translation_entity) that knows whether a field / instance is translatable

Well, at least in my vision $field->isTranslatable() might not be needed but we need at least $definition['translatable'], which is what tells you whether a field has translation enabled or not. If ET is disabled translatable fields cannot be translated, thus it doesn't matter whether they are translatable or not. When ET is enabled you can disable translation if you want from the UI.

The point is that we have the definition property the field system can rely on without knowing anything about ET, which would be just be responsible for altering it to match its settings.

yched’s picture

but we need at least $definition['translatable'], which is what tells you whether a field has translation enabled or not. If ET is disabled translatable fields cannot be translated, thus it doesn't matter whether they are translatable or not. When ET is enabled you can disable translation if you want from the UI

Right, but from #2004626: Make non-configurable field translation settings available in the content language settings, I understood that this configuration ("I want this field, whether config field or base field, to be translatable and that other one to not be") would be edited in a UI provided by ET and be stored in ET's own config entries, not in $field / $instance structures ?

plach’s picture

#2004626: Make non-configurable field translation settings available in the content language settings provides a UI to enable/disable translation for any (non-configurable) field but it has the problem of determining which non-configurable fields display on the UI (you don't want to translate a "nid"), hence it relies on the field definitions: if fields are marked as translatable it shows them in the UI, otherwise you cannot change their translatability. It also alters field definitions and marks them as translatable or not depending on its settings. When there are no settings available for a given field it just ignores it. This way the first time you configure a field you get the translatability status originally set in its definition, the following times you get the settings stored in ET's configuration.

As you can see we cannot completely skip something indicating whether a field supports translation.

yched’s picture

Hm, I might be a bit confused, let me see if I got this right.

You want to have two separate properties:
a) A property indicating support for translatability. This is hardcoded metadata about the field.
It dictates which fields (base or config) show up in the UI so that admins can choose to make them actually translatable or not.
b) A property indicating, among the ones listed above, those that have been configured to actually *be* translatable. This is configuration.

In D7, translatability only involves "Field API" ('configurable") fields, and
a) doesn't really exist - it's always TRUE, all Field API fields *support* translatability
b) is config, stored as part of the $field definition structure

So it looks like we're going to get creative on the naming side, because if b) is about being 'translatable', then a) is about... being 'translatable-able' ? :-)

Then, questions:
- For base fields, a) would be part of the hardcoded field definition, right ?
- For config fields, whose definition *is* config: is a) always TRUE like in D7 ?
Do we need to support creating custom fields that are never ever going to be proposed for translatability ?
Isn't level b) sufficient for config fields ? A two-step configuration seems a bit complex to make a field actually translated ("you need to make your field translatable-able, then you can make it translatable"...)
- Am I right in understanding that you want to store b) in one unified location in config, for both base and config fields, and that ET would be responsible for that ?

It also *alters* field definitions and marks them as translatable or not depending on its settings

It's the *alter* verb that worries me.
From my understanding, b) wouldn't be read from FieldDefinitions. FieldDefinitions only store a).

plach’s picture

- For base fields, a) would be part of the hardcoded field definition, right ?

Yep

- For config fields, whose definition *is* config: is a) always TRUE like in D7 ?

Exactly

From my understanding, b) wouldn't be read from FieldDefinitions. FieldDefinitions only store a).

Right now in #2004626: Make non-configurable field translation settings available in the content language settings FieldDefinitions store a) when ET is disabled and b) when it is enabled and the related field has an entry in the ET configuration. We actually don't need both a) and b) we need a) only the first time we show the translation UI for a field. This avoids a clash in terminology since both a) and b) ultimately mean the same thing: this field can be translated. Whether it is going to be actually translated is something the field system should not care of. AAMOF all original field values are going to be stored with 'und' language no matter if thery are translatable or not, so the field translatability doesn't really matter unless you enable ET.

yched’s picture

Hm. So FieldDefinition::isTranslatable() would do something like:

if (ET is enabled && it has settings about this field in this bundle) {
  return those settings
}
else {
  return the 'default translatability' 
    - as specified in the hardcoded definition for base fields
    - TRUE for config fields
}

?

Then yes, the notion of "this bundle" might be tricky for base fields, because I'm not sure they intend to provide separate objects for each entity bundle. @fago should be the one chiming in there.

fago’s picture

>...so the field translatability doesn't really matter unless you enable ET.

Hm, ok so we just flag everything as 'translatable' if it supports translation. Then ET comes in makes it configurable whether the translation UI is actually used for it + stores that on its own - right?

If so, that means API wise translatable fields being not translated by ET appear as translatable fields having no other translations. Sounds reasonable, but does that work out with querying also? I mean, we'd know which language we query or have translation fallbacks being picked up for querying also. Query-fallbacks are quite hard and probably out of scope for d8 though? What means we need to know whether it's really translated and have flags for a) and b) ?

plach’s picture

In my mind FieldDefinition::isTranslatable() would just return $this->translatable (or whatever it's called right now ;). It's ET's job to alter that value based on its configuration, the field system should not know anything of ET.

#2004626: Make non-configurable field translation settings available in the content language settings is doing more or less this, just only for non-configurable fields right now.

yched’s picture

In my mind FieldDefinition::isTranslatable() would just return $this->translatable (or whatever it's called right now ;). It's ET's job to alter that value based on its configuration, the field system should not know anything of ET.

I agree with the first and last propositions, but, again, it's the verb "alter" in the middle one that worries me :-). You don't mean physical, in-place alter of the value held in the FieldDefinition, right ?

yched’s picture

Issue tags: +D8MI, +Entity Field API

tagging

plach’s picture

I mean hook_entity_field_info_alter(), not sure why it's so scary :(

yched’s picture

Not sure how hook_entity_field_info_alter() is going to play with entity_field_info being FieldDefinition objects, some of them being the actual Field / Fieldinstance config entity objects). I sure don't want to let people alter FieldInstance config entities at runtime :-)

plach’s picture

So we would change the actual Field/FieldInstance config values based on the ET config instead?

yched’s picture

That wouldn't be great either :-(

IMO if the configuration about translatability of fields is stored "somewhere else" (outside of the field definitions), the function / method that check whether a field is translatable needs to read from that "somewhere else", not try to somehow inject / alter it in the structures that don't know the info.

That's what I meant in #5 with

It's another system (translation_entity) that knows whether a field / instance is translatable (and that decides whether this is by field or by instance). So if you want to know about translatability, you ask that system, not the FieldDefinition ?

plach’s picture

Well, then we need a way to make that work without hardcoding a dependency to ET, which wouldn't make sense. So FieldDefinition::isTranslatable() would say "who knows whether this field is translatable for real?" and ET would answer "I do", correct? Can we make this reasonably performant, given the amount of times it would be invoked?,

yched’s picture

Hm. hook_entity_field_info_alter() was a good place for that, sure.
I guess that's a question to keep in mind when deciding the future of getFieldDefinitions() / hook_entity_field_info() / hook_entity_field_info_alter() after #1950632: Create a FieldDefinitionInterface and use it for formatters and widgets...

yched’s picture

@plach: where do we stand on this now ?
What is the current official replacement for field_is_translatable() ?

plach’s picture

I'd say FieldDefinitionInterface::isTranslatable(): in D7 field_is_translatable() used to check also whether the entity type supported translation, but since now untranslatable fields behave like original translatable fields, I think we no longer need such a check.

yched’s picture

Thanks!
So I guess a patch here should replace field_is_translatable() calls with isTranslatable() ?

@plach: About isTranslatable() / setTranslatable() : #2114707-18: Allow per-bundle overrides of field definitions could use your feedback :-)

yched’s picture

Issue summary: View changes
Status: Active » Needs review
FileSize
5.75 KB

Let's see, then.

yched’s picture

I'm not really sure this will work. field_is_translatable() currently includes checks for $entity_info['translatable'] too, while $field->isTranslatable() is just "is the translatable bit set to true in the field".

This seems really related to the setTranslatable() / isTranslatable() stacked logic we're stumbling upon in #2114707: Allow per-bundle overrides of field definitions.

Status: Needs review » Needs work

The last submitted patch, 25: field_is_translatable-2018685-25.patch, failed testing.

yched’s picture

Status: Needs work » Needs review

25: field_is_translatable-2018685-25.patch queued for re-testing.

Can't reproduce that exception locally ?

yched’s picture

Well, green then :-)

andypost’s picture

Status: Needs review » Needs work

There's one reference left

$ git grep field_is_translatable
core/modules/field/field.multilingual.inc:33: * calling field_is_translatable(), which checks the $field['translatable']
yched’s picture

@andypost: that whole file and doc is stale and up for the execution board in #2067079: Remove the Field Language API, but sure, we might as well be consistent here...

plach’s picture

Status: Needs work » Reviewed & tested by the community

As I said in #23, I don't think the check for the entity type translatability is still necessary, and if it turns out it still is we should bake it into FieldDefinitionInterface::isTranslatable(). That's why this looks good to me :)

Since we have only a comment change and the previous patch was green, I assume this will be too. RTBC.

yched’s picture

yched’s picture

Title: Decide what to do with field_is_translatable() » Remove field_is_translatable()
Priority: Normal » Major

Re-titling.

This is blocking #2067079: Remove the Field Language API, so bumping priority accordingly. Would be cool to have a quickish commit ?

webchick’s picture

Priority: Major » Normal
Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x.

Status: Fixed » Closed (fixed)

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