New feature: Improved jstranslator.xml capabilities #56

justinbeaty · 2024-11-21T18:11:06Z

Description

Improved the jstranslator.xml syntax to allow including strings based on the scope, layout handle, and/or script included in <head>. Also added support very basic sprintf like syntax in the JS Translate class.

Rationale

I wanted to utilize the jstranslator.xml feature, but it has one very glaring problem: every string is included on every page load on both adminhtml and frontend. So if I add 10 new strings meant only for the admin panel, they'll also be included on the frontend. This could even be a security issue if you include strings that leak information about the server setup or installed modules.

Of course, this only happens if you're on a language pack other than English, so to illustrate the problem comment out the unset() line below. This simulates a language pack with all strings defined.

maho/app/code/core/Mage/Core/Helper/Js.php

Lines 143 to 146 in fddaaf9

    
           foreach ($this->_translateData as $key => $value) { 
        
               if ($key == $value) { 
        
                   unset($this->_translateData[$key]); 
        
               }

Then view the source of the homepage, and you'll see messages that are only used in the admin panel. I highlighted an example where it could be a security issue.

With this PR the string count goes from 70 -> 41, and will prevent future strings from being included unnecessarily.

Solution

I added some new syntax options to jstranslator.xml, here is what's possible:

<?xml version="1.0"?>
<jstranslator>

    <!-- Legacy style, backwards compatible with existing XML files -->
    <my-string-1 translate="message" module="core">
        <message>This string is included everywhere</message>
    </my-string-1>

    <!--
         Global scope node, this is equivalent to the legacy style,
         except you only need to define the module="" attribute once.

         Note: the module="" attribute is always optional, defaults to 'core'
    -->
    <global module="core">
        <my-string-2 translate="message">
            <message>This string is also included everywhere</message>
        </my-string-2>
        <my-string-3 translate="message">
            <message>Another global string</message>
        </my-string-3>
        <!-- You can override the parent module attribute if you like -->
        <my-string-4 translate="message" module="catalog">
            <message>Global string translated with another module</message>
        </my-string-4>
    </global>

    <!-- Frontend only -->
    <frontend module="core">
        <my-string-4 translate="message">
            <message>This string is included only on the frontend</message>
        </my-string-4>
    </frontend>

    <!-- Adminhtml only -->
    <adminhtml module="core">
        <my-string-5 translate="message">
            <message>This string is included only on the admin panel</message>
        </my-string-5>
    </adminhtml>

    <!--
         Layout node, include only if the handle="" attribute matches the current layout

         Example handles:
           - STORE_default
           - THEME_frontend_rwd_default
           - catalog_product_view
           - PRODUCT_TYPE_configurable
           - customer_logged_in
           - cms_page
           - cms_index_index
           - etc

         Note: you can specify multiple handles by separating them with a comma
    -->
    <layout handle="cms_index_index" module="core">
        <my-string-6 translate="message">
            <message>This string is included only on the home page</message>
        </my-string-6>
    </layout>

    <!--
        Script node, include only if the script was added to the head block.
        This ONLY detects scripts added to the head block with addJs or addItem, it does not
        detect hardcoded <script src=""></script> tags.

        path="" attribute should match the parameter passed to addJs or addItem
        type="" attribute can be either 'js' or 'skin_js', defaults to 'js' if omitted

        Note: you can specify multiple scripts by separating them with a comma, but they all must be of the same type
    -->
    <script path="validation.js" type="js" module="core">
        <my-string-7 translate="message">
            <message>This string is only included if validation.js is loaded</message>
        </my-string-7>
    </script>

    <script path="js/msrp_rwd.js,js/opcheckout_rwd.js" type="skin_js" module="core">
        <my-string-8 translate="message">
            <message>This string is only included if msrp_rwd.js or opcheckout_rwd.js is loaded</message>
        </my-string-8>
    </script>

    <!--
         Scope attribute, you can combine the legacy style, layout, and script nodes with the
         area="" attribute to further limit loading strings
    -->
    <script path="validation.js" area="adminhtml" module="adminhtml">
        <my-string-9 translate="message">
            <message>This string is only included if validation.js is loaded AND we're on the admin panel</message>
        </my-string-9>
    </script>

</jstranslator>

Additionally, you can add messages manually with PHP code. You could do this either in a controller or block class, as long as it's before the head block is rendered (i.e. don't do it in a blocks _toHtml() method).

// Add a single string translated with the 'core' module
Mage::helper('core/js')->addTranslateData('Some string'); 

