Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

[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.

Sign up for GitHub

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

Closed
wants to merge 2 commits into from
Closed

[Translation] Fix InvalidArgumentException when using untranslated plural forms from .po files #24594

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7469cf3
[Translation][transChoice] Fix InvalidArgumentException when using un…
BjornTwachtmann Oct 17, 2017
0663d72
Prevent unnecessary backtracking on regex
BjornTwachtmann Oct 20, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Prevent unnecessary backtracking on regex
  • Loading branch information
@BjornTwachtmann
BjornTwachtmann committed Nov 6, 2017
commit 0663d723e8115148fc4b472d3776441aa21b7b97
2 changes: 1 addition & 1 deletion 2 src/Symfony/Component/Translation/MessageSelector.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ class MessageSelector
public function choose($message, $number, $locale)
{
$parts = array();
if (preg_match('/^\|+$/', $message)) {
if (preg_match('/^\|++$/', $message)) {
$parts = explode('|', $message);
Copy link
Member

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

Copy link
Contributor Author

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.

Copy link

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 "||" because PoFileLoader.php use pipes as a concatenator. This PR fix the cases where the messages are empty.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if only some of the plurals are empty ?

Copy link
Contributor Author

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, $message should contain characters other than | , and the /^|+$/ regex will not match. Execution should then proceed into the elseif block and behaviour will be exactly the same as the current code.

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.

Copy link
Member

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

} elseif (preg_match_all('/(?:\|\||[^\|])++/', $message, $matches)) {
$parts = $matches[0];
Expand Down
Morty Proxy This is a proxified and sanitized view of the page, visit original site.