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.

#2072945: Remove the $langcode parameter in EntityAccessControllerInterface::access() and friends

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Berdir’s picture

Status: Active » Needs review
FileSize
39.66 KB

Getting started to see how this goes.

Haven't touched formatter and widget methods yet, but we want to remove it there as well.

Berdir’s picture

Priority: Normal » Major

Status: Needs review » Needs work
Issue tags: -API change, -Entity Field API

The last submitted patch, langcode-view-2073217-1.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review

#1: langcode-view-2073217-1.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +API change, +Entity Field API

The last submitted patch, langcode-view-2073217-1.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
38.7 KB

Re-roll.

Status: Needs review » Needs work

The last submitted patch, langcode-view-2073217-6.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
1.46 KB
39.79 KB

Interesting, 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.

The last submitted patch, langcode-view-2073217-8.patch, failed testing.

jibran’s picture

Status: Needs work » Needs review

8: langcode-view-2073217-8.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 8: langcode-view-2073217-8.patch, failed testing.

plach’s picture

Issue tags: +rc deadline
plach’s picture

Wim Leers’s picture

OMG yes…

andypost’s picture

Status: Needs work » Needs review
FileSize
45.17 KB

Patch build from scratch based on #8 because code changed slightly

Pushed to d8mi sandbox 2073217-entity_view-language

Status: Needs review » Needs work

The last submitted patch, 16: 2073217-entity_view-language-14.patch, failed testing.

andypost’s picture

Status: Needs work » Needs review
FileSize
739 bytes
45.11 KB

Should fix fatal

Status: Needs review » Needs work

The last submitted patch, 18: 2073217-entity_view-language-18.patch, failed testing.

andypost’s picture

Only views filters related tests fail, no idea why yet.

Also filed #2578433: \Drupal\views\Plugin\views\filter\FieldList uses undefined function

The last submitted patch, 16: 2073217-entity_view-language-14.patch, failed testing.

The last submitted patch, 18: 2073217-entity_view-language-18.patch, failed testing.

andypost’s picture

+++ b/core/includes/entity.inc
@@ -357,12 +354,12 @@ function entity_page_label(EntityInterface $entity, $langcode = NULL) {
-function entity_view(EntityInterface $entity, $view_mode, $langcode = NULL, $reset = FALSE) {
+function entity_view(EntityInterface $entity, $view_mode, $reset = FALSE) {

@@ -394,12 +388,12 @@ function entity_view(EntityInterface $entity, $view_mode, $langcode = NULL, $res
-function entity_view_multiple(array $entities, $view_mode, $langcode = NULL, $reset = FALSE) {
+function entity_view_multiple(array $entities, $view_mode, $reset = FALSE) {

looks that only API change here
all other places $langcode an optional argument already

andypost’s picture

Status: Needs work » Needs review
FileSize
1.59 KB
46.53 KB

a bit more clean-ups, but search and views still fails

Status: Needs review » Needs work

The last submitted patch, 24: 2073217-entity_view-language-24.patch, failed testing.

The last submitted patch, 24: 2073217-entity_view-language-24.patch, failed testing.

plach’s picture

Sorry, didn't get to this one. I will tomorrow...

Fabianx’s picture

Overall 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)

plach’s picture

I 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 via EntityManagerInterface::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:

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)

I think the former: ContentEntityInterface::getTranslation($langcode) is all it's required to restore the context.

plach’s picture

Title: Remove the $language parameter from the entity view/render system » Remove the $langcode parameter from the entity view/render system

Additionally, this would be a way smaller API break, since ::view() and ::viewMultiple() are the main API "entry points".

Fabianx’s picture

Issue tags: +Needs change record

Looks much better to me now, needs a change record though.

Wim Leers’s picture

Indeed, this looks much better!

Status: Needs review » Needs work

The last submitted patch, 29: 2073217-entity_view-language-29.patch, failed testing.

The last submitted patch, 29: 2073217-entity_view-language-29.patch, failed testing.

plach’s picture

Let's try this

plach’s picture

+++ b/core/lib/Drupal/Core/Entity/entity.api.php
@@ -468,7 +468,9 @@
- * // You can omit the language ID if the default language is being used.
+ * // You can omit the language ID, by default the current content language will
+ * // be used. If no translation is available for the current language, fallback
+ * // rules will be used.

Mmh, I guess this could use some improvement

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +DX (Developer Experience)

Burdening developers far less == win.

  • effulgentsia committed ace96de on 8.0.x
    Issue #2073217 by andypost, Berdir, plach: Remove the $langcode...
effulgentsia’s picture

Title: Remove the $langcode parameter from the entity view/render system » [Needs change record] Remove the $langcode parameter from the entity view/render system
Priority: Major » Critical
Status: Reviewed & tested by the community » Needs work

Thank you, thank you, thank you! Very happily pushed to 8.0.x.

"Needs work" for the change record.

+++ b/core/modules/contact/contact.module
@@ -142,7 +142,7 @@ function contact_mail($key, &$message, $params) {
-      $build = entity_view($contact_message, 'mail', $language->getId());
+      $build = entity_view($contact_message, 'mail');

Are we sure about this? If this is incorrect, please open a followup to revert it.

xjm’s picture

Priority: Critical » Major

We don't make things critical for CR anymore; usually require them before commit. :)

The last submitted patch, 35: 2073217-entity_view-language-35.patch, failed testing.

Gábor Hojtsy’s picture

Title: [Needs change record] Remove the $langcode parameter from the entity view/render system » Remove the $langcode parameter from the entity view/render system
Status: Needs work » Fixed
Issue tags: -Needs change record

Added and published change notice at https://www.drupal.org/node/2581003.

Gábor Hojtsy’s picture

@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.

plach’s picture

@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 the contact_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.

Status: Fixed » Closed (fixed)

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