// Add multiple strings translated with the 'adminhtml' module
Mage::helper('core/js')->addTranslateData([
    'My string 1',
    'My string 2',
    'My string 3',
], 'adminhtml');

Of course, you can also add messages in phtml files as before:

<script>
    Translator.add('Some string', '<?= $this->__('Some string') ?>');

    Translator.add({
        'My string 1': '<?= $this->__('My string 1') ?>',
        'My string 2': '<?= $this->__('My string 2') ?>',
        'My string 3': '<?= $this->__('My string 3') ?>',
    });
</script>

Basic sprintf compatibility

I also added a very basic sprintf feature to the JS translate class. It only matches %s and %d tokens, but that's pretty much all that is ever used in Magento translation files. Example:

Translator.translate('Please enter a password with at most %s characters.', 256);

Comments

Most of the still included strings on the frontend are from validation.js. I moved some that are clearly admin only to Mage/Adminhtml/etc/jstranslator.xml.

I also included a debug commit here that will be reverted. It simply includes all messages without a translation pack and pretty prints the JSON array in the source code.

fballiano · 2024-11-21T18:32:59Z

Hey @justinbeaty, the backend/frontend separation is very nice and needed, but I'm not super about the advanced features because if users use a full page cache, it will probably break everything right?

justinbeaty · 2024-11-21T18:46:51Z

Hey @justinbeaty, the backend/frontend separation is very nice and needed, but I'm not super about the advanced features because if users use a full page cache, it will probably break everything right?

While I don't have much experience with FPC and Magento, I'm not sure I see how it could break. FPC modules must cache the head block per page, right?

Here's the head block's phtml on the frontend:

maho/app/design/frontend/base/default/template/page/html/head.phtml

Lines 23 to 26 in fddaaf9

    
           <?= $this->getCssJsHtml() ?> 
        
           <?= $this->getChildHtml() ?> 
        
           <?= $this->helper('core/js')->getTranslatorScript() ?> 
        
           <?= $this->getIncludes() ?>

So if $this->getCssJsHtml() is cached, then so is $this->helper('core/js')->getTranslatorScript().

The only potential case I could see is if the FPC module ignores the loaded layout handles. I.e. customer_logged_out vs customer_logged_in. But if that's the case, then the FPC cache module is already broken, because we can (and do) include different JS files based on that handle. In this case, varien/weee.js wouldn't be loaded, but neither would any strings associated with that script, so at least here it's not an issue.

maho/app/design/frontend/base/default/layout/weee.xml

Lines 33 to 37 in fddaaf9

    
           <customer_logged_in> 
        
               <reference name="head"> 
        
                   <action method="addItem"><type>js</type><name>varien/weee.js</name></action> 
        
               </reference> 
        
           </customer_logged_in>

Is there a specific scenario you can think of that would break things?

justinbeaty · 2024-11-21T19:10:42Z

Put another way, for each page on the frontend:

The same JS files are always loaded
The same layout handles are always loaded (except customer_logged_in and customer_logged_out).

Thus for each page, the same strings are included whether FPC is enabled or not.

fballiano · 2024-11-21T19:15:58Z

if I remember correctly some blocks are cache site wise, but probably you're right and the head block is page-based anyway (eg: because javascripts are different between category and product page).

I was checking Lesti but it does seem to only work on a page-basis. Maybe I was remembering stuff from Turpentine or the Magento EE full page cache module that's not relevant anymore.

justinbeaty · 2024-11-21T19:28:17Z

So then I think we can agree the strings must be correct for the <script path=""> type node.

It's still possible you could break things with layout handles. As a completely dumb example you add a randomly generated handle to each page load dynamically from your controller file, and somehow expect to add strings based on that. But I think the solution is, don't do that if you're using FPC. Which isn't unreasonable since there are other similar things you can't do when using FPC.

justinbeaty · 2024-11-21T19:31:04Z

if I remember correctly some blocks are cache site wise, but probably you're right and the head block is page-based anyway (eg: because javascripts are different between category and product page).

Also I would assume the head block is page-based since the <title> tag is in there too.

app/code/core/Mage/Core/Helper/Js.php

app/code/core/Mage/Core/Model/Translate/Config.php

justinbeaty · 2024-11-21T21:52:40Z

Reverted the debugging commit, however you can apply it with:

