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 isTRUE
orFALSE
. 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 extendFieldStorageDefinitionInterface
.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 aFieldBundleOverrideInterface
which will extend onlyFieldDefinitionInterface
. - 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 solutionWrite a patchFix 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 extendsFieldStorageDefinitionInterface
FieldInstanceConfig::getField()
is renamed toFieldInstanceConfig::getFieldStorageDefinition()
FieldInstanceConfig
has a new propertytranslatable
FieldInstanceConfigInterface
therefore no longer extendsFieldStorageDefinitionInterface
which meansFieldInstanceConfig
no longer implementsgetProvider()
,isRevisionable()
,getCardinality()
,isMultiple()
,getTargetEntityTypeId()
,isQueryable()
,getPropertyDefinition()
,getPropertyDefinitions()
,getPropertyNames()
,getMainPropertyName()
,getSchema()
,getColumns()
,hasCustomStorage()
- now to access these methods useFieldInstanceConfig::getFieldStorageDefinition()->METHOD()
Comment | File | Size | Author |
---|---|---|---|
#96 | field_translatability-2143291-96.patch | 123.54 KB | plach |
#92 | field_translatability-2143291-92.patch | 121.35 KB | plach |
Comments
Comment #1
yched CreditAttribution: yched commentedComment #2
swentel CreditAttribution: swentel commentedHmm, so you want to calculate this on the fly during hook_field_entity_load() ?
Comment #3
yched CreditAttribution: yched commentedcontent_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...
Comment #4
swentel CreditAttribution: swentel commentedWell, 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)
Comment #5
yched CreditAttribution: yched commentedMmh. 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 ?
Comment #6
swentel CreditAttribution: swentel commentedCurious how this will behave
Comment #8
plach@#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.Comment #9
yched CreditAttribution: yched commentedWell, 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 ?
Comment #10
yched CreditAttribution: yched commentedAs 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
Comment #11
plachI read again all the discussion at #2114707: Allow per-bundle overrides of field definitions before getting back to this. Here are my current thoughts:
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:
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:
translatable
property set toTRUE
in their field storage definition.translatable
setting goes away.translatable
property toTRUE
wherever that makes sense for them. For instancenid
would beFALSE
,title
would beTRUE
).hook_entity_field_info_alter()
to alter just field definitions based on its configuration.translatable
property set toTRUE
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.Comment #12
yched CreditAttribution: yched commentedI 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.
Comment #13
jibran6: 2143291-6.patch queued for re-testing.
Comment #15
BerdirSee 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.
Comment #16
plachSorry, 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.
Comment #17
andypostFaced with this in #2136197-80: Move field/instance/widget/formatter settings out of annotation / plugin definition
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 translatable2) 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
Comment #18
plachThis 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.
Comment #19
plach#2226197: Introduce FieldStorageDefinitionInterface in the Entity Field API is providing IMO everything we need to fix this, hence postponing on it.
Comment #20
plachComment #21
plachNow we can properly fix this :)
Comment #22
pwolanin CreditAttribution: pwolanin commentedHere'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.
Comment #23
BerdirBase fields are stored differently and kind of re-applied in content_translation_entity_base_field_info_alter() but the logic there is broken..
Comment #25
pwolanin CreditAttribution: pwolanin commentedYes, I'm having it fail for our new menu link content entity.
I see the problem on line 351 of content_translation.admin.inc
This returns NULL for the fields that are in the base definition of the entity. It's trying to load this config entity:
But that doesn't exist.
So is this a valid test case that should pass?
Comment #26
plachI will work on this a bit today.
Comment #27
plachTo fix this properly we need a
setTranslatable
method, that is #2225961: Introduce "controlled" setters on FieldDefinitionInterface to avoid special-casing FieldDefinition.Comment #28
plachMy original plan was the following:
FieldStorageDefinitionInterface::isTranslatable()
tells whether the field supports translationFieldDefinitionInterface::isTranslatable()
tells whether a field "instance" has translation enabledIn 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 toFieldInstanceConfig
and makeFieldConfig::isTranslatable()
always returnTRUE
, 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.Comment #29
pwolanin CreditAttribution: pwolanin commentedThis is causing a lot of problems trying to support translateable base fields - I'd vote for a quick fix if we can.
Comment #30
YesCT CreditAttribution: YesCT commentedComment #31
yched CreditAttribution: yched commented.
+1, and I don't think we need to support configurable fields that do not allow translatability on the storage level.
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".
Comment #32
plach@yched:
Sorry for the late reply, I missed your post :(
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.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 ofFieldInstanceConfig
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.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.
Comment #33
yched CreditAttribution: yched commentedI 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.
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.
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 ?
Comment #34
andypostHm, suppose field "translatability" defands on entity, so in case of entity has no langcode field there's no way to translate fields as well
Comment #35
alexpott#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?
Comment #36
yched CreditAttribution: yched commentedWell, 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'.
Comment #37
plachHere's what I have for now. Let's see what the bot thinks about it.
Comment #39
plachLet's try again
Comment #40
alexpott#2224761: Add a generic way to add third party configuration on configuration entities and implement for field configuration is now postponed on this. So this is a critical beta blocker.
Comment #41
plachAfter 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.
Comment #43
plachAnother try
Comment #44
plachComment #46
plachThis should be green again
Comment #47
fagoThanks! I reviewed it, in particular the entity field api changes. Cannot really say much about the content translation changes.
Not sure why it builds this $definitions array up here, is this related?
Why does this happen for menu links only? Maybe check for having a content entity type?
They have their own alter. Where's the problem with that?
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.
Comment #48
plachThis 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.
Yep, it's used below to check whether the field instance is translatable.
Comment #49
YesCT CreditAttribution: YesCT commentedI'm reading through this.
(Probably will still need a read by fago or someone else.)
Comment #50
fagoThanks.
Why doesn't it just check for being an content entity then? Seems to be better than trying and catching exceptions later on.
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.
Comment #51
YesCT CreditAttribution: YesCT commentedI 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:
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.
?
Comment #52
plachRemoved 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.
Comment #53
Gábor HojtsyReformatted 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.
Comment #54
plachThe issue summary was still not accurate, sorry :)
(mainly the proposed solution)
Assigning to @yched for a final sign-off.
Comment #55
plachComment #56
plachComment #57
effulgentsia CreditAttribution: effulgentsia commentedStraight reroll.
Comment #58
effulgentsia CreditAttribution: effulgentsia commentedWas 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?
Comment #60
Gábor Hojtsy@effulgentsia: I don't know the specifics but suspect that would be for automated schema generation?
Comment #61
effulgentsia CreditAttribution: effulgentsia commented@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:
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.
Comment #62
effulgentsia CreditAttribution: effulgentsia commentedI 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.
a) The PHPDoc needs to be updated.
b) The issue summary says no UI change, but this looks like a UI change.
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?
Comment #63
effulgentsia CreditAttribution: effulgentsia commentedAnd the nits:
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?
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.
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.
Does this comment apply to the line above it? If not, then I don't understand it. If so, let's move it up.
Looks like a good change, but how it is relevant to this issue?
Comment #64
plachWe 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.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 ;)
@fago is planning to automate this in another issue (I don't remember which one) so
FieldDefinition
will always setcustom_storage
toTRUE
internally if the definition is marked ascustom
(which is aFieldDefinitionInterface
property only). We agreed this is a sufficient fix for now.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.
Comment #65
plachFixed the issue summary
Comment #66
yched CreditAttribution: yched commentedAs 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.
Comment #67
yched CreditAttribution: yched commentedAnd here's my review for the rest of the patch.
(+ 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 ?
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)
extraneous "is"
Nitpick : would need the same @todo comment as in _content_translation_form_language_content_settings_form_alter() above ?
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 ?
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 ?
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
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 ?
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 ?
Comment #68
effulgentsia CreditAttribution: effulgentsia commentedRe #67.1: because FSDI doesn't have an isComputed() method. See also #64.3.
Comment #69
yched CreditAttribution: yched commented@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)
Comment #70
plachThanks for the reviews, I will try to address them in the weekend, although I might not have time. If not will do monday night.
Comment #71
plachJust a reroll for now.
(sorry too many conflicts)
Comment #72
plachfor reals now
Comment #74
xjmI 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.
Comment #75
xjmmeep
Comment #77
xjmThe 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.Comment #78
alexpottFixed
ContentTranslationSettingsTest
. Working on #66 and #67Comment #79
alexpottDone what @yched suggests in #76. Let's see what fails.
FieldDefinitionInterface
also hasgetType()
andgetName()
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.Comment #81
xjmComment #82
xjmComment #83
alexpottrerolled... the drop is always moving :)
Comment #85
alexpottStill very likely to fail but it will get past standard install.
Comment #87
alexpottOkay 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?
Comment #88
alexpottPatch addresses #67:
FieldDefinition::CARDINALITY_UNLIMITED
Comment #89
yched CreditAttribution: yched commentedRegarding 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.
Comment #90
yched CreditAttribution: yched commentedRegarding point 5, _content_translation_update_field_translatability() :
I still have the feeling that the
part is not needed because the value we set is already there, but I might be wrong (phone review...)
Comment #91
plachI 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.
Comment #92
plachThis 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.
Comment #93
plachRe-uploading the broken patch (which is expected to fail tests).
Comment #95
xjmIs the API change section for this issue up to date?
Can we check for any existing change records that might need an update?
Comment #96
plachThis 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.
Comment #97
yched CreditAttribution: yched commentedThanks ! No more remarks on my side.
@xjm I'll let you RTBC when summary and change notice are sorted out ?
Comment #98
alexpottComment #99
alexpottI'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.
Comment #100
xjmI think a short announcement that translatability is now per-instance rather than per-field is called for, no?
FieldDefinitionInterface
inherits fromFieldStorageDefinition
in HEAD:Couple other questions/observations:
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?We also seem to be changing this default.
BTW, all the red in this base class underscores for me that this is a good change.
Comment #101
xjmxpost
Comment #102
alexpottI 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 :)
Drupal\editor\Plugin\InPlaceEditor\Editor
is working both with base fields and configurable fields.getFieldDefinition()
only guarantees youFieldDefinitionInterface
so you need to get the storage definition to work with its methods.I'm searching for more affected CRs
Comment #103
alexpottI'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.
Comment #104
alexpottI've updated those CR's for the FSDI change and think we need to update these once this is in.
Comment #105
xjm@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.Comment #106
xjmAlso, 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.
Comment #107
alexpottActually 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.
Comment #108
effulgentsia CreditAttribution: effulgentsia commentedRe #105: #2289391: Reconsider whether FieldDefinition should implement FieldStorageDefinitionInterface
Comment #109
xjm@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. ;)
Comment #110
catchLooked over this, the main niggles are covered by the other critical task, so committed/pushed to 8.x, thanks!
Comment #112
xjmI've made the change record updates @alexpott listed and published the updated version of https://www.drupal.org/node/2236285.
Comment #113
xjm...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.
Comment #114
YesCT CreditAttribution: YesCT commentedThis introduced #2290849: Regression: Fields missing from translation overview since "Clarify handling of field translatability"
Comment #115
xjmAlright, fixed that CR with @Berdir's help. All done here!
Comment #117
Gábor Hojtsy