Problem/Motivation

Currently field translatability settings are very confusing:

  • Content Translation alters field definitions to set field translatability based on its own configuration, both for base fields and for configurable fields. This approach actually duplicates the translatability configuration data for configurable fields, making unclear who is the ultimate responsible for such configuration.
  • To determine whether a base field supports translation (and thus should be listed in the content translation settings UI and enabled for translation), CT checks whether the unaltered value for the translatable property is TRUE or FALSE. An untranslatable base field will not be listed in the UI and so it will not get a configuration entry based on which altering its definition.
  • This convoluted solution is required because we do not have a clean way to distinguish between the supports translation and has translation enabled concepts: basically the unaltered translatable value is used to determine translation support and the (possibily) altered value determines the actual translatability, which is stored in the CT configuration.
  • Additionally the CT UI seems to suggest that translatability can be set per-bundle but the actual value is actually shared among all field instances, making the UI very confusing for end-users. This also introduced a critical bug (#2217543: Cannot translate title of Basic Page node if other content types are not translatable) after the last refactoring of field definitions.

All this mess is caused mainly by two problems:

  • field translatability cannot be stored per instance
  • there is no centralized way to store configuration for base fields

Proposed resolution

  • Make translatability configurable at bundle level.
  • Use the split between field definition and storage definition to distinguish between translation support and translatablity:
    • translation support is something that is strictly tied to the storage (that is the ability to store multilingual values).
    • translatability is instead something that is tied to runtime status of the field definition (and is the information we need in 99.9% of cases).

    Field definitions are overridden per bundle and track field translatability, while storage definitions track field translation support. For this to properly work we should not be able to pass field definitions as storage definitions, otherwise per-bundle overrides might be misinterpreted as non-overridden base values. For translatability this would mean that a field not enabled for translation on a certain bundle could be interpreted as a field not supporting translation. Hence FieldDefinitionInterface should not extend FieldStorageDefinitionInterface. FieldDefinition will still implement both (this is fine for base fields), but #2224761: Add a generic way to add third party configuration on configuration entities and implement for field configuration we will introduce a FieldBundleOverrideInterface which will extend only FieldDefinitionInterface.

  • For now we will special case configurable fields and save their translatable status in their config entities. In #2224761: Add a generic way to add third party configuration on configuration entities and implement for field configuration we will introduce a new config entity to track per-bundle base field overrides, this way every bundle field definition will be "configurable" and will expose a save() method, thus removing the need for CT to alter field definitions.
  • Configurable fields always support translation by default. This can be changed by altering their storage deifnition if a field (e.g. the Commerce Price) should never be enabled for translation.

Remaining tasks

  • Find consensus on a solution
  • Write a patch
  • Fix tests
  • Reviews

User interface changes

  • The Field UI "Users may translate this field" checkbox has been moved from the Field edit form to the Instance edit form.
  • The Content Translation Settings UI will finally work as intended.

API changes

  • FieldDefinitionInterface no longer extends FieldStorageDefinitionInterface
  • FieldInstanceConfig::getField() is renamed to FieldInstanceConfig::getFieldStorageDefinition()
  • FieldInstanceConfig has a new property translatable
  • FieldInstanceConfigInterface therefore no longer extends FieldStorageDefinitionInterface which means FieldInstanceConfig no longer implements getProvider(), isRevisionable(), getCardinality(), isMultiple(), getTargetEntityTypeId(), isQueryable(), getPropertyDefinition(), getPropertyDefinitions(), getPropertyNames(), getMainPropertyName(), getSchema(), getColumns(), hasCustomStorage() - now to access these methods use FieldInstanceConfig::getFieldStorageDefinition()->METHOD()
CommentFileSizeAuthor
#96 field_translatability-2143291-96.patch123.54 KBplach
#96 field_translatability-2143291-96.interdiff.txt2.19 KBplach
#93 field_translatability-2143291-92-broken.patch120.09 KBplach
#92 field_translatability-2143291-92.broken.patch120.09 KBplach
#92 field_translatability-2143291-92.interdiff.txt6.83 KBplach
#92 field_translatability-2143291-92.patch121.35 KBplach
#88 2143291.88.patch119.7 KBalexpott
#88 87-88-interdiff.txt7.73 KBalexpott
#87 2143291.87.patch122.57 KBalexpott
#87 85-87-interdiff.txt6.38 KBalexpott
#85 2143291.85.patch115.78 KBalexpott
#85 81-85-interdiff.txt3.22 KBalexpott
#83 2143291.81.patch112.07 KBalexpott
#79 2143291.79.patch112.02 KBalexpott
#79 78-79-interdiff.txt23.2 KBalexpott
#78 2143291.78.patch89.93 KBalexpott
#78 74-78-interdiff.txt1.24 KBalexpott
#74 interdiff-comment-revert.txt687 bytesxjm
#74 2143291-74.patch89.94 KBxjm
#72 field_translatability-2143291-71.patch90.61 KBplach
#71 field_translatability-2143291-71.patch0 bytesplach
#64 field_translatability-2143291-64.patch91.46 KBplach
#64 field_translatability-2143291-64.interdiff.txt5.38 KBplach
#57 field_translatability-2143291-57.patch89.23 KBeffulgentsia
#52 field_translatability-2143291-52.patch89.24 KBplach
#52 field_translatability-2143291-52.interdiff.txt2.07 KBplach
#48 field_translatability-2143291-48.patch90.28 KBplach
#48 field_translatability-2143291-48.interdiff.txt5.8 KBplach
#46 field_translatability-2143291-46.patch92.02 KBplach
#46 field_translatability-2143291-46.interdiff.txt3.36 KBplach
#44 field_translatability-2143291-43.patch89.13 KBplach
#43 field_translatability-2143291-43.patch89.13 KBplach
#43 field_translatability-2143291-43.interdiff.txt1.37 KBplach
#41 field_translatability-2143291-41.patch87.76 KBplach
#41 field_translatability-2143291-41.interdiff.txt45.76 KBplach
#39 field_translatability-2143291-39.patch43.31 KBplach
#39 field_translatability-2143291-39.interdiff.txt5.8 KBplach
#37 field_translatability-2143291-37.patch38.45 KBplach
#22 2143291-22.patch1.66 KBpwolanin
#6 2143291-6.patch2.56 KBswentel
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

yched’s picture

Issue summary: View changes
swentel’s picture

Hmm, so you want to calculate this on the fly during hook_field_entity_load() ?

yched’s picture

content_translation alters the fields definitions at runtime and sets the 'translatable' property on the right ones.

That's what it does already - see content_translation_entity_field_info_alter() in HEAD? The hook currently runs on field definitions as "typed data definition arrays", which obfuscates a bit what is happening.

But after #2047229: Make use of classes for entity field and data definitions, the hook runs on FieldDefinitionInterface objects - some of which are field.modules config entities - and does ->setTranslatable(bool) on them (search for content_translation_entity_field_info_alter() in https://drupal.org/files/issues/d8_field_definition_class_6.patch).

Meaning, field config entities do not need (and thus shouldn't) store 'translatable', since it's also stored elsewhere, and set from the outside...

swentel’s picture

Well, right, it does that in current HEAD too no ? But the entity field definitions are cached in getFieldDefinitions() so just wondering where the best place would be then to put it on the field object at runtime then since content_translation_entity_field_info_alter() will only run once no ? Or do we just get the field definition in isFieldTranslatable() ?

(going todo a little test now on during the sessions here at droidcon)

yched’s picture

Mmh. Content_translation_entity_field_info_alter() currently already works before the "list of fields" gets cached, so I guess content_translation does a cache_clear when its "is translatable" settings change ?

swentel’s picture

Status: Active » Needs review
FileSize
2.56 KB

Curious how this will behave

Status: Needs review » Needs work

The last submitted patch, 6: 2143291-6.patch, failed testing.

plach’s picture

@#5: exactly :)

Anyway, +1 on #3.

@swentel:

We need to make sure Field module's entity field definitions as always translatable. Then CT will pick them up and make translatability configurable. Fields that are natively untranslatable (like for instance the uuid) are just left alone.

yched’s picture

We need to make sure Field module's entity field definitions as always translatable

Well, that means for config fields, $field->translatable should still be "FALSE, unless e_t alters it otherwise".
We can't make $field->isTranslatable() suddenly return TRUE on all config fields :-)

The drawback with that whole approach ('translatable' not stored in the field YML anymore but always altered in at runtime) is that you can't simply set it up when creating the field anymore, but need to manually set it in e_t's config.
- This means a bunch of tests need e_t now (or some custom implementation of hook_entity_field_info_alter()) - hence the fails here.
- This means it's way less friendly for shipping fields in default config and make them translatable.

Wondering if we should still save 'translatable' in our field yaml files :-)
Then e_t admin form would need to act differently for base fields and configurable fields:
- for configurable fields, update the field config directly
- for base fields, still use its own config storage, since there's no other place for those
And e_t_field_info_lter() only needs to alter base fields, not configurable fields.

It just means configurable fields can be made 'translatable' by themselves, without e_t (which is currently the case). e_t happens to provides the UI for it.

This being said, e_t admin screen offers per-bundle granularity, while only $fields have a translatable flag, not $instances ?
I'm probably missing something as to how it works currently, but shouldn't the 'translatable' flag move to $instance rather than $field ?

yched’s picture

Title: Do not store 'translatable' in Field config entities anymore » Clarify handling of field translatability
Issue summary: View changes

As per #2114707-54: Allow per-bundle overrides of field definitions, repurposing the issue for "sort out the flow of field translatability".

Updated the OP with the proposals from #9 above and #2114707-14: Allow per-bundle overrides of field definitions

plach’s picture

I read again all the discussion at #2114707: Allow per-bundle overrides of field definitions before getting back to this. Here are my current thoughts:

  • I agree we need clean way to distinguish between the "supports translation" and "has translation enabled" concepts.
  • I don't like the idea of special-casing configurable fields: I think our ultimate goal should be to port to core every concept that is needed outside the Field module. Every time we don't do that, we are actually limiting the use-cases custom fields not provided by the Field module will be able to implement.
  • I don't like the idea of a new method on the field definition because it would make DX more convoluted: there would always be the risk that one picked the wrong one. The main reason DX would be worse here is that wherever we need a complete field definition (an instance in the old parlance?), we are actually needing the runtime value, that is the "has translation enabled" concept. The fact that a field may be untranslatable because it does not actually support translation is an information that is completely useless in most of the current core use-cases.

If I am understanding the discussion happened in #2114707: Allow per-bundle overrides of field definitions correctly, it seems we are going to introduce two different concepts: a field storage definition, holding all the field info that are common to the field instances and a full field definition, a "runtime" definition.

This split is IMO perfect to distinguish the two concepts we are dealing with here:

  • translation support is something that is strictly tied to the storage, in fact the storage controller could use that information to generate the proper schema.
  • translatability is instead something that is tied to runtime status of the field definition (and is the information we need in 99.9% of cases).

That said, if we had a way to access the field storage definition and the field definition separately, we could easily solve the current ambiguity, without the need to introduce two methods on the field definition.

In this scenario this is how I would envision the whole thing to work:

  • Configurable fields always have the translatable property set to TRUE in their field storage definition.
  • The Field config translatable setting goes away.
  • Base field storage definitions set their translatable property to TRUE wherever that makes sense for them. For instance nid would be FALSE, title would be TRUE).
  • All (base/configurable) field definitions are by default non-translatable.
  • Content Translation implements hook_entity_field_info_alter() to alter just field definitions based on its configuration.
  • Only fields whose storage definitions have the translatable property set to TRUE are listed in the UI.

