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

Remove risky and useless sprintf() calls #8381

Merged
merged 5 commits into from Oct 31, 2017

Conversation

Projects
None yet
6 participants
@Quetzacoalt91
Member

Quetzacoalt91 commented Oct 2, 2017

Questions Answers
Branch? develop
Description? We often receive issues regarding translation, and the concerned translations always have a call to sprintf(). The new function trans() is already able to replace the placeholders without throwing fatal errors in case of errors. So this PR removes all calls to sprintf where trans can be used alone.
Type? improvement
Category? CO
BC breaks? Nope
Deprecations? Nope
Fixed ticket? /
How to test? Adding a new %s in the concerned messages threw errors with sprintf(). With this PR, there is no fatal errors.

This change is Reviewable

@eternoendless

This comment has been minimized.

Show comment
Hide comment
@eternoendless

eternoendless Oct 3, 2017

Member

Reviewed 35 of 36 files at r1, 1 of 1 files at r2.
Review status: all files reviewed at latest revision, 2 unresolved discussions.


classes/checkout/DeliveryOptionsFinder.php, line 100 at r2 (raw file):
PSR-2 states:

Argument lists MAY be split across multiple lines, where each subsequent line is indented once. When doing so, the first item in the list MUST be on the next line, and there MUST be only one argument per line.

So please split the arguments list so that there's only one argument per line.


controllers/admin/AdminTranslationsController.php, line 2546 at r2 (raw file):

                '.$this->trans('There was a problem getting the mail files.', array(), 'Admin.International.Notification').'<br>
                '.$this->trans('English language files must exist in [1]%folder%[/1] folder', array(
                    '%folder%' => '<em>'.preg_replace('@/[a-z]{2}(/?)$@', '/en$1', $mails['directory']).'</em>'

It looks like the replacements for [1] and [/1] are missing


Comments from Reviewable

Member

eternoendless commented Oct 3, 2017

Reviewed 35 of 36 files at r1, 1 of 1 files at r2.
Review status: all files reviewed at latest revision, 2 unresolved discussions.


classes/checkout/DeliveryOptionsFinder.php, line 100 at r2 (raw file):
PSR-2 states:

Argument lists MAY be split across multiple lines, where each subsequent line is indented once. When doing so, the first item in the list MUST be on the next line, and there MUST be only one argument per line.

So please split the arguments list so that there's only one argument per line.


controllers/admin/AdminTranslationsController.php, line 2546 at r2 (raw file):

                '.$this->trans('There was a problem getting the mail files.', array(), 'Admin.International.Notification').'<br>
                '.$this->trans('English language files must exist in [1]%folder%[/1] folder', array(
                    '%folder%' => '<em>'.preg_replace('@/[a-z]{2}(/?)$@', '/en$1', $mails['directory']).'</em>'

It looks like the replacements for [1] and [/1] are missing


Comments from Reviewable

@Quetzacoalt91

This comment has been minimized.

Show comment
Hide comment
@Quetzacoalt91

Quetzacoalt91 Oct 4, 2017

Member

Changes applied. Thanks guys

Member

Quetzacoalt91 commented Oct 4, 2017

Changes applied. Thanks guys

@eternoendless

This comment has been minimized.

Show comment
Hide comment
@eternoendless

eternoendless Oct 19, 2017

Member

:lgtm:


Reviewed 7 of 7 files at r3.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

Member

eternoendless commented Oct 19, 2017

:lgtm:


Reviewed 7 of 7 files at r3.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@eternoendless

This comment has been minimized.

Show comment
Hide comment
@eternoendless

eternoendless Oct 19, 2017

Member

@LouiseBonnard do you approve the wording?

Member

eternoendless commented Oct 19, 2017

@LouiseBonnard do you approve the wording?

@eternoendless eternoendless added this to the 1.7.3.0 milestone Oct 19, 2017

@LouiseBonnard

This comment has been minimized.

Show comment
Hide comment
@LouiseBonnard

LouiseBonnard Oct 19, 2017

Contributor

À part la review plus haut, c'est okay pour moi.

Contributor

LouiseBonnard commented Oct 19, 2017

À part la review plus haut, c'est okay pour moi.

@eternoendless

This comment has been minimized.

Show comment
Hide comment
@eternoendless

eternoendless Oct 30, 2017

Member

@Quetzacoalt91 could you please rebase?

Member

eternoendless commented Oct 30, 2017

@Quetzacoalt91 could you please rebase?

@Quetzacoalt91

This comment has been minimized.

Show comment
Hide comment
@Quetzacoalt91

Quetzacoalt91 Oct 31, 2017

Member

Rebase done!

Member

Quetzacoalt91 commented Oct 31, 2017

Rebase done!

@mickaelandrieu

This comment has been minimized.

Show comment
Hide comment
@mickaelandrieu

mickaelandrieu Oct 31, 2017

Contributor

And merged, thanks @Quetzacoalt91

Contributor

mickaelandrieu commented Oct 31, 2017

And merged, thanks @Quetzacoalt91

@mickaelandrieu mickaelandrieu merged commit af1bc41 into PrestaShop:develop Oct 31, 2017

2 of 3 checks passed

code-review/reviewable 7 files left (eternoendless)
Details
codacy/pr Good work! A positive pull request.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Nobodaddy

This comment has been minimized.

Show comment
Hide comment
@Nobodaddy

Nobodaddy Nov 23, 2017

Contributor

controllers/admin/AdminTaxRulesGroupController.php
'hint' => $this->trans('(Total tax: 9%)', array(), 'Admin.International.Help'),
lol, you must be kidding. Please fix this new introduced bug!

Contributor

Nobodaddy commented on e15d730 Nov 23, 2017

controllers/admin/AdminTaxRulesGroupController.php
'hint' => $this->trans('(Total tax: 9%)', array(), 'Admin.International.Help'),
lol, you must be kidding. Please fix this new introduced bug!

This comment has been minimized.

Show comment
Hide comment
@Quetzacoalt91

Quetzacoalt91 Nov 23, 2017

Member

Sorry @Nobodaddy, but it seems we do not have the same humor.

Just made a quick test, just for you:

dump(sprintf($translator->trans('(Total tax: %s)', array(), 'Admin.International.Help'), '9%'));
dump($translator->trans('(Total tax: 9%)', array(), 'Admin.International.Help'));

And their result:
capture du 2017-11-23 13-37-52

As we deal with hints and example, The number isn't really important. You may have missed this, but the previous example got its value hardcoded anyway. :)

Member

Quetzacoalt91 replied Nov 23, 2017

Sorry @Nobodaddy, but it seems we do not have the same humor.

Just made a quick test, just for you:

dump(sprintf($translator->trans('(Total tax: %s)', array(), 'Admin.International.Help'), '9%'));
dump($translator->trans('(Total tax: 9%)', array(), 'Admin.International.Help'));

And their result:
capture du 2017-11-23 13-37-52

As we deal with hints and example, The number isn't really important. You may have missed this, but the previous example got its value hardcoded anyway. :)

This comment has been minimized.

Show comment
Hide comment
@Nobodaddy

Nobodaddy Nov 23, 2017

Contributor

No offense, @Quetzacoalt91
but nobody but you will realize that this is meant as a hint. Sorry for the misleading "new introduced". If you try to give the user a hint, then do it!
A hint would be for example "Choose the preferred tax rate", but not "(total tax: 9%)". Because this is not the total tax, but a possible tax rate. This explanation was ... just for you.

Contributor

Nobodaddy replied Nov 23, 2017

No offense, @Quetzacoalt91
but nobody but you will realize that this is meant as a hint. Sorry for the misleading "new introduced". If you try to give the user a hint, then do it!
A hint would be for example "Choose the preferred tax rate", but not "(total tax: 9%)". Because this is not the total tax, but a possible tax rate. This explanation was ... just for you.

This comment has been minimized.

Show comment
Hide comment
@Quetzacoalt91

Quetzacoalt91 Nov 24, 2017

Member

The job of this PR is to remove the calls to the function sprintf(), not modifying the content of the page.

If something is unclear to you, you may be right, but note this is not coming from that PR. That line was added in PS 1.5.3.0 b776d6c#diff-21c9ebbacec5a9ec2ffa89f9c5370019R308 which is 6 years old.

I could have left a variable in this string if there are other occurrences of it, but nothing. There's no need to use a placeholder if this is the only place we use that translated string.

Member

Quetzacoalt91 replied Nov 24, 2017

The job of this PR is to remove the calls to the function sprintf(), not modifying the content of the page.

If something is unclear to you, you may be right, but note this is not coming from that PR. That line was added in PS 1.5.3.0 b776d6c#diff-21c9ebbacec5a9ec2ffa89f9c5370019R308 which is 6 years old.

I could have left a variable in this string if there are other occurrences of it, but nothing. There's no need to use a placeholder if this is the only place we use that translated string.

This comment has been minimized.

Show comment
Hide comment
@Nobodaddy

Nobodaddy Nov 24, 2017

Contributor

You're right, thank you. I checked it now. Never realized that this nonsense hint was there since years.

Contributor

Nobodaddy replied Nov 24, 2017

You're right, thank you. I checked it now. Never realized that this nonsense hint was there since years.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment