In #2076445-69: Make sure language codes for original field values always match entity language regardless of field translatability @yched said:
I'm not sure I see the value in keeping the internal runtime storage keyed by LANGCODE_DEFAULT (in $this->translations, $this->fields[$field_name], $this->values[$field_name]), rather than the actual langcode. It complicates the code and logic in a lot of places in ContentEntityBase ?
To which @fago replied:
I think we should start arranging data according to our translation model, i.e. entity - language - fields. I agree the keying by default language stuff seems deprecated since the object now deals with all translations, instead we could start doing shortcuts as $this->current_fields =& $this->fields[$this->activeLangcode];. But that all does not have to happen in this issue as it's internal restructuring, so let's do as it's easier?
Comment | File | Size | Author |
---|---|---|---|
#21 | refactor-2142885-21.patch | 13.33 KB | siva_epari |
#7 | refactor-internal-storage-2142885-1.patch | 13.17 KB | Berdir |
Comments
Comment #1
yched CreditAttribution: yched commentedWe also discussed putting hardcoded logic in FieldItemList::getLangcode(), removing the need for setLangcode().
I'd favor doing that in this patch too (easier to review "final, simplified code" than an intermediary step containing remnants of complex logic that we know is useless anyway; + at this point in time, yay on less followups mentally track)
But that probably depends on the actual code impact.
Comment #2
BerdirThis will also affect the constructor logic in #1867228: Make EntityTypeManager provide an entity factory
Comment #3
yched CreditAttribution: yched commented#2076445: Make sure language codes for original field values always match entity language regardless of field translatability is RTBC - maybe we can unpostpone this ?
(patches would need to include that other patch until it gets committed, but at least there's a solid ground)
Comment #4
yched CreditAttribution: yched commentedFWIW, I still think we should do this. The internal code supporting ContentEntity translation is more complex / confusing that it needs to be - and we don't really need more complexity here :-)
Comment #5
BerdirI don't disagree :)
The entity cache makes this less of a problem, but it would still improve the performance of loading entities from the storage after cache clears/updates/inserts.
Comment #6
chx CreditAttribution: chx commentedAs berdir asked me to comment: I am neutral. Just let me know what changes and I will adapt.
Comment #7
BerdirOk, let's try this. Entity tests are passing.
Comment #8
yched CreditAttribution: yched commentedOh sweet, thanks a ton for kicking this @Berdir.
Internally keying by language first definitely makes more sense in D8 :-)
Any chance we can get rid of the weirdness of "the internal key for the default langcode is LANGCODE_DEFAULT ('x-default') rather than the actual langcode" ? IIRC that would further simplify ContentEntityBase by removing needless massaging and special casing.
Comment #10
BerdirI'm not sure, one problem is that the default langcode will also be keyed by that very langcode :) That's kind of recursive ;)
It would essentially mean that we trade the $translations argument, against a $default_langcode argument to make that possible?
Comment #11
yched CreditAttribution: yched commented@Berdir : you mean the boolean property pointed by the $this->defaultLangcodeKey key ? In fact, I don't clearly see where it's actually used, and why exactly it would be an issue that we don't have direct access to it - but looking back in the code trying to figure it out brings lots of places where removing that LANGCODE_DEFAULT special casing would simplify things :-)
Comment #12
yched CreditAttribution: yched commentedAlso, detail :
We could simply inline this as
foreach (array_keys($values) as $langcode) {
lower in the __construct() ?Comment #13
BerdirThat's kind of my point :)
We don't have to use it, because we know that the default language is keyed by LANGCODE_DEFAULT.
If you get fields with 'en', 'fr' and 'de' in there, to find out which one is the default language, you have to loop over the values of that field until you find the one that says 1.
And for new entities, where do you put the values? You might not even have a language code until the langcode field is initialized, so you'd need a temporary value, or hardcode the default value implementation of the language field (we might actually have that somewhere already, not sure).
But maybe it's easier than I think, let's see what @plach thinks.
Comment #14
yched CreditAttribution: yched commentedSure, but :
- it doesn't seem like having to loop on $values until you find the one where $values[$some_langcode]['default_langcode'] == TRUE would be a real perf hit ?
- I'm not even sure which part of the code actually needs that info anyway ?
As you point, I guess new entities being created, and the fact that their langcode is, well, part of the values keyed by langcode, might be a bit more problematic, true :-/. It would be a bit sad that this has to spill over the code and runtime structures in ContentEntityBase, cause the impact on code complexity is real...
Comment #15
plachIt's used in the
::onChange()
method to prevent the related field value from being changed, since it could be initialized via field setters and not via constructor values. Theisset()
check is used to detect this scenario.I don't think this is a big deal unless you have 50 translations, however...
... honestly I don't see how using an actual language would make things simpler. You'd basically trade
LanguageInterface::LANGCODE_DEFAULT
with$this->defaultLangcode
more or less as-is. And additionally you'd have to detect which is the default language by looping on$values
and rekey everything when the default language changes. Also, this would break when constructing the entity with empty values and initializing field values via setters, as we might not have thedefault_langcode
flag available to determine the default language. I suspect trying to implement this approach would make things more complex.The second test looks redundant. Also we could skip one nesting level by and-ing the first one and the parent one.
Comment #16
plachInstead, @fago's suggestion about using internal references (see the OP) could be leveraged to point to
$this->values[LanguageInterface::LANGCODE_DEFAULT]
and avoid referring toLanguageInterface::LANGCODE_DEFAULT
in most places.Comment #17
yched CreditAttribution: yched commentedOK, too bad about LANGCODE_DEFAULT.
Having $this->current_fields as a ref to $this->fields[$this->activeLangcode] sounds handy, but then wouldn't we want the same for $this->current_values / $this->fields[$this->activeLangcode] ?
Comment #20
BerdirTagging for reroll.
Comment #21
siva_epari CreditAttribution: siva_epari commentedPatch rerolled.
Comment #29
geek-merlin