Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Resources: set language-specific cacheCode, fixes AngularJS translation with multilingual #22236

Merged
merged 1 commit into from
Dec 16, 2021

Conversation

mlutfy
Copy link
Member

@mlutfy mlutfy commented Dec 9, 2021

Overview

To reproduce:

  • Enable another language (does not need to be multi-lingual, just multiple enabled locales)
  • Under Admin>Debugging, enable asset caching.
  • Go to an AngularJS page (with asset caching enabled) in English, such as the CiviCRM System Status, or the CiviCRM dashboard.
  • Then switch languages, reload the AngularJS page

Result: the page will still be in English.

Example URL on dmaster, after adding French, and enabling asset-caching (dmaster does not enable it by default, because debugging is enabled):

(because the core language switcher does not support AngularJS, fortunately the language-switcher extension does.)

Before

Pages are randomly translated:

image

After

Pages are always in the correct language.

Technical Details

My fix feels duct-tape-y. I don't know if it will cause other problems. Testing it in production for now. Opening the PR for discussion.

@civibot
Copy link

civibot bot commented Dec 9, 2021

(Standard links)

@civibot civibot bot added the master label Dec 9, 2021
$this->cacheCode = Civi::settings()->get($cacheCodeKey);
// AngularJS json partials are language-specific because they ship with the strings
// for the current language.
$this->cacheCode = Civi::settings()->get($cacheCodeKey) . CRM_Core_I18n::getLocale();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I like the general idea of incorporating the locale into the browser-side cache-busting code. It seems like it could make several kinds of resources more amenable to multilingual.

Applying the trick in this spot means it applies once - at the start of the page-request. These values might rotate during the page-request (e.g. if the cache-code needs to be expired/reset, or if the locale changes).

Haven't tested, but I think this might be more responsive:

diff --git a/CRM/Core/Resources.php b/CRM/Core/Resources.php
index 0175befd74..4e93411b04 100644
--- a/CRM/Core/Resources.php
+++ b/CRM/Core/Resources.php
@@ -334,9 +334,11 @@ class CRM_Core_Resources implements CRM_Core_Resources_CollectionAdderInterface
 
   /**
    * @return string
+   *   A random-looking string that may be appended to resource-URLs.
+   *   This should be URL-safe (ie use alphanumerics and underscores).
    */
   public function getCacheCode() {
-    return $this->cacheCode;
+    return $this->cacheCode . CRM_Core_I18n::getLocale();
   }
 
   /**
@@ -563,7 +565,7 @@ class CRM_Core_Resources implements CRM_Core_Resources_CollectionAdderInterface
     $hasQuery = strpos($url, '?') !== FALSE;
     $operator = $hasQuery ? '&' : '?';
 
-    return $url . $operator . 'r=' . $this->cacheCode;
+    return $url . $operator . 'r=' . $this->getCacheCode();
   }
 
   /**

@mlutfy
Copy link
Member Author

mlutfy commented Dec 10, 2021

Thanks Tim! I tested your suggestion and it works as well, and makes more sense. Updated my PR.

@seamuslee001
Copy link
Contributor

@mlutfy test fails seem related

@mlutfy mlutfy force-pushed the angularCacheCodeLanguage branch 2 times, most recently from 2d4ff3b to 6ccd2ae Compare December 13, 2021 14:49
@mlutfy
Copy link
Member Author

mlutfy commented Dec 13, 2021

Yay tests pass, but I had to tweak them. Can someone double-check if it looks OK?

@totten
Copy link
Member

totten commented Dec 16, 2021

@mlutfy Patch looks good. Update to the tests makes perfect sense. Did an r-run (browsing civicrm/a/#/status and changing language) and confirmed that the language updates (both visually and in the source/URLs).

@totten totten merged commit 19a1a6e into civicrm:master Dec 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants