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

Sprintf function broken in 1.7.6 for custom module templates #14786

Merged
merged 2 commits into from Jul 31, 2019

Conversation

@roja45
Copy link
Contributor

commented Jul 19, 2019

Questions Answers
Branch? develop
Description? When using the sprintf function in a front office module template, the translation function is called twice on the input string to be translated. As far as I can see in the code, the line I have removed is not used as the variable being set is immediately overwritten. It does however translate the input string, which in then passed into the translation a second time, this corrupts the output.
Type? bug fix
Category? FO
BC breaks? no
Deprecations? no
Fixed ticket? Fixes #14706
How to test? There is a test module attached to the original ticket. If installed in 1.7.6 clean, you will see the error. Apply this change and the corrupted string is fixed.

This change is Reviewable

@roja45 roja45 requested a review from PrestaShop/prestashop-core-developers as a code owner Jul 19, 2019

@PierreRambaud

This comment has been minimized.

Copy link
Contributor

commented Jul 23, 2019

Not sure this is the right way to fix the original bug 🤔
@eternoendless @matthieu-rolland Wdyt?

@marionf

This comment has been minimized.

Copy link
Contributor

commented Jul 23, 2019

It's a regression of 1.7.6.0, so should be fixed on 1.7.6.x branch

@roja45

This comment has been minimized.

Copy link
Contributor Author

commented Jul 23, 2019

@PierreRambaud yup, that's quite likely, the translation code is not somewhere I would usually find myself. All I can say is that;

this line: $ret = str_replace('"', '"', $string); is not doing anything, that I can see.

and it's this line: $string = Translate::checkAndReplaceArgs($string, $sprintf); being called twice that is causing the problem.

    ```
if ($_MODULES == null) {
            if (
                $sprintf !== null &&
                (!is_array($sprintf) || !empty($sprintf)) &&
                !(count($sprintf) === 1 && isset($sprintf['legacy']))
            ) {
                $string = Translate::checkAndReplaceArgs($string, $sprintf);
            }
            $ret = str_replace('"', '"', $string);
        }

Let me know if I need to do something more.

Cheers

@PierreRambaud PierreRambaud requested a review from eternoendless Jul 23, 2019

@PierreRambaud PierreRambaud requested a review from matthieu-rolland Jul 23, 2019

@PierreRambaud PierreRambaud changed the title Fix issue 14706 Sprintf function broken in 1.7.6 for custom module templates Jul 23, 2019

@PierreRambaud PierreRambaud self-assigned this Jul 30, 2019

@PierreRambaud PierreRambaud force-pushed the roja45:fix-issue-14706 branch from 615dc21 to b786f47 Jul 30, 2019

@PierreRambaud PierreRambaud changed the base branch from develop to 1.7.6.x Jul 30, 2019

@PierreRambaud

This comment has been minimized.

Copy link
Contributor

commented Jul 30, 2019

We investigate and we think your solution is the best one 👍
(Because of

which is a duplicate :D)

@marionf marionf added this to the 1.7.6.1 milestone Jul 30, 2019

@marionf marionf added 1.7.6.x and removed develop labels Jul 30, 2019

@sarahdib sarahdib added QA ✔️ and removed waiting for QA labels Jul 30, 2019

@PierreRambaud PierreRambaud force-pushed the roja45:fix-issue-14706 branch from b786f47 to a212b1f Jul 30, 2019

@PierreRambaud

This comment has been minimized.

Copy link
Contributor

commented Jul 30, 2019

PR rebased for CI.

@PierreRambaud PierreRambaud merged commit 0bbe1d9 into PrestaShop:1.7.6.x Jul 31, 2019

2 checks passed

PrettyCI Code formatting
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@PierreRambaud

This comment has been minimized.

Copy link
Contributor

commented Jul 31, 2019

Thanks a lot @roja45 😸

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.