Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Translation] Fix InvalidArgumentException when using untranslated plural forms from .po files#24594
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
[Translation] Fix InvalidArgumentException when using untranslated plural forms from .po files#24594
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| preg_match_all('/(?:\|\||[^\|])++/',$message,$parts); | ||
| $parts =array(); | ||
| if (preg_match('/^\|+$/',$message)) { | ||
| $parts =explode('|',$message); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
this is broken if previous parts of the message have escaped pipes in them
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@stof ,sorry, but I don't understand. What do you mean by "previous parts of the message"? Is this function called recursively, or on partial strings?
As far as I can see the regex matches any $message which has only pipes in it. My assumtion is that this means the message itself is empty. Could you explain if this is incorrect, I'm not that familiar with Translate internals.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I can confirm this fix the issue I have reported previously#24359
When for example a translation has 3 empty plurals then the value of $message is "||" becausePoFileLoader.php use pipes as a concatenator. This PR fix the cases where the messages are empty.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
What if onlysome of the plurals are empty ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
If only some of the plurals are empty,
I appreciate that my patch doesn't fix the case you describe. However it doesn't break that case any further, and I cannot figure out how to fix that case at this stage in the program's execution. As I mentioned in the linked ticket, I could only fix that by revisiting the idea of representing multiple translations as a pipe separated string, and I'm not familiar enough with all of the use cases for Translate to attempt that.
This does fix the most common case in PO files, that's all I'm attempting to do. It would make sense for somebody with more knowledge of translate to address the edge cases you describe, but until that can be done this will at least prevent a lot of problems.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
ah, I missed the fact that it was anchored on both ends
| { | ||
| preg_match_all('/(?:\|\||[^\|])++/',$message,$parts); | ||
| $parts =array(); | ||
| if (preg_match('/^\|+$/',$message)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
please change the regex to^\|++$ to avoid useless backtracking (which could cause issues with long strings). Backtracking will never allow to find a match in a different way for this regex.
BjornTwachtmann commentedOct 20, 2017
@stof , changes made as requested :) |
nicolas-grekas commentedNov 5, 2017
But tests are failing. |
…translated plural forms from .po files
BjornTwachtmann commentedNov 6, 2017
@nicolas-grekas I've rebased the branch on 3.3 and all tests pass now. I think there were some failing tests in the 3.2 branch |
BjornTwachtmann commentedNov 27, 2017
@nicolas-grekas@stof tests are passing, is this okay to merge now? |
Tasiobg commentedDec 7, 2017
This has been rebased on 3.3 and tests are passing. Can this be merged please? |
xabbuh commentedDec 8, 2017
Are 2.7 and 2.8 not affected? |
BPScott commentedDec 8, 2017
fabpot commentedDec 11, 2017
Thank you@BjornTwachtmann. |
…anslated plural forms from .po files (BjornTwachtmann)This PR was squashed before being merged into the 3.3 branch (closes#24594).Discussion----------[Translation] Fix InvalidArgumentException when using untranslated plural forms from .po files| Q | A| ------------- | ---| Branch? | 3.2| Bug fix? | yes| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |#24593| License | MITThis PR fixes the bug in#24593. It's not the absolutely ideal way to address the issue, but I don't see how else to handle it without either dropping support for pips in translation strings, or rearchitecting the code that feeds translations to the MessageSelector as pipe separated in the first place.Commits-------fea815b [Translation] Fix InvalidArgumentException when using untranslated plural forms from .po files
Uh oh!
There was an error while loading.Please reload this page.
This PR fixes the bug in#24593. It's not the absolutely ideal way to address the issue, but I don't see how else to handle it without either dropping support for pips in translation strings, or rearchitecting the code that feeds translations to the MessageSelector as pipe separated in the first place.