Problem/Motivation
ContentEntityInterface::getTranslation()
returns a new translation if the specified language is valid but there's no corresponding translation for it yet. This is not ideal, we should be creating new translations explicitly. Moreover this has also performance implications, since ContentEntityInterface::addTranslation()
is slow (see also #2382675: hook_entity_create() affects the data of new translations of existing entities in unexpected and undocumented ways).
Proposed resolution
Remove the ContentEntityInterface::addTranslation()
invocation from ContentEntityInterface::getTranslation()
.
Remaining tasks
- Figure out what's the desired behavior when
LanguageInterface::LANGUAGE_NOT_SPECIFIED
is passed. - Reviews
User interface changes
None
API changes
Yes
Data model changes
None
Beta phase evaluation
Issue category | Bug because this can trigger unexpected behaviors. |
---|---|
Issue priority | Major because this has several implications: it makes the Entity Translation API more fragile, it may cause perfomance issues when listing multiple entities in multilingual environments and is also preventing us from finding a sane solution for #2072945: Remove the $langcode parameter in EntityAccessControllerInterface::access() and friends. |
Prioritized changes | The main goals of this issue are reducing API fragility and improving performance and security. |
Disruption | Disruptive for contributed and custom modules leveraging the Entity Translation API in an improper way. Core fixes are mainly in tests. |
Comment | File | Size | Author |
---|---|---|---|
#22 | et-invalid_translation_exception-2090983-22.patch | 38.02 KB | plach |
Comments
Comment #1
yched CreditAttribution: yched commentedYes, we need to refactor how widgets and formatters are invoked on the fields of an entity (field_attach_*() / field_invoke_method*())
But for that multilingual logic and language fallback needs to be sorted out.
Comment #2
jibranThere is no EntityNG in the core now.
Comment #3
BerdirYeah, let's see if this still happens.
Comment #5
yched CreditAttribution: yched commented- addTranslation() is unnecessarly called :
field_invoke_method_multiple() is long gone now, not sure whether that's still the case ?
- addTranslation() is slow :
Yeah, posted a patch in #2382675: hook_entity_create() affects the data of new translations of existing entities in unexpected and undocumented ways, that removes the entity_create() - let's discuss there ?
Comment #6
plachAnd with invalid I mean both a language not installed on the site and a language for which a translation does not exist.
Comment #7
plachLet's try this
Comment #9
plachComment #10
plachOops, wrong patch
Comment #11
plachAnd now with less debuggity
Comment #13
plachGreen patch, reviews welcome :)
Comment #14
yched CreditAttribution: yched commented@plach : the IS is from 2 years ago :-) Can you provide a small description of what the patch actually does, to help reviewing ?
Comment #15
plachI'm not sure this is correct/desirable.
@yched:
Updated the IS. I'm working the API sanity aspects here and punting the rest to #2382675: hook_entity_create() affects the data of new translations of existing entities in unexpected and undocumented ways.
Comment #16
andypostSo no exception thrown... when requested unknown language, maybe assert() this?
"a configured language" confusing, maybe "configurable?
Makes sense! but docs for \Drupal\Core\TypedData\TranslatableInterface::getTranslation() should be updated
Comment #17
yched CreditAttribution: yched commentedStill wrapping my mind around the behavior change, no real opinion atm :-)
Just, side notes (not introduced by the patch, but might be worth clarifying since dealing with that very piece of logic here) :
- ContentEntityBase::getLanguages() says "{@inheritdoc}" but that seems to be a lie, according to my PHPstorm there's nothing to inherit from :-)
- The ->isLocked() checks in getTranslation() are not really clear, could use a comment ?
Comment #18
yched CreditAttribution: yched commentedAlso, that comment looks fuzzzy, the whole crux of the issue is that this is about "existing translation for the entity" rather than "existing language", right ? :-)
Comment #19
yched CreditAttribution: yched commentedPatch looks consistent, but I'm not sure how we can justify that the API break is worth it ?
That would be something like "you now cannot write code that inadvertently calls $entity->getTranslation($arbitrary_langcode) and triggers a costly translation creation" ?
Looks like this would be a perf issue moslty in cases where this is done in a very naive loop (like "foreach (enabled languages) {get the translation}") ?
Comment #20
plachMy main reason for doing this is helping with #2072945: Remove the $langcode parameter in EntityAccessControllerInterface::access() and friends: the moment you cannot request a non-existing translation also access checks become more explicit. Combine API sanity, Performance improvement and Security hardening and I think we have a good reason to do this.
Comment #21
yched CreditAttribution: yched commentedOK, makes sense.
Then wondering whether returning NULL ("there is no translation") would be better than an exception ? I guess the exception makes the API change more "in you face", but then again doing anything with an $entity that is NULL will also break quite fast ?
Comment #22
plachThis addresses #15, I just discovered quite a few bugs in our tests by just removing that magic. It should address also #16 and #18, thanks.
@#17:
1: Yep, looks like an unrelated doc bug
2: Locked languages are built-in system languages that do not map to any actual language, like
und
. Added a comment to clarify what's happening in::addTranslation()
.@#21:
Yep, I think with an exception it is way easier to figure out what's going wrong.
Comment #23
plachComment #24
yched CreditAttribution: yched commentedLooks good to me. Temptative RTBC, but committers will probably want a beta evaluation
Comment #25
plachAnd a change record :)
Thanks!
Comment #26
plachAdded beta evaluation and promoted to major since this is soft-blocking a major.
Comment #27
plachAnd the CR: https://www.drupal.org/node/2578373
Comment #28
yched CreditAttribution: yched commentedBack to RTBC then :-)
Comment #29
yched CreditAttribution: yched commentedComment #32
alexpottDisambiguating getting and creating translations is a really good idea. Committed 5c0eb9b and pushed to 8.0.x. Thanks!
Fixed on commit.
Comment #36
yched CreditAttribution: yched commentedGee, PIFR you're great but also painful
Comment #37
mkalkbrennerSorry for hitting the retest link!
But I think there's something wrong with this patch. I opened a follow-up:
#2579187: Revert to an older entity revision with less translations leads to fatal error caused by EntityStorageException
Comment #38
yched CreditAttribution: yched commentedOops, sorry @mkalkbrenner, I didn't intend to be rude, I didn't see your retest :-)
I thought that was the bot, coz it keeps doing these kind of things currently.
Comment #40
quietone CreditAttribution: quietone at PreviousNext commentedPublished the change record