Problem/Motivation

I translated field labels through the configuration translation module UI. When I create a node and then translate it the translated version reflects translated field labels so for example fr/node/nid has all labels translated. However if I clear the cache while still being on translated page (any path with language prefix) all labels become English (default language) again. If I switch to default language and then clear cache one more time then switch to French labels are translated again.

Basically every time the cache is cleared through UI while on the page with language prefix fr/... it resets translation for previously translated configuration items. And when the cache is cleared on default language the translation is back to normal.

On the site all multilingual core modules are enabled. Translation is enabled for content types and for fields. Default language of the site is English. Nodes were created in English with French translation.

Steps to reproduce

  1. Install all modules in Multilingual (language, locale, config_translation, content_translation)
  2. Added French language (any language should do) via /admin/config/regional/language
  3. Enable translation for Article under Content->Article at admin/config/regional/content-language
  4. Create a test Article node with some Tags content via /node/add/article
  5. View /fr/node/1 (or whatever), the Tags label should show as Étiquettes
  6. Navigate to /fr/admin/config/development/performance and clear cache via the UI
  7. Back to /fr/node/1 and the Étiquettes label is now "Tags"

Proposed resolution

After further investigation revealed that the translations getting cached while clearing cache in a state language override isn't in place. Which causes the system to save english/default translation into language prefixed cache key. This is caused by triggering entity title call in views route subscriber. Moving title of the route to title callback fixes the problem.

Remaining tasks

Manual testing and review.

User interface changes

N/A

API changes

N/A

Data model changes

N/A

CommentFileSizeAuthor
#146 interdiff-2650434.txt571 bytescatch
#137 interdiff-2650434-135-137.txt1.29 KBduozersk
#137 interdiff-2650434-128-137.txt3.01 KBduozersk
#137 clearing_cache_via_ui-2650434-137.patch10.83 KBduozersk
#135 interdiff-2650434-128-135.txt2.72 KBduozersk
#135 clearing_cache_via_ui-2650434-135.patch10.05 KBduozersk
#128 clearing_cache_via_ui-2650434-128.patch10.23 KBeiriksm
#126 clearing_cache_via_ui-2650434-126.patch10.18 KBeiriksm
#122 2650434-122.patch10.78 KBdavidraijmakers
#118 clearing_cache_via_ui-2650434-118.patch10.25 KBYogesh Pawar
#114 2650434--114.patch10.3 KBpguillard
#113 2650434-d8--2.patch10.42 KBdawehner
#107 2650434-103.patch10.26 KBtstoeckler
#105 interdiff-2650434-95-103.txt2.81 KBchipway
#103 interdiff-95-103.diff2.81 KBTwoD
#103 2650434-103.patch10.26 KBTwoD
#95 2650434-95.patch10.23 KBrobert-os
#91 2650434-91.patch10.27 KBandypost
#91 interdiff.txt1.35 KBandypost
#89 2650434-89.patch10.05 KBandypost
#89 interdiff.txt4.16 KBandypost
#84 2650434-diff-71-84.txt1.34 KBanemes
#84 2650434-84.patch10.71 KBanemes
#82 interdiff-2650434-71-82.txt2.36 KBchipway
#82 2650434-82.patch10.25 KBchipway
#79 2650434-diff-71-78.txt1.34 KBanemes
#78 2650434-78.patch0 bytesanemes
#71 2650434-diff-67-71.txt5.91 KBvijaycs85
#71 2650434-71.patch10.7 KBvijaycs85
#67 2650434-67.patch4.8 KBanemes
#65 2650434-65.patch4.46 KBanemes
#63 2650434-63.patch4.8 KBanemes
#60 2650434-60.patch3.8 KBanemes
#58 2650434-58.patch3.84 KBanemes
#55 2650434-55.patch18.48 KBvijaycs85
#55 2650434-diff-36-55.txt2.91 KBvijaycs85
#36 2650434-diff-35-36.txt4.33 KBvijaycs85
#36 2650434-36.patch18.42 KBvijaycs85
#35 2650434-diff-32-35.txt9.91 KBvijaycs85
#35 2650434.35.patch15.4 KBvijaycs85
#32 2650434-diff-13-32.txt1.15 KBvijaycs85
#32 2650434-32.patch7.04 KBvijaycs85
#23 2650434-22.patch1.89 KBvaplas
#18 2650434-18.patch8.37 KBvijaycs85
#17 2650434-config_translation_pass.png650.07 KBvijaycs85
#17 2650434-config_translation_fail.png613.97 KBvijaycs85
#13 2650434-13-diff-pass-fail.txt516 bytesvijaycs85
#13 2650434-13-fail.patch5.9 KBvijaycs85
#13 2650434-13-pass.patch5.87 KBvijaycs85
#10 2650434-10.patch1.95 KBvijaycs85
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

amykhailova created an issue. See original summary.

Cottser’s picture

Version: 8.0.1 » 8.0.x-dev

I was able to reproduce this as well with a clean D8 install.

  1. Clean install of D8, standard profile, commit hash b422873
  2. Install all modules in Multilingual (language, locale, config_translation, content_translation)
  3. Added French language (any language should do) via /admin/config/regional/language
  4. Create a test Article node with some Tags content via /node/add/article
  5. View /fr/node/1 (or whatever), the Tags label should show as Étiquettes
  6. Navigate to /fr/admin/config/development/performance and clear cache via the UI
  7. Back to /fr/node/1 and the Étiquettes label is now "Tags"

And to "fix" it you can clear the cache via /admin/config/development/performance to restore the French label.

Cottser’s picture

Title: Clearing cache on translated page resets translation of field labels to default language » Clearing cache via UI in translated language resets config translation of field labels to default language
Issue tags: -translation cache congiguration
zuuperman’s picture

I can confirm this issue. It's also happening on the entity forms.

swentel’s picture

Priority: Normal » Major

Been willing to confirm this as well for a couple of weeks now, but never got to it until today. Raising to major since it's extremely annoying - and also sometimes hard to reproduce consistently.

swentel’s picture

We're probably missing a tag and/or context somewhere.

penyaskito’s picture

Just guessing: maybe 'route.name' in \Drupal\Core\EventSubscriber\CacheRouterRebuildSubscriber::onRouterFinished?

vijaycs85’s picture

just to update the progress on the investigation:

  1. Commenting \Drupal::service('router.builder')->rebuild(); in drupal_flush_all_caches prevent the issue
  2. RouteBuilder::rebuild dispatches RoutingEvents::DYNAMIC, RoutingEvents::ALTER and RoutingEvents::FINISHED events which might be causing issue
  3. There is no difference in DB level (esp. state object in keyvalue table)
vijaycs85’s picture

#7:

Just guessing: maybe 'route.name' in \Drupal\Core\EventSubscriber\CacheRouterRebuildSubscriber::onRouterFinished?

We have just ['4xx-response', 'route_match'] in there (8.0.x). Are you saying we should add route.name?

vijaycs85’s picture

Status: Active » Needs review
FileSize
1.95 KB

Here is the test-only patch that fails.

Status: Needs review » Needs work

The last submitted patch, 10: 2650434-10.patch, failed testing.

vijaycs85’s picture

After further investigation, found that the issue is caused by \Drupal\views\EventSubscriber\RouteSubscriber::routes and \Drupal\views\EventSubscriber\RouteSubscriber::onAlterRoutes.

When we clear cache at [langcode]/admin/config/development/performance,

  1. RouteBuilder:rebuild()
  2. \Drupal\views\EventSubscriber\RouteSubscriber::routes and \Drupal\views\EventSubscriber\RouteSubscriber::alterRoutes(eventually as part of dispatching RoutingEvents::ALTER event).
  3. after few levels of calls (\Drupal\views\Plugin\views\display\PathPluginBase::getRoute -> $this->view->getTitle() -> initStyle()), it hits the entityManager
  4. This is where \Drupal\Core\Entity\EntityFieldManager::getFieldDefinition try to save the definitions discovery in cache_discovery.
  5. Since the \Drupal\Core\Config\Entity\ConfigEntityStorage->overrideFree is true at this stage, we are saving original/default definition with language prefixed cache key (e.g. entity_bundle_field_definitions:comment:vote:fr) permanently on cache_discovery
vijaycs85’s picture

Status: Needs work » Needs review
FileSize
5.87 KB
5.9 KB
516 bytes

Patches to prove #12. Just split the changes in #10 as separate test case (as we might need to add more tests for config translation + cache scenarios).

vijaycs85’s picture

Issue tags: +D8MI, +sprint

Status: Needs review » Needs work

The last submitted patch, 13: 2650434-13-fail.patch, failed testing.

tstoeckler’s picture

Re #12: Wow, that's majorly messed up. *Incredible* detective work @vijaycs85, very impressive!

Do you know where \Drupal\Core\Config\Entity\ConfigEntityStorage->overrideFree() gets set to TRUE?

vijaycs85’s picture

@tstoeckler: thank you :).

So it's basically happening at \Drupal\Core\Config\Entity\ConfigEntityStorage->loadMultipleOverrideFree. However how we are getting there in working vs not-working case is very different.

Attaching screenshot where overrideFree is TRUE/FALSE. Thought they are not exactly when the problem happens, just give idea of stack trace.

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
8.37 KB

Looks like somewhere we lost the functionality provided by Drupal\language\EventSubscriber\LanguageRequestSubscribe which was trying to setConfigOverrideLanguage, but has no effect. Attached patch fix the issue. Not sure, if we still need the LanguageRequestSubscribe.

Status: Needs review » Needs work

The last submitted patch, 18: 2650434-18.patch, failed testing.

Gábor Hojtsy’s picture

