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.

Sign up for GitHub

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

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

Merged
merged 9 commits into from
Aug 18, 2017
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
a1dab12
Improved calculating version hash for the js-translation.json file.
hostep Jul 30, 2017
b3a1c7e
Refactored getting translation file version into a more desirable sta…
hostep Jul 31, 2017
507cd42
Start using the file contents for calculating the hash again instead …
hostep Jul 31, 2017
def3cff
Fixes integration tests.
hostep Jul 31, 2017
75990d4
magento/magento2#10378: Merge branch 'develop' of github.com:magento/…
ishakhsuvarov Aug 1, 2017
cc44db7
magento/magento2#10378: Merge branch 'develop' of github.com:magento/…
ishakhsuvarov Aug 8, 2017
93ca363
magento/magento2#10378: Improved calculating version hash for the js-…
ishakhsuvarov Aug 8, 2017
5b1b749
magento/magento2#10378: Merge branch 'develop' of github.com:magento/…
ishakhsuvarov Aug 17, 2017
cbca085
magento/magento2#10378: Improved calculating version hash for the js-…
ishakhsuvarov Aug 17, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions app/code/Magento/Translation/Block/Js.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,4 +68,12 @@ public function getTranslationFilePath()
{
return $this->fileManager->getTranslationFilePath();
}

/**
* @return string
*/
public function getTranslationFileFullPath()
{
return $this->fileManager->getTranslationFileFullPath();
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

}
}
2 changes: 1 addition & 1 deletion app/code/Magento/Translation/Model/FileManager.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ public function getTranslationFileTimestamp()
/**
* @return string
*/
protected function getTranslationFileFullPath()
public function getTranslationFileFullPath()
Copy link
Contributor

Choose a reason for hiding this comment

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

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

{
return $this->directoryList->getPath(DirectoryList::STATIC_VIEW) .
\DIRECTORY_SEPARATOR .
Expand Down
2 changes: 1 addition & 1 deletion app/code/Magento/Translation/view/base/templates/translate.phtml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()); ?>
Copy link
Contributor

Choose a reason for hiding this comment

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

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


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

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds completely reasonable to me

Copy link
Contributor

Choose a reason for hiding this comment

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

@ishakhsuvarov pull request prepared #10904

dependencies.push(
Expand Down