[Translation] Fix InvalidArgumentException when using untranslated plural forms from .po files #24594
Conversation
| preg_match_all('/(?:\|\||[^\|])++/', $message, $parts); | ||
| $parts = array(); | ||
| if (preg_match('/^\|+$/', $message)) { | ||
| $parts = explode('|', $message); |
There was a problem hiding this comment.
this is broken if previous parts of the message have escaped pipes in them
There was a problem hiding this comment.
@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.
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 "||" because PoFileLoader.php use pipes as a concatenator. This PR fix the cases where the messages are empty.
There was a problem hiding this comment.
What if only some of the plurals are empty ?
There was a problem hiding this comment.
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.
ah, I missed the fact that it was anchored on both ends
| @@ -49,10 +49,16 @@ class MessageSelector | |||
| */ | |||
| public function choose($message, $number, $locale) | |||
| { | |||
| preg_match_all('/(?:\|\||[^\|])++/', $message, $parts); | |||
| $parts = array(); | |||
| if (preg_match('/^\|+$/', $message)) { | |||
There was a problem hiding this comment.
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.
|
@stof , changes made as requested :) |
|
But tests are failing. |
…translated plural forms from .po files
|
@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 |
|
@nicolas-grekas @stof tests are passing, is this okay to merge now? |
|
This has been rebased on 3.3 and tests are passing. Can this be merged please? |
|
Are 2.7 and 2.8 not affected? |
|
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 | MIT 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. Commits ------- fea815b [Translation] Fix InvalidArgumentException when using untranslated plural forms from .po files
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.