Updated: Comment #7

Problem/Motivation

Static cache for language_list() is not cleared in _locale_rebuild_js()

Proposed resolution

The patch attached may work. However the file it is applied to no longer exists. A new patch should be created to work on the same function, which now lives in locale.module

Remaining tasks

The patch needs to be recreated for the proper file and tested

User interface changes

n/a

API changes

n/a

#1260510: Introduce a language_load($langcode)

Original report

There's a comment about consistency in runtime and database but we have a static cache for language_list() that is not cleared...

I found it working on #1260510: Introduce a language_load($langcode)
But suppose it should be fixed and backported to D7

CommentFileSizeAuthor
#17 drupal8_static_cache-1261212-17.patch2.1 KBstefank
#17 drupal8_static_cache_test_only-1261212-17.patch1.56 KBstefank
#15 drupal8_static_cache-1261212-15.patch2.13 KBstefank
#12 drupal8_static_cache_simpletest-1261212-11.patch1.63 KBpiyuesh23
#10 drupal8-static_cache_reroll-1261212-10.patch550 bytespwieck
#8 drupal8-static_cache_reroll-1261212.patch659 bytesKRaisor
locale_static.patch735 bytesandypost
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Status: Needs review » Needs work

The last submitted patch, locale_static.patch, failed testing.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Issue tags: +Needs backport to D7

Fixing tag.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Issue tags: +language-ui

Adding UI language translation tag.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented

Currently the logic here changed - language_delete call for invalidation (deferred clean-up), but actual _locale_rebuild_js() is used language_list() so deleted language files seems get's lost

so needs testing

YesCT’s picture

YesCT CreditAttribution: YesCT commented
Issue tags: +Needs tests, +Needs steps to reproduce, +Needs reroll

Probably needs a reroll.

Tests might be able to be written, even if a fix is not obvious.

An issue summary update using the issue summary template, and remaining tasks, might help.

#1260510: Introduce a language_load($langcode) might be useful to write steps to reproduce.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
Issue tags: +Needs issue summary update
andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented

probably this could be covered with unittest

KRaisor’s picture

KRaisor CreditAttribution: KRaisor commented
Status: Needs work » Needs review
FileSize
drupal8-static_cache_reroll-1261212.patch659 bytes

Patch re-rolled as the code has been significantly re-factored.

Status: Needs review » Needs work

The last submitted patch, drupal8-static_cache_reroll-1261212.patch, failed testing.

pwieck’s picture

pwieck CreditAttribution: pwieck commented
Status: Needs work » Needs review
FileSize
drupal8-static_cache_reroll-1261212-10.patch550 bytes

re-rolled with current head

pwieck’s picture

pwieck CreditAttribution: pwieck commented
Issue tags: -Needs reroll

#10 passed and ready for review.

piyuesh23’s picture

piyuesh23 CreditAttribution: piyuesh23 commented
FileSize
drupal8_static_cache_simpletest-1261212-11.patch1.63 KB

Attaching a simpletest written to test this case.

piyuesh23’s picture

piyuesh23 CreditAttribution: piyuesh23 commented
piyuesh23’s picture

piyuesh23 CreditAttribution: piyuesh23 commented
Issue summary: View changes

Adding summary template

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
CreditAttribution: jhedstrom commented
Issue summary: View changes
Status: Needs review » Needs work

The fix from #10 and the test from #12 should be in a single patch. It's also odd that the test passes without the fix.

stefank’s picture

stefank CreditAttribution: stefank commented
Status: Needs work » Needs review
FileSize
drupal8_static_cache-1261212-15.patch2.13 KB

Here is a patch with the fix and the test.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented

@stefank please file a separate patch without fix (test only) to make sure that the test covers the bug.
Also 2 nitpicks, check another tests to format class properly

+++ b/core/modules/locale/src/Tests/LocaleStaticTest.php
@@ -0,0 +1,43 @@
+  public static $modules = array('locale');
...
+  }
+}

needs empty line

stefank’s picture

stefank CreditAttribution: stefank commented
FileSize
drupal8_static_cache_test_only-1261212-17.patch1.56 KB
drupal8_static_cache-1261212-17.patch2.1 KB

Thanks @andypost.
I've corrected the format, but Im still not sure about the text- if it does make sense.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented 
+++ b/core/modules/locale/locale.module
@@ -1166,6 +1166,7 @@ function _locale_invalidate_js($langcode = NULL) {
+  drupal_static_reset('language_list');

Does this do anything anymore? language_list is just a deprecated wrapper now for the language manager. That does have an internal cache that could be reset, but this static reset will not do that right? So looks like the bug is gone, no? That the test only patch passes supports that too :)

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented

Yep, seems only d7 affected now or test is outdated

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
CreditAttribution: jhedstrom at Phase2 commented
Version: 8.0.x-dev » 7.x-dev
Status: Needs review » Needs work

Moving to 7 based on #19 and #18.