Updated: Comment #N
Problem/Motivation
The new entity translation API allows to pass around an entity object for a given language with $entity->getTranslation('de');
.
That makes the $langcode arguments useless and confusing (which language should be used now?), so it should be removed.
Proposed resolution
Remove $langcode from all $entity_view(), view_multiple() and render controller methods. Make sure that the correct translation is passed in elsewhere.
Remaining tasks
Write the patch. Possibly handle configuration entities somehow.
User interface changes
-
API changes
Removal of the additional arguments. Different way to access the entity fields, which is in line with the ongoing EntityNG changes.
Related Issues
#2072945: Remove the $langcode parameter in EntityAccessControllerInterface::access() and friends
Comment | File | Size | Author |
---|---|---|---|
#35 | 2073217-entity_view-language-35.patch | 35.13 KB | plach |
#35 | 2073217-entity_view-language-35.interdiff.txt | 4.58 KB | plach |
#29 | 2073217-entity_view-language-29.patch | 36.61 KB | plach |
#29 | 2073217-entity_view-language-29.interdiff.txt | 15.2 KB | plach |
#24 | 2073217-entity_view-language-24.patch | 46.53 KB | andypost |
Comments
Comment #1
BerdirGetting started to see how this goes.
Haven't touched formatter and widget methods yet, but we want to remove it there as well.
Comment #2
BerdirComment #4
Berdir#1: langcode-view-2073217-1.patch queued for re-testing.
Comment #6
BerdirRe-roll.
Comment #8
BerdirInteresting, looks like search is the only use case that we have right where the global language negotation logic doesn't work.
So the multilingual test will continue to fail, but the notices should be fixed.
This will then need the language changes that are going in other changes to simply use the entity in the right language.
Comment #10
plachComment #11
jibran8: langcode-view-2073217-8.patch queued for re-testing.
Comment #13
plachComment #14
plachThis might make things easier here: #2090983: ContentEntityInterface::getTranslation() should throw an exception when an invalid language is specified.
Comment #15
Wim LeersOMG yes…
Comment #16
andypostPatch build from scratch based on #8 because code changed slightly
Pushed to d8mi sandbox
2073217-entity_view-language
Comment #18
andypostShould fix fatal
Comment #20
andypostOnly views filters related tests fail, no idea why yet.
Also filed #2578433: \Drupal\views\Plugin\views\filter\FieldList uses undefined function
Comment #23
andypostlooks that only API change here
all other places $langcode an optional argument already
Comment #24
andyposta bit more clean-ups, but search and views still fails
Comment #27
plachSorry, didn't get to this one. I will tomorrow...
Comment #28
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedOverall looks good to me, just a question.
To (optionally) lazy build an entity, what is needed is to then strip out $entity->language()->id() and pass it in and then would I need to do anything to load this specific language?
Or would I need to somehow restore the context to be able to render this specific language? (e.g. in an ESI callback)
Comment #29
plachI played with this and thought about it during the last couple of days. I came to the conclusion the probably removing the
$langcode
parameter for the::view()
/::viewMultiple()
methods is not a good idea, because it would imply a big DX regression. In fact, in the 80% of the use cases when viewing an entity we want to do that in the current language, however we still need to account for the remaining 20% where we want explicitly view the entity in a specific language (e.g. when sending translated newsletters).Removing the
$langcode
parameter to rely solely on the translation language would have the consequence of requiring the 80% of use cases to manually perform entity language negotiation viaEntityManagerInterface::getTranslationFromContext()
, which would be very fragile and likely to break, since we learned the hard way that people don't code with multilingual scenarios in mind by default (including me :).The attached patch reverts only that change but removes the
$langcode
parameter everywhere else down the invocation chain, where we can safely assume entity language negotiation has already been performed. Hopefully this will provide a good balance between DX and API sanity. I realize this is slightly inconsistent with what we are aiming to do in #2072945: Remove the $langcode parameter in EntityAccessControllerInterface::access() and friends, but IMHO there the context in which the API will be used is way less defined, and so assuming that the current language would be a good default is way more risky. Moreover, since we are dealing with access control, developers need to be as explicit as possible about their intentions.@Fabianx:
I think the former:
ContentEntityInterface::getTranslation($langcode)
is all it's required to restore the context.Comment #30
plachAdditionally, this would be a way smaller API break, since
::view()
and::viewMultiple()
are the main API "entry points".Comment #31
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedLooks much better to me now, needs a change record though.
Comment #32
Wim LeersIndeed, this looks much better!
Comment #35
plachLet's try this
Comment #36
plachMmh, I guess this could use some improvement
Comment #37
Wim LeersBurdening developers far less == win.
Comment #39
effulgentsia CreditAttribution: effulgentsia at Acquia commentedThank you, thank you, thank you! Very happily pushed to 8.0.x.
"Needs work" for the change record.
Are we sure about this? If this is incorrect, please open a followup to revert it.
Comment #40
xjmWe don't make things critical for CR anymore; usually require them before commit. :)
Comment #42
Gábor HojtsyAdded and published change notice at https://www.drupal.org/node/2581003.
Comment #43
Gábor Hojtsy@effulgentsia: re the two entity_view() changes in contact module, it looks like entity_view() still takes a langcode, and there is nothing in the contact code apparently to make sure the right translation of the message is loaded. (OTOH at least in core, messages cannot be translated because they don't persist and have monolingual input only). So it may or may not be a mistake depending on what kind of expectations we set for contact message translatability.
Comment #44
plach@effulgentsia: @Gábor Hojtsy:
I can envision a scenario where a contrib
contact_translation
module could hook into the execution flow, and add translations to the contact message via a machine-translation service. This would be useful to allow a support center to react promptly even to support request in languages they don't understand. However thecontact_message
entity type is not even translatable currently, so this is not a problem in core, although those language parameters should be restored to allow translatability to be enabled in contrib.Opened #2582621: Support translation of already-submitted contact messages for that.