This would be consistent with the way translatability is handled for entity type info/bundle info: the translatable property at entity type level indicates whether the entity type supports translation, while the bundle-level property is the runtime information that indicates whether a specific bundle is enabled for translation.

yched’s picture

I don't think we can afford to wait on the "field definition / field storage definition" split to happen first to clear the 'translatable' situation here. Even though I'm convinced that the split is the approach we should have gone for, I'm frankly not sure I have it in me to make this happen at this point. Burnout ++ :-/

+ see #9 for the drawbacks of not storing 'translatable' in the yaml of configurable field instances :
- you need to enable entity_translation to make fields translatable
That's a regression from D7 (+ now a bunch of tests need to enable e_t).
- you cannot simply ship translatable fields in your module's default config/ anymore, you also need to ship the corresponding settings for e_t, and those probably won't have the granularity to ship that single info ("field F is translatable in entity type ET bundle B") in a yaml file.

jibran’s picture

Status: Needs work » Needs review

6: 2143291-6.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 6: 2143291-6.patch, failed testing.

Berdir’s picture

See also the findings in #2201051: Convert path.module form alters to a field widget, the settings for base fields are messed up right now, it looks like it's merging, but as you save all bundles, only the first one is apparently being considered, because it just merges the keys together and if the first says FALSE, then no other will make it TRUE.

Not sure how related it is to this issue, apparently not as much as I thought, but still wanted to mention it.

plach’s picture

Sorry, for being vacant, but I have been too busy to work on core in the latest weeks. Hopefully in the next days things should calm down.

Just a quick note on this: I think I can live with @yched's proposal but at least I'd wish to introduce a new method in the core API to set back values so that we don't have to special-case Field API fields in CT.

andypost’s picture

Faced with this in #2136197-80: Move field/instance/widget/formatter settings out of annotation / plugin definition

  • translation support is something that is strictly tied to the storage, in fact the storage controller could use that information to generate the proper schema.
  • translatability is instead something that is tied to runtime status of the field definition (and is the information we need in 99.9% of cases).

My thought on this:
1) core at least needs runtime knowledge of "translatability"
- base fields defines this through "translatable" entity definition (tells about storage) + setTranslatable() for each base field (to properly render widgets/labels)
- configurable instances are translatable by default.
- field types in propertyDefinitions() should mark needed properties as translatable

2) content translation should only affect entities/fields/properties that marked as translatable.
- once entity storage supports storing translation it could be enabled
- once field type storage supports translation then all it's fields with none-calculated properties could be enabled

Anyway C_T stores its settings in own config, so actually only performance reasons about access to this config in runtime should be investigated.

Suppose better to collect the places that needs exactly to know that translation enabled for the piece of info.

Currently in core:
1) entity form/view needs this to fetch proper translation and display language selection (if configured)
2) widgets require this information to display proper labels

EDIT good example is a comment field, by default we do not want to have this enabled per entity translation, but this settings could be overridden by C_T for that purpose

plach’s picture

Priority: Normal » Critical
Issue tags: -content translation +language-content, +D8MI, +sprint

This is blocking #2217543: Cannot translate title of Basic Page node if other content types are not translatable and it would probably be better to at least reach consensus on a solution for this before startng the actual work on #1498720: [meta] Make the entity storage system handle changes in the entity and field schema definitions, hence promoting to critical.

plach’s picture

Status: Needs work » Postponed

#2226197: Introduce FieldStorageDefinitionInterface in the Entity Field API is providing IMO everything we need to fix this, hence postponing on it.

plach’s picture

plach’s picture

Status: Postponed » Active

Now we can properly fix this :)

pwolanin’s picture

Status: Active » Needs review
FileSize
1.66 KB

Here's a test code which I think demonstrates part of the problem - at least in my testing, it's not possible to enable translation of base fields because there is no FieldConfig found for them.

This causes 1 fail for me locally.

Berdir’s picture

Base fields are stored differently and kind of re-applied in content_translation_entity_base_field_info_alter() but the logic there is broken..

Status: Needs review » Needs work

The last submitted patch, 22: 2143291-22.patch, failed testing.

pwolanin’s picture

Yes, I'm having it fail for our new menu link content entity.

I see the problem on line 351 of content_translation.admin.inc

$field = FieldService::fieldInfo()->getField($entity_type, $field_name);

This returns NULL for the fields that are in the base definition of the entity. It's trying to load this config entity:

entity_load('field_config', $entity_type . '.' . $field_name)

But that doesn't exist.

So is this a valid test case that should pass?

plach’s picture

Assigned: Unassigned » plach
Issue tags: +DrupalDays Milano 2014

I will work on this a bit today.

plach’s picture

plach’s picture

My original plan was the following:

  • FieldStorageDefinitionInterface::isTranslatable() tells whether the field supports translation
  • FieldDefinitionInterface::isTranslatable() tells whether a field "instance" has translation enabled

In 99% of cases we need the latter, so most developers won't need to know the difference: in most cases you want to know whether a field instance can hold translations or not, and in that case whether it doesn't because it's not configured for translation or it just doesn't support translation is completely irrelevant. I think this approach is very consistent with the rest of the definition system, where the "default" storage values can be overridden per bundle.

To address the rest of @yched concerns I was planning to move the 'translatable' setting to FieldInstanceConfig and make FieldConfig::isTranslatable() always return TRUE, unless we want to support natively untranslatable configurable fields. CT would alter bundle field definitions based on its settings and obviously configurable fields not supporting translation would not appear in the UI, so would be left alone. This approach would allow configurable field translatability to be configured/deployed regardless of CT, but would allow the latter not to special case configurable fields.

However to implement this plan we need #2225961: Introduce "controlled" setters on FieldDefinitionInterface to avoid special-casing FieldDefinition, unless we just go ahead and introduce FieldDefinitionInterface::setTranslatable() here, regardless of the outcomes we have there.

pwolanin’s picture

Status: Postponed » Active

This is causing a lot of problems trying to support translateable base fields - I'd vote for a quick fix if we can.

YesCT’s picture

Status: Active » Needs work
yched’s picture

move 'translatable' setting to FieldInstanceConfig and make FieldConfig::isTranslatable() always return TRUE, unless we want to support natively untranslatable configurable fields

.
+1, and I don't think we need to support configurable fields that do not allow translatability on the storage level.

CT would alter bundle field definitions based on its settings and obviously configurable fields not supporting translation would not appear in the UI, so would be left alone. This approach would allow configurable field translatability to be configured/deployed regardless of CT, but would allow the latter not to special case configurable fields.

So, is the 'translatable' poperty natively stored in the FieldInstanceConfig CMI record, or is it altered-in by CT ? It's one or the other, having both "it's stored in the FIConfig definition but also stored by CT somewhere else and gets overwritten at alter time" is the exact kind of WTF we want to move away from.

The notion of field translation and support for it at storage and runtime is natively baked in Core/Entity and Core/Field. So I still stand that it's IMO perfectly fine for 'translatable' to be stored nativeley in FIConfig. The job of CT is to present a centralized UI for "translatability", and to store the corresponding config for the fields that have no config records of their own - i.e "non configurable fields".

plach’s picture

@yched:

Sorry for the late reply, I missed your post :(

I don't think we need to support configurable fields that do not allow translatability on the storage level.

I was mainly thinking to use-cases like Commerce: if I understood correctly, the plan for D8 is still relying on configurable fields. In D7 all the code was hardcoding LANGUAGE_NONE everywhere when dealing with prices, so I think in this case it would make sense to be able to declare the field as not supporting translation. OTOH I guess the same effect could be obtained by altering field storage definitions.

It's one or the other, having both "it's stored in the FIConfig definition but also stored by CT somewhere else and gets overwritten at alter time" is the exact kind of WTF we want to move away from.

IMHO this is a WTF only for people with a deep knowledge of the internals of the various subsystems involved here. If we stick to an architectural overview, I see no problem with this approach: we have two distinct subsystems with their own configurations that interact through core interfaces. None of the two needs to know about the existence of the other, nor make any assumption about the underlying implementation. I call this good architecture. OTOH if CT is forced to special-case fields provided by the Field module, we are basically leaving out SOL any other field provider module that may rely on configuration to provide its (bundle) field definitions. Instead with the former approach, the 'translatable' property of FieldInstanceConfig could be populated/stored in config storage only when it needs to be set to TRUE without CT (I'd say a percentage close to 0). I think this would deeply alleviate the WTF you are outlining.

The notion of field translation and support for it at storage and runtime is natively baked in Core/Entity and Core/Field. So I still stand that it's IMO perfectly fine for 'translatable' to be stored nativeley in FIConfig. The job of CT is to present a centralized UI for "translatability", and to store the corresponding config for the fields that have no config records of their own - i.e "non configurable fields".

What is not baked in Core (at least atm) is the notion of configurable field, that is a field having a definition whose values are stored in config. If we had such an interface and Field module just implemented it, I could accept this argument but atm, as I wrote above, it would mean neglecting any provider of bundle field definitions except the Field module.

yched’s picture

IMHO this is a WTF only for people with a deep knowledge of the internals of the various subsystems involved here

I disagree. It will be confusing for whoever looks at config files (e.g looking at diffs when running "config sync", or looking at git diffs when committing config files).
"A setting being saved in two different places in two separate systems" is totally obscure about which actually controls the behavior, and leaves the possibility of getting out of sync, and what happens if they do. Definition of a WTF to me, and something we should really really try to avoid.

we have two distinct subsystems with their own configurations that interact through core interfaces

I have one visible behavior I want to control : "is this field translatable ?". Who is in charge of that, and where is stored ? This can't be the responsibility of two distinct subsystems simultaneously.

if CT is forced to special-case fields provided by the Field module, we are basically leaving out SOL any other field provider module that may rely on configuration to provide its (bundle) field definitions

Feels a bit hypothetical to me, but OK :-). Then we need to formalize and architecture around this possibility : there are field definitions that can store their "translatability" setting themselves, and others that don't.
Something like a method, or a marker interface, that lets CT know it shouldn't store a translatability flag itself, without hardcoding that behavior on core's field.module's "configurable fields" - which is basically your last point in #32 ?

andypost’s picture

Hm, suppose field "translatability" defands on entity, so in case of entity has no langcode field there's no way to translate fields as well

alexpott’s picture

#2224761: Add a generic way to add third party configuration on configuration entities and implement for field configuration is proposing a new config entity to track extra settings for field (both base and configurable) potentially that unlocks a cleaner solution that suggestion in the summary.

@yched and other field maintainers - does translatable on a field actually have any meaning outside the content_translation module?

yched’s picture

does translatable on a field actually have any meaning outside the content_translation module

Well, the notion of "entity and fields can be translatable" is deeply enrooted in Core's (Content)Entity and Field APIs and storage classes, so in that sense the answer is yes.

FieldDefinitionInterface has an isTranslatable() method, implemented in the various subclasses in Core\Field (base fields) and field.module (configurable fields), and those implementations have to return a value based on what they know from where they stand (Core\Field doesn't know about content_translation.module).

In the approach you propose, isTranslatable() would return a value based on some key in the DumpingGroundFor3rdPartyConfigAboutAField config entity introduced by #2224761: Add a generic way to add third party configuration on configuration entities and implement for field configuration - which would be something Core\Field knows about, even though it doesn't know exactly everything that's inside.

Still a bit nervous on the idea of "the configuration of non-configurable fields" and what goes there exactly, but I guess you're right, if #2224761: Add a generic way to add third party configuration on configuration entities and implement for field configuration happens, then it should also be used for 'translatable'.

plach’s picture

Status: Needs work » Needs review
FileSize
38.45 KB

Here's what I have for now. Let's see what the bot thinks about it.

Status: Needs review » Needs work

The last submitted patch, 37: field_translatability-2143291-37.patch, failed testing.

plach’s picture

Status: Needs work » Needs review
FileSize
5.8 KB
43.31 KB

Let's try again

alexpott’s picture

plach’s picture

After discussing this with @yched, @fago, @alexpott, @xjm and @mtift we agreed that for this to actually work we should not be able to pass field definition as storage definition, otherwise per-bundle overrides might be misinterpreted as non-overriden base values. For translatability this would mean that a field not enabled for translation on a certain bundle could be interpreted as a field not supporting translation.

Status: Needs review » Needs work

The last submitted patch, 41: field_translatability-2143291-41.patch, failed testing.

plach’s picture

Another try

plach’s picture

Status: Needs work » Needs review
FileSize
89.13 KB

Status: Needs review » Needs work

The last submitted patch, 44: field_translatability-2143291-43.patch, failed testing.

plach’s picture

Status: Needs work » Needs review
FileSize
3.36 KB
92.02 KB

This should be green again

fago’s picture

Issue summary: View changes

Thanks! I reviewed it, in particular the entity field api changes. Cannot really say much about the content translation changes.

  1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityDatabaseStorage.php
    @@ -951,8 +950,10 @@ protected function doLoadFieldItems($entities, $age) {
    +    $definitions = array();
    ...
    +      $definitions[$bundle] = $this->entityManager->getFieldDefinitions($this->entityTypeId, $bundle);
    

    Not sure why it builds this $definitions array up here, is this related?

  2. +++ b/core/modules/content_translation/content_translation.admin.inc
    @@ -79,7 +79,17 @@ function _content_translation_form_language_content_settings_form_alter(array &$
    +    // @todo Remove the try/catch block as soon as also Menu Links are converted
    +    //   to the Entity Field API. See https://drupal.org/node/2256521.
    

    Why does this happen for menu links only? Maybe check for having a content entity type?

  3. +++ b/core/modules/content_translation/content_translation.module
    @@ -145,22 +142,21 @@ function content_translation_entity_bundle_info_alter(&$bundles) {
    +    // @todo $base_field_definitions should probably passed also to the alter
    +    //   hook.
    

    They have their own alter. Where's the problem with that?

  4. StorableFieldDefinitionInterface

    As discussed, let's postpone introducing this to the patch which introduces the base override config entities as so far there is no use for it, i.e. you can just check for FieldInstanceConfigInterface.

plach’s picture

This should fix #47: I just removed the @todo about Menu links as it's just matter of checking whether the entity types exposes fields. Translation support is checked earlier.

Not sure why it builds this $definitions array up here, is this related?

Yep, it's used below to check whether the field instance is translatable.

YesCT’s picture

I'm reading through this.
(Probably will still need a read by fago or someone else.)

fago’s picture

Thanks.

This should fix #47: I just removed the @todo about Menu links as it's just matter of checking whether the entity types exposes fields.

Why doesn't it just check for being an content entity then? Seems to be better than trying and catching exceptions later on.

+++ b/core/lib/Drupal/Core/Entity/Schema/ContentEntitySchemaHandler.php
@@ -84,8 +84,10 @@ public function getSchema() {
-          $this->addFieldSchema($schema[$table_name], $field_name, $column_names);
+          if (isset($this->fieldStorageDefinitions[$field_name])) {

Not sure why this is skipped when it's not there. If the table mapping is incorrect, it should throw an exception I suppose? If it may not happen, we need not check it.

YesCT’s picture

I read through the issue summary. I have not finished reading the patch yet.

This issue is great and is resolving significant confusion I had when this stuff first went in. So super!

This bit in the issue summary confused me:

This also lets you turn translatability on an instance directly without requiring you to enable c_t and mess with its settings.

Is that saying:
This also lets you support translatability on an instance directly without requiring you to enable c_t and mess with its settings.
?

plach’s picture

Removed the try/catch block in favor of the content entity type check. I also removed the if mentioned in #50 because I cannot recall why that was failing. The bot will tell us.

@YesCT:

Yes, however the issue summary could use an update, I will try to work on it when I get home.

Gábor Hojtsy’s picture

Issue summary: View changes

Reformatted the summary to be better readable, according to how I understand the structure at least. Could not find the referenced content_translation_entity_field_info_alter() anymore, not sure where that code is now? Hope this helps @plach.

plach’s picture

Assigned: plach » yched
Issue summary: View changes

The issue summary was still not accurate, sorry :)
(mainly the proposed solution)

Assigning to @yched for a final sign-off.

plach’s picture

Issue summary: View changes
plach’s picture

Issue summary: View changes
effulgentsia’s picture

Straight reroll.

effulgentsia’s picture

+++ b/core/lib/Drupal/Core/Field/FieldStorageDefinitionInterface.php
@@ -85,6 +85,16 @@ public function getSetting($setting_name);
   /**
+   * Sets whether the field is translatable.
+   *
+   * @param bool $translatable
+   *   Whether the field is translatable.
+   *
+   * @return $this
+   */
+  public function setTranslatable($translatable);

Was this added to the correct interface? Seems like it should be either on FieldDefinitionInterface or on both, but not only on this one. If the answer is it should be on both, why? I get the use case of CT needing to set on FieldDefinitionInterface, but what's the use case of needing to set on FieldStorageDefinitionInterface?

Status: Needs review » Needs work

The last submitted patch, 57: field_translatability-2143291-57.patch, failed testing.

Gábor Hojtsy’s picture

@effulgentsia: I don't know the specifics but suspect that would be for automated schema generation?

effulgentsia’s picture

@Gábor: automated schema generation needs to call isTranslatable(), but not setTranslatable(). #28 mentions we need a setTranslatable() on FieldDefinitionInterface, but makes no mention of needing it on FieldStorageDefinitionInterface. So perhaps the patch just added it to the wrong file? However, from the issue summary:

Configurable fields always support translation by default. This can be changed by altering their storage deifnition if a field (e.g. the Commerce Price) should never be enabled for translation.

Is it only configurable fields that Commerce should be able to alter in this way? If so, then setTranslatable() only needs to be on FieldConfig, and not on FieldStorageDefinitionInterface. Otherwise, perhaps it does need to be on FieldStorageDefinitionInterface.

effulgentsia’s picture

I read through the entire patch, and overall, like it a lot. Besides #58/#61, here's my only other substantive feedback. I'll follow up with a separate comment for nits.

  1. +++ b/core/modules/content_translation/content_translation.module
    @@ -638,56 +632,35 @@ function content_translation_entity_extra_field_info() {
     /**
      * Implements hook_form_FORM_ID_alter() for 'field_ui_field_edit_form'.
      */
    -function content_translation_form_field_ui_field_edit_form_alter(array &$form, array &$form_state, $form_id) {
    +function content_translation_form_field_ui_field_instance_edit_form_alter(array &$form, array &$form_state) {
    

    a) The PHPDoc needs to be updated.
    b) The issue summary says no UI change, but this looks like a UI change.

  2. +++ b/core/modules/field_ui/src/FieldOverview.php
    @@ -210,7 +210,7 @@ public function buildForm(array $form, array &$form_state, $entity_type_id = NUL
             // contrib modules can form_alter() the value for newly created fields.
             'translatable' => array(
               '#type' => 'value',
    -          '#value' => FALSE,
    +          '#value' => TRUE,
    

    So, looks like the patch still leaves FieldConfig::translatable around in the above, in field.field schema, and in FieldConfig::toArray(). Are we sure we need/want that? Wouldn't it make more sense to keep 'translatable' out of field.field YAML entirely, and have use cases like Commerce implement an alter on create/load rather than via form_alter?

effulgentsia’s picture

And the nits:

  1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityDatabaseStorage.php
    @@ -951,8 +950,10 @@ protected function doLoadFieldItems($entities, $age) {
         // Collect impacted fields.
         $fields = array();
    +    $definitions = array();
         foreach ($bundles as $bundle => $v) {
    -      foreach ($this->entityManager->getFieldDefinitions($this->entityTypeId, $bundle) as $field_name => $instance) {
    +      $definitions[$bundle] = $this->entityManager->getFieldDefinitions($this->entityTypeId, $bundle);
    +      foreach ($definitions[$bundle] as $field_name => $instance) {
             if ($instance instanceof FieldInstanceConfigInterface) {
               $fields[$field_name] = $instance->getField();
    

    The local variable names are getting confusing here. Would it make sense to rename:
    $fields => $storage_definitions
    $definitions => $field_definitions
    $instance => $field_definition

    Or similar?

  2. +++ b/core/modules/comment/src/Entity/Comment.php
    @@ -294,7 +294,8 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
         $fields['field_name'] = FieldDefinition::create('string')
           ->setLabel(t('Comment field name'))
           ->setDescription(t('The field name through which this comment was added.'))
    -      ->setComputed(TRUE);
    +      ->setComputed(TRUE)
    +      ->setCustomStorage(TRUE);
    

    So based on above, "custom storage" could mean either "I have storage and I'm managing it myself" or "I have no storage". Is that ok? Seems like it could be, but not sure, so asking.

  3. +++ b/core/modules/content_translation/content_translation.admin.inc
    @@ -98,44 +100,34 @@ function _content_translation_form_language_content_settings_form_alter(array &$
    -            // @todo Remove this special casing as soon as configurable and
    ...
    +              // Display the column translatability configuration widget.
    +              if ($definition instanceof FieldInstanceConfigInterface) {
    +                $column_element = content_translation_field_sync_widget($definition);
    

    Looks like we still have special casing for configurable, so can we retain the @todo, just move it down to where it applies? Bonus points if we can also update the comment to explain why the sync widget is only for configurable fields, and link to an issue.

  4. +++ b/core/modules/content_translation/content_translation.admin.inc
    @@ -98,44 +100,34 @@ function _content_translation_form_language_content_settings_form_alter(array &$
    +              $translatable = $definition instanceof FieldInstanceConfigInterface ? $definition->isTranslatable() : !empty($field_settings[$field_name]);
    +              // If we have a storable field definition we use the stored value
    +              // instead of our settings to determine translatability.
    

    Does this comment apply to the line above it? If not, then I don't understand it. If so, let's move it up.

  5. +++ b/core/modules/field/src/Plugin/views/field/Field.php
    @@ -902,8 +902,8 @@ function field_langcode(EntityInterface $entity) {
    -        array($this->languageManager->getCurrentLanguage(Language::TYPE_CONTENT), $default_langcode),
    -        $this->view->display_handler->options['field_language']
    +        array($this->languageManager->getCurrentLanguage(Language::TYPE_CONTENT)->id, $default_langcode),
    +        $this->view->display_handler->options['field_langcode']
    ...
    +++ b/core/modules/views/src/Plugin/views/display/DisplayPluginBase.php
    @@ -180,11 +180,11 @@ public function initDisplay(ViewExecutable $view, array &$display, array &$optio
    -    // Convert the field_language and field_language_add_to_query settings.
    -    $field_language = $this->getOption('field_language');
    +    // Convert the field_langcode and field_language_add_to_query settings.
    +    $field_langcode = $this->getOption('field_langcode');
    

    Looks like a good change, but how it is relevant to this issue?

plach’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
5.38 KB
91.46 KB

So, looks like the patch still leaves FieldConfig::translatable around in the above, in field.field schema, and in FieldConfig::toArray(). Are we sure we need/want that?

We need it in the ::toArray() return value otherwise it won't be serialized/cached and alteration won't be possible. Since we have it there it will be stored in configuration so I thought it would be better to leave it in the schema too.

The local variable names are getting confusing here. Would it make sense to rename:

It definitely would, but the entity cache patch is touching the very same code so I'd tend to avoid too much conflicts. Moreover it looks like actual variable names deserve their own bikeshedding ;)

So based on above, "custom storage" could mean either "I have storage and I'm managing it myself" or "I have no storage". Is that ok? Seems like it could be, but not sure, so asking.

@fago is planning to automate this in another issue (I don't remember which one) so FieldDefinition will always set custom_storage to TRUE internally if the definition is marked as custom (which is a FieldDefinitionInterface property only). We agreed this is a sufficient fix for now.

Looks like a good change, but how it is relevant to this issue?

That code is broken in HEAD, it seems some change here is exposing that bug as tests are now failing. I had to fix it to make tests pass.

plach’s picture

Issue summary: View changes

Fixed the issue summary

yched’s picture

As a first remark:

FieldDefinitionInterface no more extends FieldStorageDefinitionInterface
Yay, this means FieldDefinitionInterface s/"is a"/"has a"/ FieldStorageDefinitionInterface. Sanity++

But this has a couple consequences :

1. we then need an API method to "get the (separate) FSDI object for a given FDI object", or #2144263: Decouple entity field storage from configurable fields is blocked (getting the FSDI for a given FDI is what that other issue is entirely about).

Currently, that logic is different depending on what kind of $field_definition FDI you have :
- if $field_definition is a FieldDefinition, the FSDI is $field_definition itself
- if $field_definition is a FieldInstanceConfig, the FSDI is $field_definition->getField() (from FieldInstanceConfigInterface)

So we need to:
- add FDI::getFieldStorageDefinition()
- implement it in FieldDefinition (just returns $this)
- implement it in FieldInstanceConfig (it is just the current getField() method, renamed)
- remove FieldInstanceConfigInterface::getField(), and replace all existing calls.

We could say that this missing API would be left to #2144263: Decouple entity field storage from configurable fields to add, but strictly speaking this belongs to the same patch that removes "$field_definition instanceof FSDI" IMO. In short : if we replace "is a" by "has a", we need to have an API for this "has a".

2. since FieldInstanceConfig doesn't implement FSDI anymore, there are a couple methods that we could / should remove from the class.
At least, FieldInstanceConfig has methods that are still documented as @{inheritdoc}, and for which this is now a lie since they actually don't implement anything from upstream anymore.

yched’s picture

And here's my review for the rest of the patch.

  1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityDatabaseStorage.php
    @@ -236,11 +235,11 @@ protected function schemaHandler() {
    -        return !$definition->isComputed() && !$definition->hasCustomStorage() && !$definition->isMultiple();
    +        return !$definition->hasCustomStorage() && !$definition->isMultiple();
    

    (+ the related additions of explicit setCustomStorage(TRUE) for the current computed fields in core)
    I don't get how this is related to the task here ?

  2. +++ b/core/lib/Drupal/Core/Entity/ContentEntityDatabaseStorage.php
    @@ -953,8 +952,10 @@ protected function doLoadFieldItems($entities, $age) {
    +      $definitions[$bundle] = $this->entityManager->getFieldDefinitions($this->entityTypeId, $bundle);
    

    Same remark as @effulgentsia above about var names in there, but I'm fine with cleaning that up in #2144263: Decouple entity field storage from configurable fields or other.
    (will really need to happen though, the current is really confusing)

  3. +++ b/core/lib/Drupal/Core/Field/FieldStorageDefinitionInterface.php
    @@ -77,14 +77,24 @@ public function getSettings();
    +   * Sets whether the field is supports translation.
    

    extraneous "is"

  4. +++ b/core/modules/content_translation/content_translation.admin.inc
    @@ -300,68 +297,38 @@ function content_translation_form_language_content_settings_submit(array $form,
    +          // If we have a storable field definition we directly persist any
    +          // change to translatability, otherwise we store changes in our config
    +          // so we can properly alter field definitions later.
    

    Nitpick : would need the same @todo comment as in _content_translation_form_language_content_settings_form_alter() above ?

  5. +++ b/core/modules/content_translation/content_translation.admin.inc
    @@ -300,68 +297,38 @@ function content_translation_form_language_content_settings_submit(array $form,
    +  foreach ($settings as $entity_type_id => &$entity_settings) {
    +    foreach ($entity_settings as $bundle => &$bundle_settings) {
    +          $definition = $definitions[$field_name];
    +          if ($definition instanceof FieldInstanceConfigInterface) {
    +            [... update and ->save() the FieldInstanceConfig ...]
    +          }
    +          else {
    +            $bundle_settings['fields'][$field_name] = $translatable;
               }
    

    I might be reading the code wrong, but since $bundle_settings['fields'] already contains the submitted form values, and is what gets ultimately saved in content_translation_save_settings(), I'd expect an unset() in the "if FieldInstanceConfig" code branch, rather the "else" branch doing a set of something that looks like it's already there ?
    So it seems the current code still saves the translatabilty of configurable instances in c_t's own settings, in addition to within the FieldIstanceConfig record ?

  6. +++ b/core/modules/content_translation/content_translation.module
    @@ -145,22 +142,19 @@ function content_translation_entity_bundle_info_alter(&$bundles) {
    +    $base_field_definitions = \Drupal::entityManager()->getBaseFieldDefinitions($entity_type->id());
    +    foreach ($settings as $name => $translatable) {
    +      // Skip storable field definitions as those are supposed to have already
    +      // been changed.
    +      if (isset($base_field_definitions[$name]) && !$base_field_definitions[$name] instanceof FieldInstanceConfigInterface) {
    +        if (!isset($fields[$name])) {
    +          $fields[$name] = clone $base_field_definitions[$name];
    +        }
    

    Not sure I get this code here.

    We reason on and restrict to BaseFieldDefinitions ?
    What about code-defined (bundle)FieldDefinitions ? I'd think we want to iterate on "bundle fields that are not FieldInstanceConfig" ? (i.e based on the $fields we receive as an argument rather than on getBaseFieldDefinitions())
    The "not instanceof FieldInstanceConfigInterface" check is currently meaningless, a FieldInstanceConfig never shows up in getBaseFieldDefinitions(), right ?

  7. +++ b/core/modules/field/tests/modules/field_test_config/config/install/field.field.entity_test.field_test_import.yml
    @@ -8,7 +8,6 @@ settings:
    -translatable: false
    

    Not thrilled either about keeping 'translatable' in FC, but I get the use case. If so, and if it stays in the config schema and gets saved on FC::save(), then it's an official part of FC, a FieldConfig remains "translatable or not", and there's no point in removing the 'translatable' entry in the various field.field.*.yml present in core ?

    field.field.entity_test.field_test_import.yml
    field.field.entity_test.field_test_import_2.yml
    field.field.taxonomy_term.forum_container.yml
    field.field.node.field_image.yml
    field.field.node.field_tags.yml
    field.field.user.user_picture.yml

  8. +++ b/core/modules/system/src/Tests/Ajax/MultiFormTest.php
    @@ -6,8 +6,7 @@
     namespace Drupal\system\Tests\Ajax;
    -
    -use Drupal\Core\Field\FieldDefinitionInterface;
    +use Drupal\Core\Field\FieldStorageDefinitionInterface;
    

    Nitpick : patch removes the empty line between "namespace" and "use" statements in a bunch of files.
    I think that empty line is part of our standards ?

  9. +++ b/core/modules/user/src/Entity/User.php
    @@ -530,7 +530,7 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
         $fields['roles'] = FieldDefinition::create('string')
           ->setCustomStorage(TRUE)
           ->setLabel(t('Roles'))
    -      ->setCardinality(FieldDefinitionInterface::CARDINALITY_UNLIMITED)
    +      ->setCardinality(FieldStorageDefinitionInterface::CARDINALITY_UNLIMITED)
    

    Right, of course - but having to use a constant from FieldStorageDefinitionInterface while creating a FieldDefinition is a bit awkward DX wise.

    As a mitigation, we could add a setCardinalityUnlimited() method ?

effulgentsia’s picture

Re #67.1: because FSDI doesn't have an isComputed() method. See also #64.3.

yched’s picture

@effulgentsia: oh, right.

Yeah, not sure whether we still need isComputed() once we have setCustomStorage(TRUE) ?
(outside the scope of this issue, but curious about that - I get the notion of a "computed field property", but the notion of "computed *field*" always puzzled me a bit)

plach’s picture

Thanks for the reviews, I will try to address them in the weekend, although I might not have time. If not will do monday night.

plach’s picture

FileSize
0 bytes

Just a reroll for now.
(sorry too many conflicts)

plach’s picture

FileSize
90.61 KB

for reals now

Status: Needs review » Needs work

The last submitted patch, 72: field_translatability-2143291-71.patch, failed testing.

xjm’s picture

I think this is breaking following #2228763: Create a comment-type config entity and use that as comment bundles, require selection in field settings form maybe? -- the merge doesn't look quite right. Attached reverts the changes to Comment which resolves some test fatals for me locally. Also pushed this to the sandbox in my own branch.

xjm’s picture

Status: Needs work » Needs review

meep

Status: Needs review » Needs work

The last submitted patch, 74: 2143291-74.patch, failed testing.

xjm’s picture

Assigned: yched » xjm

The remaining PHP fatal in ContentTranslationSettingsTest is also related to #2228763: Create a comment-type config entity and use that as comment bundles, require selection in field settings form -- fixing that too.

alexpott’s picture

Status: Needs work » Needs review
FileSize
1.24 KB
89.93 KB

Fixed ContentTranslationSettingsTest. Working on #66 and #67

alexpott’s picture

FileSize
23.2 KB
112.02 KB

Done what @yched suggests in #76. Let's see what fails. FieldDefinitionInterface also has getType() and getName() since if FieldInstanceConfig didn't implement them the change would be huge AND both of these values are saved to the field instance config file so it makes sense.

Status: Needs review » Needs work

The last submitted patch, 79: 2143291.79.patch, failed testing.

xjm’s picture

Issue summary: View changes
xjm’s picture

Assigned: xjm » alexpott
alexpott’s picture

Assigned: alexpott » xjm
Issue summary: View changes
Status: Needs work » Needs review
FileSize
112.07 KB

rerolled... the drop is always moving :)

Status: Needs review » Needs work

The last submitted patch, 83: 2143291.81.patch, failed testing.

alexpott’s picture

Assigned: xjm » alexpott
Status: Needs work » Needs review
FileSize
3.22 KB
115.78 KB

Still very likely to fail but it will get past standard install.

Status: Needs review » Needs work

The last submitted patch, 85: 2143291.85.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
6.38 KB
122.57 KB

Okay this should resolve the fallout of removing FSDI from FieldInstanceConfigInterface. Now onto #67

Just wondering out loud are we sure that we don't want FieldInstanceConfigInterface to implement FSDI?

alexpott’s picture

FileSize
7.73 KB
119.7 KB

Patch addresses #67:

  1. Answered by eff
  2. Lets leave this to the other issue or a followup to both
  3. Fixed
  4. Fixed
  5. Fixed (and added test)
  6. Simplified and fixed (i think)
  7. Fixed
  8. Fixed
  9. We can just use FieldDefinition::CARDINALITY_UNLIMITED
yched’s picture

Regarding point 9 : the code example I gave in #67 was just one occurrence, there are other similar places in core.

I was not aware that you could define a constant on an interface and refer to it through a concrete subclass, but yeah, why not I guess.

yched’s picture

Regarding point 5, _content_translation_update_field_translatability() :
I still have the feeling that the

+ else { 
+   $bundle_settings['fields'][$field_name ] = $translatable; 
+ }

part is not needed because the value we set is already there, but I might be wrong (phone review...)

plach’s picture

Assigned: alexpott » plach

I reviewed @alexpott's changes and most of them look good to me. I am working on a couple of fixes and I will have a look to #89 and #90.

plach’s picture

This should address #89 (I found only one other occurrence actually), #90 and fix #67.6, which was not actually correct in its latest form. In fact we were no longer instantiating bundle field definitions when they were missing.
Attached you can find a patch with only the improved tests + #88 that shows the failures.

plach’s picture

Re-uploading the broken patch (which is expected to fail tests).

The last submitted patch, 92: field_translatability-2143291-92.patch, failed testing.

xjm’s picture

Is the API change section for this issue up to date?

Can we check for any existing change records that might need an update?

plach’s picture

This should fix path language tests.

@xjm:

The API changes section is up-to-date, I think. It's not very descriptive, though. We probably need to update the change record where storage definitions were announced.

yched’s picture

Thanks ! No more remarks on my side.

@xjm I'll let you RTBC when summary and change notice are sorted out ?

alexpott’s picture

Issue summary: View changes
alexpott’s picture

I'm not sure that this issue needs a change notice of its own per se. I think an update to https://www.drupal.org/node/2236285 should suffice.

xjm’s picture

Issue summary: View changes

I think a short announcement that translatability is now per-instance rather than per-field is called for, no?

  • Per api.d.o, these are the members FieldDefinitionInterface inherits from FieldStorageDefinition in HEAD:
    DataDefinitionInterface::createFromDataType
    DataDefinitionInterface::getClass
    DataDefinitionInterface::getDataType
    DataDefinitionInterface::isComputed
    DataDefinitionInterface::isList
    DataDefinitionInterface::isReadOnly
    FieldStorageDefinitionInterface::CARDINALITY_UNLIMITED
    FieldStorageDefinitionInterface::getCardinality
    FieldStorageDefinitionInterface::getColumns
    FieldStorageDefinitionInterface::getConstraint
    FieldStorageDefinitionInterface::getConstraints
    FieldStorageDefinitionInterface::getDescription
    FieldStorageDefinitionInterface::getLabel
    FieldStorageDefinitionInterface::getMainPropertyName
    FieldStorageDefinitionInterface::getName
    FieldStorageDefinitionInterface::getPropertyDefinition
    FieldStorageDefinitionInterface::getPropertyDefinitions
    FieldStorageDefinitionInterface::getPropertyNames
    FieldStorageDefinitionInterface::getProvider
    FieldStorageDefinitionInterface::getSchema
    FieldStorageDefinitionInterface::getSetting
    FieldStorageDefinitionInterface::getSettings
    FieldStorageDefinitionInterface::getTargetEntityTypeId
    FieldStorageDefinitionInterface::getType
    FieldStorageDefinitionInterface::hasCustomStorage
    FieldStorageDefinitionInterface::isMultiple
    FieldStorageDefinitionInterface::isQueryable
    FieldStorageDefinitionInterface::isRevisionable
    FieldStorageDefinitionInterface::isTranslatable
    
  • These are the members we're adding in the patch:
    +++ b/core/lib/Drupal/Core/Field/FieldDefinitionInterface.php
    @@ -52,7 +52,28 @@
    +  public function getName();
    ...
    +  public function getType();
    
    @@ -131,4 +152,29 @@ public function isRequired();
    +  public function isTranslatable();
    ...
    +  public function setTranslatable($translatable);
    ...
    +  public function getFieldStorageDefinition();
    
  • We're also renaming this method (so no, the API changes section was not complete). :)
    +++ b/core/modules/field/src/FieldInstanceConfigInterface.php
    @@ -21,7 +21,7 @@
    -  public function getField();
    +  public function getFieldStorageDefinition();
    

Couple other questions/observations:

  1. +++ b/core/lib/Drupal/Core/Field/FieldDefinition.php
    @@ -15,7 +15,7 @@
    -class FieldDefinition extends ListDataDefinition implements FieldDefinitionInterface {
    +class FieldDefinition extends ListDataDefinition implements FieldDefinitionInterface, FieldStorageDefinitionInterface {
    
    +++ b/core/modules/editor/src/Plugin/InPlaceEditor/Editor.php
    @@ -29,7 +29,7 @@ public function isCompatible(FieldItemListInterface $items) {
    -    if ($field_definition->getCardinality() != 1) {
    +    if ($field_definition->getFieldStorageDefinition()->getCardinality() != 1) {
    

    So FieldDefinition itself actually implements both interfaces. So if we have a FieldDefinition object here, why do we need to get the field storage definition first? The docs say it's a FieldItemListInterface; is $field_definition just a not-quite-correct variable name? Or?

  2. +++ b/core/modules/field/src/Entity/FieldConfig.php
    @@ -111,11 +111,11 @@ class FieldConfig extends ConfigEntityBase implements FieldConfigInterface {
    -  public $translatable = FALSE;
    +  public $translatable = TRUE;
    

    We also seem to be changing this default.

  3. +++ b/core/modules/field/src/Entity/FieldInstanceConfig.php
    +++ b/core/modules/field/src/Entity/FieldInstanceConfig.php
    @@ -121,6 +121,15 @@ class FieldInstanceConfig extends ConfigEntityBase implements FieldInstanceConfi
    

    BTW, all the red in this base class underscores for me that this is a good change.

xjm’s picture

Issue summary: View changes

xpost

alexpott’s picture

I think a short announcement that translatability is now per-instance rather than per-field is called for, no?

I think not since content_translation has always been making it look like it works this way.

The API section was more complete :) as it is again :)

  1. So Drupal\editor\Plugin\InPlaceEditor\Editor is working both with base fields and configurable fields. getFieldDefinition() only guarantees you FieldDefinitionInterface so you need to get the storage definition to work with its methods.
  2. Yep because configurable fields by default support translation.
  3. Yep we are one step closer to making fields / instances / base fields be a bit more grokable

I'm searching for more affected CRs

alexpott’s picture

I'm going through all these CRs https://www.drupal.org/list-changes/published/drupal?keywords_descriptio... to check if they need updating for FSDI or this issue.

alexpott’s picture

I've updated those CR's for the FSDI change and think we need to update these once this is in.

xjm’s picture

@alexpott and I both find it a little off for FieldDefinition to implement both interfaces. @alexpott pointed out correctly that changing that would be out of scope here, but let's reference a followup discussion? Also adding related issue.

xjm’s picture

Also, re: #104 and #99. One way to provide updated change records is to make a copy of the existing change record in a draft state. Then when the patch is committed, we update the existing CR with the draft's content and toggle its published field to make it go to the top of the list. Poor man's draft revisions. :) It's unnecessary for a small string replace I think, but helpful if we need to do a major rewrite so that we don't add to the beta blocker count with missing change record updates.

Edit: So I guess for https://www.drupal.org/node/2111871 and https://www.drupal.org/node/2236285.

alexpott’s picture

Actually https://www.drupal.org/node/2111871 can be done before this issue lands - which I've done. And I've created https://www.drupal.org/node/2289119 to work on updating https://www.drupal.org/node/2236285.

xjm’s picture

Status: Needs review » Reviewed & tested by the community

@alexpott and I worked some more on the change record, and @Berdir gave it a read and said it looked good. So, RTBCing on behalf of @yched. ;)

catch’s picture

Status: Reviewed & tested by the community » Fixed

Looked over this, the main niggles are covered by the other critical task, so committed/pushed to 8.x, thanks!

  • Commit 86e432e on 8.x by catch:
    Issue #2143291 by plach, alexpott, xjm, effulgentsia, pwolanin, swentel...
xjm’s picture

I've made the change record updates @alexpott listed and published the updated version of https://www.drupal.org/node/2236285.

xjm’s picture

...Hm. Actually https://www.drupal.org/node/2090627 no longer bears any resemblance to the current codebase, and CR overall is also completely misleading now.

YesCT’s picture

xjm’s picture

Alright, fixed that CR with @Berdir's help. All done here!

Status: Fixed » Closed (fixed)

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

Gábor Hojtsy’s picture

Issue tags: -sprint