diff --git a/app/code/core/Mage/Core/Helper/Js.php b/app/code/core/Mage/Core/Helper/Js.php
index 0c23ef6e64..0e50d47ae4 100644
--- a/app/code/core/Mage/Core/Helper/Js.php
+++ b/app/code/core/Mage/Core/Helper/Js.php
@@ -51,7 +51,8 @@ class Mage_Core_Helper_Js extends Mage_Core_Helper_Abstract
      */
     public function getTranslateJson()
     {
-        return Mage::helper('core')->jsonEncode($this->_getTranslateData());
+        // return Mage::helper('core')->jsonEncode($this->_getTranslateData());
+        return json_encode($this->_getTranslateData(), JSON_PRETTY_PRINT);
     }

     /**
@@ -132,8 +133,8 @@ class Mage_Core_Helper_Js extends Mage_Core_Helper_Abstract
         $messageText = is_array($messageText) ? $messageText : [$messageText];
         foreach ($messageText as $text) {
             $translated = Mage::helper($module)->__($text);
-            if ($text && $text !== $translated) {
-                $this->_translateData[$text] = $translated;
-            }
+            // if ($text && $text !== $translated) {
+            $this->_translateData[$text] = $translated;
+            // }
         }
     }

fballiano · 2024-11-22T00:50:30Z

i've added the debug code locally to test and everything works beatifully, I though i've seen something wrong about the configurableswatches string but I was checking the category page instead of the product page, so, again, everything is perfect ❤️

fballiano · 2024-11-22T00:52:33Z

I'll put your documentation in the release notes but I'll also have to create a documentation page on the website for this

justinbeaty · 2024-11-22T00:53:38Z

I'll put your documentation in the release notes but I'll also have to create a documentation page on the website for this

Sweet! At some point I'll probably help with the docu too.

justinbeaty added 7 commits November 21, 2024 07:39

Improve jstranslator.xml syntax

689d6c6

Update jstranslator.xml files

85372eb

Debug: will revert

b9159b2

Move some validation.js messages to adminhtml only

7c7015f

Add very simple sprintf like support

5b29187

phpstan, debug commit

e3a3e82

phpcs, debug commit

c317a96

Merge branch 'main' into topic-jstranslator

fffbdbe

fballiano reviewed Nov 21, 2024

View reviewed changes

app/code/core/Mage/Core/Helper/Js.php Outdated Show resolved Hide resolved

fballiano reviewed Nov 21, 2024

View reviewed changes

app/code/core/Mage/Core/Model/Translate/Config.php Show resolved Hide resolved

justinbeaty added 2 commits November 21, 2024 11:45

add void return type

3869cc5

Revert test commit

51cad6f

fballiano changed the title ~~Improved jstranslator.xml syntax~~ New feature: Improved jstranslator.xml capabilities Nov 22, 2024

fballiano merged commit 8b8f8cb into MahoCommerce:main Nov 22, 2024
16 checks passed

justinbeaty mentioned this pull request Nov 23, 2024

Added new events: controller_action_predispatch_$routeName_$controllerName and controller_action_postdispatch_$routeName_$controllerName #57

Merged

justinbeaty mentioned this pull request Dec 16, 2024

Removed prototypejs from opcheckout.js and the whole frontend #71

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New feature: Improved jstranslator.xml capabilities #56

New feature: Improved jstranslator.xml capabilities #56

justinbeaty commented Nov 21, 2024 •

edited

Loading

fballiano commented Nov 21, 2024

justinbeaty commented Nov 21, 2024 •

edited

Loading

justinbeaty commented Nov 21, 2024

fballiano commented Nov 21, 2024

justinbeaty commented Nov 21, 2024 •

edited

Loading

justinbeaty commented Nov 21, 2024

justinbeaty commented Nov 21, 2024

fballiano commented Nov 22, 2024

fballiano commented Nov 22, 2024

justinbeaty commented Nov 22, 2024

	foreach ($this->_translateData as $key => $value) {
	if ($key == $value) {
	unset($this->_translateData[$key]);
	}

New feature: Improved jstranslator.xml capabilities #56

New feature: Improved jstranslator.xml capabilities #56

Conversation

justinbeaty commented Nov 21, 2024 • edited Loading

Description

Rationale

Solution

Basic sprintf compatibility

Comments

fballiano commented Nov 21, 2024

justinbeaty commented Nov 21, 2024 • edited Loading

justinbeaty commented Nov 21, 2024

fballiano commented Nov 21, 2024

justinbeaty commented Nov 21, 2024 • edited Loading

justinbeaty commented Nov 21, 2024

justinbeaty commented Nov 21, 2024

fballiano commented Nov 22, 2024

fballiano commented Nov 22, 2024

justinbeaty commented Nov 22, 2024

justinbeaty commented Nov 21, 2024 •

edited

Loading

justinbeaty commented Nov 21, 2024 •

edited

Loading

justinbeaty commented Nov 21, 2024 •

edited

Loading