Issue tags: +language-config
+++ b/core/modules/language/src/ConfigurableLanguageManager.php
@@ -143,6 +144,19 @@ public function init() {
   /**
+   * Sets language overrides from current user.
+   *
+   * @param \Drupal\Core\Session\AccountInterface $current_user
+   * @param \Drupal\language\LanguageNegotiatorInterface $negotiator
+   */
+  public function setLanguageFromCurrentUser(AccountInterface $current_user, LanguageNegotiatorInterface $negotiator) {
+    $negotiator->setCurrentUser($current_user);
+    $negotiator->reset();
+    $this->setNegotiator($negotiator);
+    $this->setConfigOverrideLanguage($this->getCurrentLanguage());
+  }

Should the language negotiation process not end up in this language or some other language to set? I don't see why we need this explicitly, can you elaborate?

vijaycs85’s picture

Status: Needs work » Needs review

Sorry for the confusion @Gábor Hojtsy. That was me trying to fix the issue in a wrong way. Let's ignore #18.

The explanation in #12 stands valid still: When we clear cache with language prefix, the language manager get called even before the language context loaded to get the translation. So we are getting default (en) translation and saving it with current language prefix which is cached until you clear the cache from default/other language UI (or drush of course).

we have to use the tests in #13 as proof to find a solution.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

vaplas’s picture

Sorry if I should not write in the issue. But I also have a problem with Add / Edit Form of translations. But there are no problems with Delete Form. Cleaning cache from ConfigTranslationDeleteForm it is works for me.

vijaycs85’s picture

Thanks for reporting back the issue @vaplas, but the fix in your patch might not something we want to do. we want to find the root cause so that it fixes all scenarios.

Sharique’s picture

I spend some time today on this issue, but I'm not able to reproduce it, here is what I did

  • Added new lang German,
  • Created new content type news with a field summary,
  • Created a new story in both.
  • Translated summary field label to German, it didn't appear, after clearing it appears
  • I cleared cache few times while staying on German node, summery field label remains translated.

code is latest git code , not patch applied.

grahl’s picture

@Sharique: I haven't retested this with a latest dev but I have a hunch that this might be also triggered when the default language and the field base language differ, i.e.: create some configuration English, then switch site default to German and amend configuration from there with some fields having German as the base and some English.

vijaycs85’s picture

Issue summary: View changes

Updating issue summary with steps to reproduce from #1.

vijaycs85’s picture

Issue summary: View changes
ErnoVanhala’s picture

#26 I can confirm that this NASTY problem still exists.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

vijaycs85’s picture

Status: Needs review » Needs work

Reverting back to 'Needs work'

vijaycs85’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
7.04 KB
1.15 KB

As suggested by @timplunkett changing to title to callback. Though it doesn't solve any other similar issue from contrib module (because we don't define what can be done in Route/RouteAlter), fixes the issue summary locally.

Status: Needs review » Needs work

The last submitted patch, 32: 2650434-32.patch, failed testing.

tim.plunkett’s picture

