Improved calculating version hash for the js-translation.json file. #10378

hostep · 2017-07-30T08:47:22Z

Description

Currently the version hash which is used for the js-translation.js file is derived from its modification time on the filesystem + its relative directory name.
We've noticed on a project which is being deployed over multiple webservers behind a loadbalancer, that the outputted version differs on all the nodes. This is because the modification timestamp of the file is off by a couple of seconds on all the nodes.
I haven't seen any negative effects of the different version hashes, but I think it would be nice if the outputted html from all nodes is 100% identical, which it isn't right now.

I've changed the logic, so instead of using the timestamp of the file, it uses the contents of the file to calculate the version hash. This way, it will be the same on all the nodes and the outputted version will be the same every time you refresh your browser (and hit other nodes).

This problem occurs on both Magento CE 2.1.7 and 2.2.0-RC1.6

This pull request is a first quick version, to see if there is interest in this. I think the logic of calculating the hash should be moved over to the Block itself, instead of being done in the phtml file.
There should probably be a few tests added as well.
I'm not sure if this breaks the backwards compatibility rules, since it adds a public method to an @api annotated class?
If there is interest in this change, I'll try to improve this PR further.

Fixed Issues (if relevant)

None that I could find

Manual testing scenarios

Setup a new Magento CE 2.1.7 or 2.2.0-RC1.6 installation
View the html source of the homepage, you'll see a line:
if (versionObj.version !== '6c7483a300fdf2968a0d04aea293e882d9f5bf1e') {
Run touch pub/static/frontend/Magento/luma/en_US/js-translation.json so the modification timestamp of the file is changed
Flush your caches
View the html source of the homepage again, the version hash is changed, even though the file in itself wasn't changed

Contribution checklist

Pull request has a meaningful description of its purpose
All commits are accompanied by meaningful commit messages
All new or changed code is covered with unit/integration tests (if applicable)
All automated tests passed successfully (all builds on Travis CI are green)

ihor-sviziev · 2017-07-30T09:12:56Z

I think would be better to move hash preparation logic to one method. It will allow to customize via plugins
this logic based on own needs without modifying core files.

ishakhsuvarov · 2017-07-31T08:22:41Z

app/code/Magento/Translation/Model/FileManager.php

@@ -73,7 +73,7 @@ public function getTranslationFileTimestamp()
    /**
     * @return string
     */
-    protected function getTranslationFileFullPath()
+    public function getTranslationFileFullPath()


Change of method visibility may break potential customization or extension which inherited from this class.

ishakhsuvarov · 2017-07-31T08:55:20Z

app/code/Magento/Translation/Block/Js.php

+     */
+    public function getTranslationFileFullPath()
+    {
+        return $this->fileManager->getTranslationFileFullPath();


I'm not sure if this breaks the backwards compatibility rules, since it adds a public method to an @api annotated class?

This change indeed breaks backwards compatibility rules, but with existing block design this case becomes quite complicated to solve properly without adding a method.

If we are going to violate the rule for the specific case – I would suggest implementing this in the "desirable state" way, so it would cover all the theoretical cases which may come up in the future.

Briefly, this could be a method with more general name, something like getTranslationFileVersion, which would proxy the call to the dedicated model, which calculates the value (doing so in the template is not a best practice anyway). This way it becomes a more usable "extension point" for third-party developers to possibly customize the behavior and improves ability to maintain this code for the core team.

ishakhsuvarov · 2017-07-31T08:56:36Z

app/code/Magento/Translation/view/base/templates/translate.phtml

@@ -26,7 +26,7 @@
                $.initNamespaceStorage('mage-translation-file-version');
                versionObj = $.localStorage.get('mage-translation-file-version');

-                <?php $version = sha1($block->getTranslationFileTimestamp() . $block->getTranslationFilePath()); ?>
+                <?php $version = sha1(sha1_file($block->getTranslationFileFullPath()) . $block->getTranslationFilePath()); ?>


Hashes calculation and operations on the filesystem should not be a responsibility of the presentation layer.

ishakhsuvarov · 2017-07-31T08:57:26Z

@hostep Please check the integration tests that are failing.
Let us know if you need any assistance.
Thank you!

…te, still using the modified timestamp for now.

hostep · 2017-07-31T19:34:37Z

@ishakhsuvarov: I've just refactored the code in the way you guys want I think. I did revert the functionality to use the modified timestamp again temporarily for now, to see how the tests run on Travis (I have the impression that the tests run very unreliable on my machine, throwing all kinds of errors which have nothing to do with my changes)

…of the modified timestamp.

hostep · 2017-07-31T21:24:00Z

@ishakhsuvarov: I think I've fixed the integration tests now. It looks like when running the integration tests, that the js-translation.json file doesn't exist, this feels like a bug of the integration tests itself to me, no?

Worked around the failing tests by checking if the file exists and if not, using an empty string for the hash. This was also how it worked with the modified timestamp, if the file didn't exist, nothing was returned in the getTranslationFileTimestamp method.
All of this feels really wrong to me to be quite honest.

Are there cases where the file js-translation.json won't exist when the translate.phtml file is being rendered? Besides the tests?

(I was kind of able to get the integration tests to run locally like they do on Travis I think (very unsure about this). It would be great if the devdocs could get updated with how to do this properly, because the current method which is documented over there gives a lot of strange errors)

ishakhsuvarov · 2017-08-01T09:26:27Z

Are there cases where the file js-translation.json won't exist when the translate.phtml file is being rendered? Besides the tests?

Tests execution with the translation file absent seems strange to me as well. I have to do some research on this subject.

…into improve-translation-version-hash

…on.json file

ishakhsuvarov · 2017-08-08T13:26:58Z

@hostep I've attempted to add generation of the translations file, in case it does not exist. More of a draft at the moment. Please check and provide your opinion on it.
I would also create wrappers for sha1 and sha1_file and cover all this with tests.

hostep · 2017-08-15T12:10:40Z

@ishakhsuvarov: I'm going to trust what you are doing, the testing code part of Magento is not really my thing, so I don't really have an opinion about your changes, feel free to continue with this as you see fit :)

ishakhsuvarov · 2017-08-15T12:12:15Z

@hostep Thanks. Then i'll add some more stuff which should be there, from my perspective, and will let you know (cross-review is always helpful).

…into improve-translation-version-hash

…on.json file

…on.json file. #10378

[EngCom] Public Pull Requests - MAGETWO-71659: Fix for url_rewrite on page delete via api #10568 - MAGETWO-71617: Fixes Regression in 2.2 - menu.xml config ignored #10543 - MAGETWO-71380: Fix JS translation search #10445 - MAGETWO-71201: Improved calculating version hash for the js-translation.json file. #10378

hostep · 2017-08-18T07:19:47Z

Thanks for finishing and merging this PR @ishakhsuvarov!

ishakhsuvarov · 2017-08-18T07:20:58Z

@hostep Always happy to help. Thank you for collaboration!

ihor-sviziev · 2017-09-05T10:29:58Z

app/code/Magento/Translation/view/base/templates/translate.phtml

@@ -26,7 +26,7 @@
                $.initNamespaceStorage('mage-translation-file-version');
                versionObj = $.localStorage.get('mage-translation-file-version');

-                <?php $version = sha1($block->getTranslationFileTimestamp() . $block->getTranslationFilePath()); ?>
+                <?php $version = $block->getTranslationFileVersion(); ?>

                if (versionObj.version !== '<?= /* @noEscape */ $version ?>') {


Shoudn't we escape version there? Previously it was always sha1, but from now it uses raw value from public method, that could be overridden by plugin.
@ishakhsuvarov i think it should be escaped. what do you think?

Sounds completely reasonable to me

@ishakhsuvarov pull request prepared #10904

Improved calculating version hash for the js-translation.json file.

a1dab12

ishakhsuvarov self-assigned this Jul 30, 2017

ishakhsuvarov added the develop label Jul 30, 2017

ishakhsuvarov added this to the July 2017 milestone Jul 31, 2017

ishakhsuvarov requested changes Jul 31, 2017

View reviewed changes

okorshenko added the Progress: needs update label Jul 31, 2017

Refactored getting translation file version into a more desirable sta…

b3a1c7e

…te, still using the modified timestamp for now.

hostep added 2 commits July 31, 2017 22:57

Start using the file contents for calculating the hash again instead …

507cd42

…of the modified timestamp.

Fixes integration tests.

def3cff

ishakhsuvarov approved these changes Aug 1, 2017

View reviewed changes

magento-team added the develop label Aug 1, 2017

magento#10378: Merge branch 'develop' of github.com:magento/magento2 …

75990d4

…into improve-translation-version-hash

okorshenko modified the milestones: July 2017, August 2017 Aug 1, 2017

magento-team added develop labels Aug 2, 2017

ishakhsuvarov added 2 commits August 8, 2017 16:12

magento#10378: Merge branch 'develop' of github.com:magento/magento2 …

cc44db7

…into improve-translation-version-hash

magento#10378: Improved calculating version hash for the js-translati…

93ca363

…on.json file

ishakhsuvarov added 2 commits August 17, 2017 15:19

magento#10378: Merge branch 'develop' of github.com:magento/magento2 …

5b1b749

…into improve-translation-version-hash

magento#10378: Improved calculating version hash for the js-translati…

cbca085

…on.json file

magento-team added develop Progress: accept Progress: needs update labels Aug 17, 2017

magento-team merged commit cbca085 into magento:develop Aug 18, 2017

magento-team pushed a commit that referenced this pull request Aug 18, 2017

MAGETWO-71201: Improved calculating version hash for the js-translati…

a6ff645

…on.json file. #10378

ishakhsuvarov removed the Progress: needs update label Sep 4, 2017

ihor-sviziev reviewed Sep 5, 2017

View reviewed changes

This was referenced Sep 16, 2017

Add escaping of js translation version hostep/magento2#1

Closed

Add escaping of js translation version #10904

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improved calculating version hash for the js-translation.json file. #10378

Improved calculating version hash for the js-translation.json file. #10378

hostep commented Jul 30, 2017 •

edited by ishakhsuvarov

ihor-sviziev commented Jul 30, 2017

ishakhsuvarov Jul 31, 2017

ishakhsuvarov Jul 31, 2017

ishakhsuvarov Jul 31, 2017

ishakhsuvarov commented Jul 31, 2017

hostep commented Jul 31, 2017

hostep commented Jul 31, 2017

ishakhsuvarov commented Aug 1, 2017

ishakhsuvarov commented Aug 8, 2017

hostep commented Aug 15, 2017

ishakhsuvarov commented Aug 15, 2017 •

edited

hostep commented Aug 18, 2017

ishakhsuvarov commented Aug 18, 2017

ihor-sviziev Sep 5, 2017

ishakhsuvarov Sep 5, 2017

ihor-sviziev Sep 16, 2017

Improved calculating version hash for the js-translation.json file. #10378

Improved calculating version hash for the js-translation.json file. #10378

Conversation

hostep commented Jul 30, 2017 • edited by ishakhsuvarov

Description

Fixed Issues (if relevant)

Manual testing scenarios

Contribution checklist

ihor-sviziev commented Jul 30, 2017

ishakhsuvarov Jul 31, 2017

Choose a reason for hiding this comment

ishakhsuvarov Jul 31, 2017

Choose a reason for hiding this comment

ishakhsuvarov Jul 31, 2017

Choose a reason for hiding this comment

ishakhsuvarov commented Jul 31, 2017

hostep commented Jul 31, 2017

hostep commented Jul 31, 2017

ishakhsuvarov commented Aug 1, 2017

ishakhsuvarov commented Aug 8, 2017

hostep commented Aug 15, 2017

ishakhsuvarov commented Aug 15, 2017 • edited

hostep commented Aug 18, 2017

ishakhsuvarov commented Aug 18, 2017

ihor-sviziev Sep 5, 2017

Choose a reason for hiding this comment

ishakhsuvarov Sep 5, 2017

Choose a reason for hiding this comment

ihor-sviziev Sep 16, 2017

Choose a reason for hiding this comment

hostep commented Jul 30, 2017 •

edited by ishakhsuvarov

ishakhsuvarov commented Aug 15, 2017 •

edited