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

\Magento\Framework\Phrase\Renderer\Placeholder::render() can't handle more than 9 placeholders #1374

Closed
riconeitzel opened this issue Jun 16, 2015 · 4 comments
Labels
bug report Issue: Ready for Work Gate 4. Acknowledged. Issue is added to backlog and ready for development

Comments

@riconeitzel
Copy link
Contributor

Using the code fragment

<?= 
__(
    '%1<br>%2<br>%3<br>%4<br>%5<br>%6<br>%7<br>%8<br>%9<br>%10',
    'one', 'two', 'three', 'four', 'five', 'six', 'seven', 'eight', 'nine', 'ten'
); 
?>

in a phtml file will result in the following wrong output:

one
two
three
four
five
six
seven
eight
nine
one0

because Placeholder Renderer uses str_replace which replaces %10 in the first place.

Question: What exactly was wrong with the old sprintf rendering?!

@joshdifabio
Copy link
Contributor

We can fix this easily by reversing the ordering of the items in the str_replace call and replacing the higher-numbered items first. I'll submit a PR.

@riconeitzel
Copy link
Contributor Author

@joshdifabio yeah but that just "covers" the problem that str_replace might not be the right piece of PHP for that task? What happened to sprintf here?

@joshdifabio
Copy link
Contributor

Well, I'm not part of Magento but I'll try to guess the reasons.

The interface of this method is quite different to that of sprintf: This method supports named and numbered arguments and implicitly treats all arguments as strings, whereas sprintf only supports numbered arguments and requires clients to define the type of each argument, which makes numbered arguments quite cumbersome to use.

With Magento's current approach:

__('An %foo can look like %bar', ['foo' => 'input', 'bar' => 'this'])

sprintf style:

__('An %s can look like %s', ['input', 'this'])

With Magento's current approach:

__('This %1 contains %2 of %3 so %4 numbered %5 is %6', ['sentence', 'lots', 'words', 'a', 'approach', 'helpful'])

sprintf style:

__('This %1$s contains %2$s of %3$s so %4$s numbered %5$s is %6$s', ['sentence', 'lots', 'words', 'a', 'approach', 'helpful'])

I suppose Magento only wish to support simple string replacements and therefore they opted for a smaller and more focused interface than what sprintf provides.

@riconeitzel
Copy link
Contributor Author

Thanks for taking the time to think about that @joshdifabio :-) 👍

@vpelipenko vpelipenko added the Issue: Ready for Work Gate 4. Acknowledged. Issue is added to backlog and ready for development label Jul 14, 2015
magento-team pushed a commit that referenced this issue Aug 1, 2017
MAGETWO-70683 New registered customer not showed in admin customer grid
MAGETWO-70869 Console errors after turning on CSS merging/minification
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug report Issue: Ready for Work Gate 4. Acknowledged. Issue is added to backlog and ready for development
Projects
None yet
Development

No branches or pull requests

3 participants