+++ b/core/modules/views/src/Plugin/views/display/PathPluginBase.php
@@ -129,7 +129,7 @@ protected function defineOptions() {
+      '_title_callback' => get_class($this) . '::getTitle',

@@ -533,4 +533,18 @@ public function remove() {
+  public function getTitle() {
+    return $this->view->getTitle();

This won't work, as it's trying to instantiate this plugin as a controller. Additionally, $this->view won't be set.

It likely needs to be a method on ViewPageController. But you'll have to figure out how to get from $view_id/$display_id to a ViewExecutable

A side-note, you can use static::class instead of get_class($this)

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
15.4 KB
9.91 KB

Thanks @tim.plunkett. I have updated the code base. We are getting more fails with this patch. Just adding here with progress, if anyone wants to pick it up.

vijaycs85’s picture

FileSize
18.42 KB
4.33 KB

Another try with updated container and added test for getTitle() method.

grahl’s picture

I just tried #36 and am still seeing the issue on cache clear.

vijaycs85’s picture

Issue summary: View changes

@grahl, are you sure you are following the "steps to reproduce" in issue summary? If not (e.g like steps you mentioend in #26) we need new issue with steps to reproduce.

P.S There are few scenarios(like behind proxy), we don't get the translation file from localize.drupal.org when install module. So make sure the "tags" is translated on step 5 before proceed testing further.

Gábor Hojtsy’s picture

The patch only deals with views titles, while the summary and examples in comments indicate a much more widespread problem. How did we end up fixing views only?

vijaycs85’s picture

@Gábor Hojtsy, As per my analysis between #8 and #12, concluded that the root cause of the configuration cache is views title firing the discovery on routeAlter stage and the tests in #36 proves this theory :)

Gábor Hojtsy’s picture

Oh, THAT would be important to note in the summary then in a proposed resolution. :)

vijaycs85’s picture

Issue summary: View changes
Issue tags: -Needs issue summary update

Updated :)

vijaycs85’s picture

Issue summary: View changes
vijaycs85’s picture

Issue summary: View changes
badrange’s picture

I haven't studied the patches above, I just wanted to note that I started subscribing to this issue after we had trouble with menu items getting the wrong translation - and this problem stopped occuring when we stopped clearing caches through the UI. It may be fixed by this patch, it may be a different issue, who knows.

vijaycs85’s picture

trouble with menu items getting the wrong translation - and this problem stopped occuring when we stopped clearing caches through the UI

Sounds related. Worth a try to test with patch or update steps to reproduce with this problem.

iampuma’s picture

Currently having the same problem with an English-Icelandic website (Drupal 8.1.8).

Default language is set to English, and labels are translated to Icelandic. Clearing caches resets all labels to English for some reason.

EDIT: I could conclude that flushing caches in a language other than the source language will mess up the translation of field labels. Clearing caches with drush however works ONLY if the setting "Selected language" in language detection is set to the "source language" as well.

Gábor Hojtsy’s picture

@iampuma: does this patch help though?

badrange’s picture

I just did a quick test of this using drush quick-drupal, PHP 5.6.25. I could not reproduce the issue with menu items. The problem with tags showing up in English after clearing caches from a non-english admin UI did occur, though, and the patch fixed it. Great!

The menu problem may have been fixed since we saw it (around Drupal 8.0.3 or so, if I remember correctly), or it may still be a problem that requires other steps to be reproduced.

vijaycs85’s picture

Thanks for testing it @badrange

The problem with tags showing up in English after clearing caches from a non-english admin UI did occur, though, and the patch fixed it. Great!

Thanks for confirming.

The menu problem may have been fixed since we saw it (around Drupal 8.0.3 or so, if I remember correctly), or it may still be a problem that requires other steps to be reproduced.

In any case we can't reproduce the issue for now.

Gábor Hojtsy’s picture

All right @grahl, are you sure you are talking about the same problem?

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Given no response from @grahl and the severity of this bug, let's get this in, since it does fix some of these problems at least if not all of them and in a nice way and with tests :) So let's get it in!

grahl’s picture

Sorry, I'll retest and otherwise a separate ticket, when I get the time. Thanks for your work!

catch’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/modules/config_translation/src/Tests/ConfigTranslationCacheTest.php
    @@ -0,0 +1,183 @@
    +    $filter_test_format = entity_load('filter_format', 'filter_test');
    

    Minor but we're down to 8 calls to entity_load() in core, this should use FilterFormat::load()

  2. +++ b/core/modules/views/src/Routing/ViewPageController.php
    @@ -61,4 +102,22 @@ public function handle($view_id, $display_id, RouteMatchInterface $route_match)
    +   *
    +   * Using callback as direct call to getTitle(() in route definition causing
    +   * issues with configuration translation system.
    

    Does the comment need to describe the bug report/previous behaviour? Should either remove it or explain that we don't want to call getTitle() while building routes since that in turn loads config overrides or similar. At the moment this isn't very helpful.

vijaycs85’s picture

FileSize
2.91 KB
18.48 KB

Thanks @catch.

#54.1 - Updated.
#54.2 - Not sure if it the updated text makes sense now. Moved it to route so that it's much clear?

vijaycs85’s picture

Status: Needs work » Needs review
anemes’s picture

Hello,

I spent a couple of hours on this issue and my conclusion is that when we clear the cache from UI there is a container rebuild which recreates all the services from the container. This is ok, but the LanguageManager is not initialized with the ConfgOverrideLanguage for the current language.

At the begining of the current request the LanguageManager was initialized with the current language because of the class LanguageRequestSubscriber is subscribed to the event KernelEvents::REQUEST which is called from the HttpKernel.

Since after the container rebuild there is no event dispatched LanguageManager stays with the default language and there is no and the config cache is saved with no override.

I will work on a patch today for this thing. Let me know if it makes sense what I wrote here.

anemes’s picture

Status: Needs review » Needs work

The last submitted patch, 58: 2650434-58.patch, failed testing.

anemes’s picture

anemes’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 60: 2650434-60.patch, failed testing.

anemes’s picture

Status: Needs work » Needs review
FileSize
4.8 KB

Status: Needs review » Needs work

The last submitted patch, 63: 2650434-63.patch, failed testing.

anemes’s picture

Status: Needs work » Needs review
FileSize
4.46 KB

Status: Needs review » Needs work

The last submitted patch, 65: 2650434-65.patch, failed testing.

anemes’s picture

Status: Needs work » Needs review
FileSize
4.8 KB
Gábor Hojtsy’s picture

@anemes: how does that relate to the work from @vijaycs85? Is the patch from @vijaycs85 just fixing one root cause and fixing it by chance because others don't use the language overrides system yet in the codepath? Or are the two problems separate? Your comments do not seem to explain this and you kind of took this issue over so it would be nice to clear this up.

anemes’s picture

I believe the fix I made is for the steps in the section "Steps to reproduce" and this was fixing it.

So in my opinion the problem is in the fact that when all the services get rebuild they are not recreated as they were at the beginning of the request by dispatching events like KernelEvents::REQUEST (in our case). This leaves some of the services stripped down of some functionalities which in our case are needed (the LanguageManager needs to have the current language set in order to work properly and it was only added once when the KernelEvents::REQUEST event is dispatched).

There might be also other issues generated by the fact that we expect the services to be initialized completely after the container is rebuilt. I only ran over this one yet.

I hope I answered your question.

bboty’s picture

I reproduced the translation issue from the section "steps to reproduce" and after applying the patch made by @anemes it was fixed, according to my tests.

I ran into a practical problem that could've become serious if one used url language detection with "domain" name settings selected. (see at 'admin/config/regional/language/detection/url').
Since there are separate domains for each language, if an admin is logged in on domain of a translated version's interface and goes to clear caches, the current language (that was not the default one) translations used to get broken.

vijaycs85’s picture

FileSize
10.7 KB
5.91 KB

Thanks @anemes. I was looking for this sort of solution, but wasn't able to get the point where you dispatch new event. I just added the test case from my patch.

P.S: We might need change change record and adding an event is an API addition I believe.

anemes’s picture

@vijaycs85: did you run your tests with the patch and passed?

I didn't make any tests yet because I was waiting for the approval for the solution.

I didn't understand what you wanted to say with the last part of your comment (The P.S.).

vijaycs85’s picture

Yes patch in #71 has tests. Ignore the P.S part. Looks like We do not consider event as API change.

Gábor Hojtsy’s picture

+++ b/core/lib/Drupal/Core/DrupalKernel.php
@@ -892,6 +892,7 @@ protected function initializeContainer() {
     // The request stack is preserved across container rebuilds. Reinject the
     // new session into the master request if one was present before.
+    $request = NULL;
     if (($request_stack = $this->container->get('request_stack', ContainerInterface::NULL_ON_INVALID_REFERENCE))) {
       if ($request = $request_stack->getMasterRequest()) {

Why do we need to set this to NULL here? How does this change what is already there?

anemes’s picture

The call might not pass the part with if (($request_stack = $this->container->get('request_stack', ContainerInterface::NULL_ON_INVALID_REFERENCE))) in which case the chec if ($request) would generate a Notice with undefined variable.

alexpott’s picture

  1. Something feels odd here - persisting information across container rebuilds has always been painful :(
  2. +++ b/core/lib/Drupal/Core/DrupalKernel.php
    @@ -905,7 +906,9 @@ protected function initializeContainer() {
    +    if ($request) {
    +      $this->container->get('event_dispatcher')->dispatch(self::CONTAINER_INITIALIZE_FINISHED);
    +    }
    

    Why only if there is a request?

  3. +++ b/core/lib/Drupal/Core/DrupalKernelInterface.php
    @@ -15,6 +15,15 @@
    +   * Thde CONTAINER_INITIALIZE_FINISHED event occurs when the service container finished initializing.
    

    "Thde"? And longer than 80 chars

  4. +++ b/core/lib/Drupal/Core/DrupalKernelInterface.php
    @@ -15,6 +15,15 @@
    +   * This event allows you to initialize overrides such as language to the services.
    

    Longer than 80 chars

  5. +++ b/core/modules/config_translation/src/Tests/ConfigTranslationCacheTest.php
    @@ -0,0 +1,184 @@
    +  public static $modules = [
    +    'block',
    +    'config_translation',
    +    'config_translation_test',
    +    'contact',
    +    'contact_test',
    +    'contextual',
    +    'entity_test',
    +    'field_test',
    +    'field_ui',
    +    'filter',
    +    'filter_test',
    +    'node',
    +    'views',
    +    'views_ui',
    +  ];
    ...
    +  protected $langcodes = array('fr', 'ta');
    

    Let's not mix array styles.

vasi’s picture

Thanks so much for your work on this, vijaycs85.

For anybody who needs a quick-and-dirty workaround, this is working for me so far:

function mymodule_rebuild() {
  $lm = \Drupal::languageManager();
  $lm->setConfigOverrideLanguage($lm->getCurrentLanguage());
}
anemes’s picture

FileSize
0 bytes

I added the changes requested by alexpott.

anemes’s picture

FileSize
1.34 KB

Status: Needs review » Needs work

The last submitted patch, 78: 2650434-78.patch, failed testing.

dan2k3k4’s picture

@anemes your last patch is empty file ?

chipway’s picture

Status: Needs work » Needs review
FileSize
10.25 KB
2.36 KB

New patch including comments from #74.

Status: Needs review » Needs work

The last submitted patch, 82: 2650434-82.patch, failed testing.

anemes’s picture

Sorry about the patch, I didn't upload the right file. The interdiff was correct, missed the patch file.

vijaycs85’s picture

Thanks for the update @anemes. I think we still need to answer @alexpott's questions #76.1 and #76.2. I tried my level best to answer them, but feel free to correct me if I'm wrong.

#76.1: Main reason is the order of events and availability of language component. We are getting far later than we needed.
#76.2: We want to trigger the new event only for valid requests.

KevinVb’s picture

Subscribe.
I had this issue as well. The patch solves this.

Arnoldski’s picture

Also had the same issue. Patch #84 fixed it.

andypost’s picture

  1. +++ b/core/modules/language/src/EventSubscriber/LanguageRequestSubscriber.php
    @@ -64,25 +65,38 @@ public function __construct(ConfigurableLanguageManagerInterface $language_manag
    +   *  Sets the request on the language manager.
    +   */
    +  private function setLanguageOverrides() {
    

    Comment is totally wrong

  2. +++ b/core/modules/language/src/EventSubscriber/LanguageRequestSubscriber.php
    @@ -64,25 +65,38 @@ public function __construct(ConfigurableLanguageManagerInterface $language_manag
    +    $this->negotiator->reset();
    

    no reason to call that cos setCurrentUser() doing reset

  3. +++ b/core/modules/language/src/EventSubscriber/LanguageRequestSubscriber.php
    @@ -64,25 +65,38 @@ public function __construct(ConfigurableLanguageManagerInterface $language_manag
    +    $langcode = $this->languageManager->getCurrentLanguage()->getId();
    +    $this->translation->setDefaultLangcode($langcode);
    

    this was not called in request subscriber

andypost’s picture

A bit of clean-up
1) when you fire event it should not happen conditionally
2) clean up of request better to separate to another issue #2839396: Clean-up unused variable and useless method call from LanguageRequestSubscriber
3) initialization of language manager here with default language looks wrong

This way fixes the bug but I sure firing the event is not proper way to solve that

Suppose there should be some event when container rebuild on cache clear

Status: Needs review » Needs work

The last submitted patch, 89: 2650434-89.patch, failed testing.

andypost’s picture

Status: Needs work » Needs review
FileSize
1.35 KB
10.27 KB

reverting a setting language for string translation

The last submitted patch, 89: 2650434-89.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 91: 2650434-91.patch, failed testing.

andypost’s picture

robert-os’s picture

hkirsman’s picture

This fixes also bug in 8.2.5 but some of the code is already there so the patch fails. Only file that needs patching is core/modules/language/src/EventSubscriber/LanguageRequestSubscriber.php
If I would need patch against current version, where should I post this so my composer could download it? Older versions seems to be working for 2.x too. Will test. Are they ok to use?

hkirsman’s picture

Hm, 2650434-89.patch works for me. After clearing cache the correct translation for field label is used. Do you think it's safe to use?

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
Issue tags: +SprintWeekend2017

Sending to testbot.

John Cook’s picture

Status: Needs review » Reviewed & tested by the community

I've tested against 8.4.x branch and the patch works as planned.

I followed the reproduction instructions in the description and Cottser #2.

Before the patch
When clearing the cache through the localized performance page the labels are reset to English (default language) .
Cottser's work around of clearing the cache with the non-localized performance page works.

After applying the patch from #95
Clearing the cache through the localized performance page the labels remain in the localized language.

Setting to RTBC.

Gábor Hojtsy’s picture

@tikaszvince also experienced this bug on a site. I sent him this issue on the Sprint Weekend. He tried and confirmed it worked for him. I am not sure if he did any code review.

TwoD’s picture

I can verify #95 works for us as well.
We've had major issues with this when clearing caches on non-English pages or from Drush, but with this patch in place we have no problems with either.

I have not had time to look at the tests, but at least found a minor code-style issue with an extra space here:

+++ b/core/modules/language/src/EventSubscriber/LanguageRequestSubscriber.php
@@ -64,25 +65,39 @@ public function __construct(ConfigurableLanguageManagerInterface $language_manag
+   *  Sets the language for config overrides on the language manager.
TwoD’s picture

A few comment changes only.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 103: interdiff-95-103.diff, failed testing.

chipway’s picture

Status: Needs work » Needs review
FileSize
2.81 KB

Thanks TwoD,
I suggest renaming your interdiff as interdiff-2650434-95-103.txt to avoid tests run on it.

TwoD’s picture

Yeah, I was just about to apologize for that but got an error when submitting because of your post hehe.
Btw, Is there any chance this could get in sooner than 8.4?
Seems there's only API additions and it fixes a fairly annoying and confusing problem.

tstoeckler’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
10.26 KB

Comment updates look fine, so re-RTBCing.
Also re-uploading the patch so that it will get continuously re-tested. (And not your interdiff ;-))

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 107: 2650434-103.patch, failed testing.

andypost’s picture

Status: Needs work » Reviewed & tested by the community
catch’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs issue summary update

The fix makes sense to me, but two things:

+++ b/core/lib/Drupal/Core/DrupalKernel.php
@@ -912,6 +912,9 @@ protected function initializeContainer() {
     \Drupal::setContainer($this->container);
+    if ($request) {
+      $this->container->get('event_dispatcher')->dispatch(self::CONTAINER_INITIALIZE_FINISHED);
+    }

This needs an explanatory comment.

Could also do with some of the information in #69 added to the issue summary.

Jeroen_005’s picture

Patch #107 works fine for me, but after importing new po files (admin/config/regional/translate/settings) those config translations get lost again.
Even when "Don't overwrite existing translations." is selected.

Does anyone can reproduce this issue?

pguillard’s picture

If anybody has a suggestion to the explanatory comment asked by @catch, I would be pleased to add a patch there, cause we need that patch into core !

@Jeroen_005 : I guess the trouble here is not correlated to .po files, as interface translations is configuration, not translation strictly speaking.

dawehner’s picture

Here is a patch against the 8.2.x branch

pguillard’s picture

@Daniel : There is some strange docroot stuff in your patch ?
diff --git a/docroot/core/lib/Drupal/Core/DrupalKernel.php b/docroot/core/lib/Drupal/Core/DrupalKernel.php

I just removed that in this new one.

Will test that soon.

pguillard’s picture

Status: Needs work » Needs review

I confirm the problem is solved.

  • Tested on the project I work on everyday, based on 8.2.5
  • I also applied the "Steps to reproduce" to a clean 8.2.x HEAD install

=> Whatever the current language I'm on, a clear cache no longer affects the translation of my forms and labels.

I can't set that to RTBC by myself, at least to "Needs review".

The last submitted patch, 113: 2650434-d8--2.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 114: 2650434--114.patch, failed testing.

Yogesh Pawar’s picture

Status: Needs work » Needs review
FileSize
10.25 KB

Rerolled the patch against 8.4.x branch because existing patch failed to apply on 8.4.x branch.

Status: Needs review » Needs work

The last submitted patch, 118: clearing_cache_via_ui-2650434-118.patch, failed testing.

andypost’s picture

+++ b/core/lib/Drupal/Core/DrupalKernel.php
@@ -912,6 +912,9 @@ protected function initializeContainer() {
+    if ($request) {
+      $this->container->get('event_dispatcher')->dispatch(self::CONTAINER_INITIALIZE_FINISHED);

and this still needs comment about why request could be empty

vijaycs85’s picture

@andypost, trying to test without $request check - #1970534-111: Patch testing issue without polluting this issue.

davidraijmakers’s picture

FileSize
10.78 KB

I rewrote the patch for 8.3.0-rc2

esolitos’s picture

Being such a serious and major bug, shouldn't this go directly in 8.3.x anyway?

dawehner’s picture

+++ b/core/modules/config_translation/src/Tests/ConfigTranslationCacheTest.php
@@ -0,0 +1,184 @@
+
+    // Add languages.
+    foreach ($this->langcodes as $langcode) {
+            ConfigurableLanguage::createFromLangcode($langcode)->save();
+          }
+    $this->drupalPlaceBlock('local_tasks_block');
+    $this->drupalPlaceBlock('page_title_block');
...
+  /**
+     * Tests the translation of field and field storage configuration.
+     */
+  public function testFieldConfigTranslation() {
+        // Add a test field which has a translatable field setting and a
+        // translatable field storage setting.
+        $field_name = strtolower($this->randomMachineName());
+        $field_storage = FieldStorageConfig::create([
+            'field_name' => $field_name,
+            'entity_type' => 'entity_test',
+            'type' => 'test_field',
+          ]);
+
+        $translatable_storage_setting = $this->randomString();

Please fix the coding styler in this file (using phpcbf would help you a lot here). Problems are for example: 4/8 spaces instead of 2

eiriksm’s picture

Assigned: Unassigned » eiriksm

Working on this

eiriksm’s picture

Assigned: eiriksm » Unassigned
Status: Needs work » Needs review
Issue tags: +DevDaysSeville
FileSize
10.18 KB

Fixed up the coding standards.

Also added a comment about the request variable, although I am not really sure why we check for it.

Status: Needs review » Needs work

The last submitted patch, 126: clearing_cache_via_ui-2650434-126.patch, failed testing.

eiriksm’s picture

Status: Needs work » Needs review
FileSize
10.23 KB
eelkeblok’s picture

Status: Needs review » Needs work
+++ b/core/lib/Drupal/Core/DrupalKernel.php
@@ -912,6 +912,11 @@ protected function initializeContainer() {
+    // If there is a request stack, we want to make sure other parts of the
+    // codebase can react to this.

It looks like this is based on #75. However, that seems to be about a different part of the code. The if ($request) was introduced in #67 by @anemes and seems to be a direct response to the test failures in #65, which are erroring out with:

exception: [Recoverable error] Line 37 of core/lib/Drupal/Core/Routing/RequestContext.php:
Argument 1 passed to Drupal\Core\Routing\RequestContext::fromRequest() must be an instance of Symfony\Component\HttpFoundation\Request, null given. 

If you go through the stack trace, the origin is indeed that call wrapped in the if. Now to explain that in a concise way in a comment...

Regnoy’s picture

Patch worked for me, thanks

danielnolde’s picture

Patch #128 fixes the problem for me on 8.3.1 – boy, i'm glad t have stumbled over this discussion just in time, made it barely to #8 by myself.

dan2k3k4’s picture

Adding DrupalCon Baltimore 2017 tag: Baltimore2017

cbccharlie’s picture

Patch #128 fixes the problem for me on 8.3.2.

mpp’s picture

Patch #128 fixes the problem for me on 8.3.2.

duozersk’s picture

It looks like the event @anemes suggested as a way to fix the issue should only fire when the container is rebuilt in a subrequest.
So I renamed the event from CONTAINER_INITIALIZE_FINISHED to CONTAINER_INITIALIZE_SUBREQUEST_FINISHED and moved its dispatching into the code block where the new session is being injected into the master request (exactly checking for the condition that we are in a subrequest).

Also corrected the array syntax to use the short syntax.

Status: Needs review » Needs work

The last submitted patch, 135: clearing_cache_via_ui-2650434-135.patch, failed testing. View results

duozersk’s picture

Moved event dispatching to the previous place.

andypost’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me

+++ b/core/lib/Drupal/Core/DrupalKernel.php
@@ -928,6 +929,12 @@ protected function initializeContainer() {
+    if (isset($subrequest) && $subrequest) {

imo !empty($is_subrequest) better but that's naming

r4arnys’s picture

So will this be fixed in 8.4 (because I still have this very annoying problem in 8.3.5)?

I'm a bit surprised that this is still an issue over a year later as it seems such a severe fundamental issue. I really don't want to get involved in patches if I don't have to.

vijaycs85’s picture

Looks good to me. +1 to RTBC.

Regnoy’s picture

+1 testes

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 137: clearing_cache_via_ui-2650434-137.patch, failed testing. View results

andypost’s picture

Status: Needs work » Reviewed & tested by the community

bot flux

Version: 8.4.x-dev » 8.5.x-dev

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

vijaycs85’s picture

Sent the patch in #137 to test against 8.5.x just in case.

catch’s picture

Version: 8.5.x-dev » 8.4.x-dev
Component: config_translation.module » base system
Status: Reviewed & tested by the community » Fixed
FileSize
571 bytes

Fixed the empty() on commit, see interdiff.

Committed/pushed to 8.5.x and cherry-picked to 8.4.x. Thanks!

I think we already have an issue to review subrequest usage, I don't really think they live up to expectations and we should look at dropping the concept if we can.

  • catch committed 5ba1f0e on 8.5.x
    Issue #2650434 by vijaycs85, anemes, duozersk, andypost, eiriksm, TwoD,...

  • catch committed 8a36b21 on 8.4.x
    Issue #2650434 by vijaycs85, anemes, duozersk, andypost, eiriksm, TwoD,...
esolitos’s picture

So many Kudos to getting this finally in!

Wim Leers’s picture

Status: Fixed » Active
Issue tags: +Needs followup
  1. +++ b/core/lib/Drupal/Core/DrupalKernel.php
    @@ -928,6 +929,12 @@ protected function initializeContainer() {
    +      $this->container->get('event_dispatcher')->dispatch(self::CONTAINER_INITIALIZE_SUBREQUEST_FINISHED);
    
    +++ b/core/lib/Drupal/Core/DrupalKernelInterface.php
    @@ -15,6 +15,16 @@
    +  const CONTAINER_INITIALIZE_SUBREQUEST_FINISHED = 'kernel.container.finish_container_initialize_subrequest';
    

    This is a pretty significant new API.

    Shouldn't this get a change record?

    Or is this meant to be an @internal event?

  2. +++ b/core/lib/Drupal/Core/DrupalKernelInterface.php
    @@ -15,6 +15,16 @@
    +   * This event allows you to initialize overrides such as language to the
    +   * services.
    

    "language to the services" ?

  3. +++ b/core/modules/language/src/EventSubscriber/LanguageRequestSubscriber.php
    @@ -91,6 +106,7 @@ public function onKernelRequestLanguage(GetResponseEvent $event) {
    +    $events[DrupalKernelInterface::CONTAINER_INITIALIZE_SUBREQUEST_FINISHED][] = ['onContainerInitializeSubrequestFinished', 255];
    

    Why this service priority?

Reopened for point 1, the other points probably need a follow-up.

prestonso’s picture

Issue tags: +8.4.0 release notes

Tagging for inclusion in 8.4.0-beta1 release notes.

vijaycs85’s picture

Issue tags: -sprint, -Needs followup

Hi @Wim Leers:

This is a pretty significant new API.

Shouldn't this get a change record?

Or is this meant to be an @internal event?

Yeah it is, I just added a change record (https://www.drupal.org/node/2902228). I don't think it's internal as the issue could cause to any service (in core, it's just language override)

for #150.2 and 3, created a follow up at #2902230: Cleanup code and comment around CONTAINER_INITIALIZE_SUBREQUEST_FINISHED event

andypost’s picture

Status: Active » Fixed

CR filed & published, also follow-up created