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

FIX Replace deprecated %s placeholders in translations with named placeholders #6598

Merged

Conversation

robbieaverill
Copy link
Contributor

@robbieaverill robbieaverill commented Feb 4, 2017

This pull request largely removed the use of sprintf(_t(TRANSNAME, '... %s'), $myVar) in favour of the named placeholder equivalent _t(TRANSNAME, '... {myvar}', ['myvar' => $myVar]).

All translations have been updated to ensure they don't break before in the short term.

There are some areas especially in the tests that use this deliberately to test against, so I've left them alone for now, but perhaps we should remove the syntax from the tests and not test the behaviour any more?

Resolves #6582

@robbieaverill robbieaverill force-pushed the bugfix/remove-sprintf-in-translations branch 3 times, most recently from 6aa85ae to debd432 Compare February 4, 2017 21:13
@robbieaverill
Copy link
Contributor Author

@tractorcow I'll rebase this branch shortly.

Regarding the deprecation/removal of that method we talked about (#vague) - the changelog currently says that the %s format is deprecated and will be removed in 5.0, so perhaps we should deprecate that method too instead of removing it?

@robbieaverill robbieaverill force-pushed the bugfix/remove-sprintf-in-translations branch from 56064e3 to 30c7ca5 Compare February 27, 2017 07:24
@robbieaverill
Copy link
Contributor Author

robbieaverill commented Feb 28, 2017

Do not merge

We need to set up a new Transifex account (?) for these updated translations so that merging this PR will not affect existing versions of the framework. Done.

@robbieaverill robbieaverill force-pushed the bugfix/remove-sprintf-in-translations branch from 30c7ca5 to a1b6623 Compare February 28, 2017 20:03
@tractorcow
Copy link
Contributor

The plan is to fork translations at some point in the future, and then merge this PR into that, before pushing up to transifex. That will allow us to replace upstream strings without affecting 3.x releases.

@dhensby
Copy link
Contributor

dhensby commented Mar 31, 2017

what's the progress on this? @robbieaverill it'll need a rebase at some point

@robbieaverill
Copy link
Contributor Author

what's the progress on this? @robbieaverill it'll need a rebase at some point

@dhensby yeah I'll rebase it when we're ready to look at it, otherwise it'll just keep needing to be rebased as the translations change :D

robbieaverill and others added 2 commits August 2, 2017 13:03
…ceholders

* Remove the use of sprintf and %s placeholders in the i18n tests
@tractorcow tractorcow force-pushed the bugfix/remove-sprintf-in-translations branch from 0a86177 to 1867ee8 Compare August 2, 2017 01:31
@tractorcow tractorcow merged commit 772cd4a into silverstripe:4 Aug 2, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants