Problem/Motivation

Assume the interface language is english and a new entity e.g. node is created with only one default translation - german. In \Drupal\node\NodeForm::save() there is a redirect where the entity language isn't put into the url options, which doesn't allow the language negotiation methods to properly prepare the url so that when it is visited the entity language and the content type language both are german. As this is not the case when the url is visited then the EntityConverter will initiate the language detection, but as the request miss any metadata the interface language will be used as content language leading to seeing the entity in german but having the language manager reporting content type language being english.

I think we have a lot of problems in this direction therefore I think this issue should be considered major.

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Comments

hchonov created an issue. See original summary.

hchonov’s picture

Status: Active » Needs review
StatusFileSize
new1.08 KB

Adding an assertion to \Drupal\Core\Entity\ContentEntityForm::initFormLangcodes(), which should check that the current entity language and the content type language match.

Status: Needs review » Needs work

The last submitted patch, 2: 2940992-2-assert-CEF.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

hchonov’s picture

Status: Needs work » Needs review
StatusFileSize
new1.2 KB
+++ b/core/lib/Drupal/Core/Entity/ContentEntityForm.php
@@ -309,6 +310,8 @@ protected function initFormLangcodes(FormStateInterface $form_state) {
+    assert('!$this->entity->getEntityType()->hasKey("langcode") || (\Drupal::languageManager()->getCurrentLanguage(LanguageInterface::TYPE_CONTENT)->getId() == $this->entity->language()->getId())', 'The entity language and the content language should be identical.');

It looks like, at least locally with PHP 7.0.27, that PHP cannot correctly evaluate this assertion. PHP bug? I've rewritten it to use a callback which is now working locally.

hchonov’s picture

StatusFileSize
new12.63 KB

I've found a couple of places where I think the language option should be set when building an entity url.

hchonov’s picture

Assigned: Unassigned » plach

It looks pretty complicated and I am not sure how we should proceed here, but if the content type language couldn't be relied on, then probably we should deprecate it.

Assigning to @plach for his opinion and attention on this subject :).

tstoeckler’s picture

+++ b/core/modules/node/src/NodeForm.php
@@ -298,7 +298,8 @@ public function save(array $form, FormStateInterface $form_state) {
         $form_state->setRedirect(
           'entity.node.canonical',
-          ['node' => $node->id()]
+          ['node' => $node->id()],
+          ['language' => $node->language()]
         );

I think here and elsewhere we should use $entity->toUrl() instead, which already adds the language option, at least where possible. That both improves readibility of the code and fixes these language related problems.

The last submitted patch, 4: 2940992-4-assert-CEF.patch, failed testing. View results

Status: Needs review » Needs work

The last submitted patch, 5: 2940992-5.patch, failed testing. View results

hchonov’s picture

Status: Needs work » Needs review
StatusFileSize
new12.93 KB

@tstoeckler, you are right :). I've adjusted this where it is possible, but we have places where we don't have a link relation even if the route points to an entity. This is the case for the route forum.page. Should we add a link relation for this route to the taxonomy term in a hook_entity_type_alter implementation in the forum module?

What do you think about the changes in the user module, where I check if the user entity has a translation for the current interface language and and if so use it, otherwise fallback to the default entity translation of the user entity. I thought about running the user entity through EntityRepository::getTranslationFromContext(), but I think that in this case the user entity routes will be different if the content type language changes and I am not sure if we want this as this might happen if the user just visits the e.g. spanish translation of a random entity.

P.S. I haven't uploaded an interdiff, because I had to rewrite almost everything :).

Status: Needs review » Needs work

The last submitted patch, 10: 2940992-10.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

hchonov’s picture

Status: Needs work » Needs review
StatusFileSize
new12.97 KB
new2.41 KB
tstoeckler’s picture

Should we add a link relation for this route to the taxonomy term in a hook_entity_type_alter implementation in the forum module?

I'm not necessarily opposed to this, but generally speaking this is not always possible because the route parameters that are necessary for a particular link relation (i.e. Entity::urlRouteParameters()) cannot be extended in any way, so if you need an additional route parameter for a link relation you are out of luck. That's why I generally stick to those link relations defined by the entity itself. In this case it would actually work, though, because just the term is required as a parameter and it would make the affected code quite a bit nicer. So don't feel strongly either way, not really sure.

I thought about running the user entity through EntityRepository::getTranslationFromContext(), but I think that in this case the user entity routes will be different if the content type language changes and I am not sure if we want this as this might happen if the user just visits the e.g. spanish translation of a random entity.

I'm sorry but I can't quite follow that. Can you elaborate what your concerns with ::getTranslationFromContext() are?

Status: Needs review » Needs work

The last submitted patch, 12: 2940992-12.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

hchonov’s picture

I'm sorry but I can't quite follow that. Can you elaborate what your concerns with ::getTranslationFromContext() are?

Consider the user visits a german translation of an entity and then the link pointing to the user profile will point to the german translation of the user entity.
Then the user visits the spanish translation of an entity and the link to the user profile might change and point to the spanish translation of the user entity.

I am just not sure this is the correct behavior.

hchonov’s picture

Status: Needs work » Needs review
StatusFileSize
new21.59 KB
new7.78 KB

I've found some more places where we should use the entity to build the url.

Status: Needs review » Needs work

The last submitted patch, 16: 2940992-16.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

hchonov’s picture

Status: Needs work » Needs review
StatusFileSize
new2.43 KB
new25.62 KB

We have to add the language option to the node add links as well. However changing the language in the the language widget should be changing the form action to which the form will be posted, which doesn't happen currently. This is the second time I encounter problems with the language widget - the first was that when it is changed the form langcode is not changed, so I guess we would need another issue for that.

Status: Needs review » Needs work

The last submitted patch, 18: 2940992-18.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

hchonov’s picture

Status: Needs work » Needs review
StatusFileSize
new26.21 KB
new10.1 KB

Hmm probably we should rely on the entity repository to retrieve the translation language from the context?

Status: Needs review » Needs work

The last submitted patch, 21: 2940992-21.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

hchonov’s picture

Assigned: plach » Unassigned
tstoeckler’s picture

StatusFileSize
new26.07 KB

Rerolled this one. Also took a look at the patch.

  1. +++ b/core/modules/book/src/BookBreadcrumbBuilder.php
    @@ -17,6 +17,13 @@
    +  /**
    +   * The entity manager.
    +   *
    +   * @var \Drupal\Core\Entity\EntityManagerInterface
    +   */
    +  protected $entityManager;
    
    @@ -40,6 +47,7 @@ class BookBreadcrumbBuilder implements BreadcrumbBuilderInterface {
    +    $this->entityManager = $entity_manager;
    

    Let's call this $entityRepository as that's what's actually being used here. We can still assing the entity manager, for now, but that will reduce the things we have to update once we properly deprecate the entity manager.

  2. +++ b/core/modules/responsive_image/src/ResponsiveImageStyleForm.php
    @@ -281,13 +281,10 @@ public function save(array $form, FormStateInterface $form_state) {
         if (!$responsive_image_style->hasImageStyleMappings()) {
    ...
    +      $form_state->setRedirectUrl($responsive_image_style->toUrl('edit-form'));
    ...
         else {
    ...
    +      $form_state->setRedirectUrl($responsive_image_style->urlInfo('collection'));
         }
    

    Minor, but now that this is using the same method, the setRedirectUrl() call could be moved outside of the if-else and we could only assing $url = ... in the if-else.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

gease’s picture

StatusFileSize
new27.24 KB

Re-roll of the previous patch.

gease’s picture

StatusFileSize
new43.77 KB
new22 KB

This update fixes tests (sometimes this required changing tests logic, because on entity translation save the redirect is now to the translation, not to the original entity), sometimes changing the patch itself (eg, for the user, whih is also a content entity, logic on login should be different - language should be kept), sometimes removing deprecations in the patch, or changing mocks and expectations for unit tests.

gease’s picture

Status: Needs work » Needs review
gease’s picture

StatusFileSize
new47.34 KB
new5.24 KB

Another attempt to fix all tests. And, are we sure that on user login and redirect to user profile, we want to keep content language, not the interface language?

gease’s picture

StatusFileSize
new47.24 KB

Simple reroll for 8.8.x

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

hchonov’s picture

StatusFileSize
new51.85 KB
new15.61 KB

We've noticed that on user logins the user preferred langcode is not considered, which this patch is trying to change. It also introduced some minor cleanups.

Status: Needs review » Needs work

The last submitted patch, 33: 2940992-33-8.8.x.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

sanjayk’s picture

StatusFileSize
new51.66 KB

Re-roll patch #33 on 8.9

sanjayk’s picture

StatusFileSize
new53.53 KB

Now fixed issues partially working on rest of issue once fix. I will update here.

sanjayk’s picture

Status: Needs work » Needs review
sanjayk’s picture

StatusFileSize
new47.38 KB

Re-roll for 9.2.
Not getting any clue how to fix (FATAL Drupal\Tests\language\Functional\LanguageUrlRewritingTest: test runner returned a non-zero error code (2).) error if anybody have any idea let me know. Thanks

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

smustgrave’s picture

Issue tags: +Bug Smash Initiative, +Needs issue summary update
StatusFileSize
new3.44 KB
new47.69 KB

Rerolled for 9.5 and fixed some phpcs errors.

The IS appears to be incomplete.

smustgrave’s picture

Status: Needs review » Needs work

For the issue summary.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.