Skip to content

[Translation] Prevent creating empty keys when key ends with a period #52005

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

Merged
merged 1 commit into from
Oct 13, 2023
Merged

[Translation] Prevent creating empty keys when key ends with a period #52005

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
28 changes: 28 additions & 0 deletions src/Symfony/Component/Translation/Tests/Util/ArrayConverterTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,34 @@ public static function messagesData()
],
],
],
[
// input
[
'foo.' => 'foo.',
'.bar' => '.bar',
'abc.abc' => 'value',
'bcd.bcd.' => 'value',
'.cde.cde.' => 'value',
'.def.def' => 'value',
],
// expected output
[
'foo.' => 'foo.',
'.bar' => '.bar',
'abc' => [
'abc' => 'value',
],
'bcd' => [
'bcd.' => 'value',
],
'.cde' => [
'cde.' => 'value',
],
'.def' => [
'def' => 'value',
],
],
],
];
}
}
44 changes: 43 additions & 1 deletion src/Symfony/Component/Translation/Util/ArrayConverter.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ public static function expandToTree(array $messages)
$tree = [];

foreach ($messages as $id => $value) {
$referenceToElement = &self::getElementByPath($tree, explode('.', $id));
$referenceToElement = &self::getElementByPath($tree, self::getKeyParts($id));

$referenceToElement = $value;

Expand All @@ -65,6 +65,7 @@ private static function &getElementByPath(array &$tree, array $parts)
$elem = &$elem[implode('.', \array_slice($parts, $i))];
break;
}

$parentOfElem = &$elem;
$elem = &$elem[$part];
}
Expand Down Expand Up @@ -96,4 +97,45 @@ private static function cancelExpand(array &$tree, string $prefix, array $node)
}
}
}

private static function getKeyParts(string $key)
Copy link
Member

Choose a reason for hiding this comment

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

I think we can simplify this method with something like:

function getKeyParts(string $key) {
    $parts = explode('.', $key);
    $nb = count($parts);

    if (str_starts_with($key, '.')) {
        $parts[0] = '.';
    }
    if (str_ends_with($key, '.')) {
        $parts[$nb-1] = '.';
    }
    return $parts;
}

Copy link
Contributor Author

@javleds javleds Oct 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks @welcoMattic. I love short code...

Nevertheless the shared code has 2 small disadvantages:

  1. It is using functions available from php 8.0^ (str_starts_with and, str_ends_with), this code is intended to be part of the version 5.4 which still supports older versions of php.
  2. Even if this covers the main goal (which is prevent empty keys with dots at the begining and, in the end) this is not covering some edge cases:
'abc 1.abc 2' => 'value',
'bcd 1.bcd 2.' => 'value',
'.cde 1.cde 2.' => 'value',
'.def 1.def 2' => 'value',

Please see the test of this section for more info.

If I am not addressing something, please let me know, I will be glad to adjust the PR if needed.

Copy link
Contributor

Choose a reason for hiding this comment

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

for php version, you can leverage symfony/polyfill-php (find the one v that introduced the relevant function)

Copy link
Contributor

@squrious squrious Oct 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

The polyfill that introduces str_starts_with and str_ends_with is symfony/polyfill-php80, and it is already required in the Translation component so using those functions is ok imo

Copy link
Contributor

Choose a reason for hiding this comment

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

ho indeed cool if already included

Copy link
Member

Choose a reason for hiding this comment

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

@javleds It seems the edge cases you mention are covered by simplier code here:
https://3v4l.org/2ic9k

function getKeyParts(string $key) {
    $parts = explode('.', $key);
    $nb = count($parts);

    if (str_starts_with($key, '.')) {
        $parts[0] = '.';
    }
    if (str_ends_with($key, '.')) {
        $parts[$nb-1] = '.';
    }
    return $parts;
}


var_dump(getKeyParts('abc 1.abc 2'));
var_dump(getKeyParts('bcd 1.bcd 2.'));
var_dump(getKeyParts('.cde 1.cde 2.'));
var_dump(getKeyParts('.def 1.def 2'));

Copy link
Contributor Author

@javleds javleds Oct 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for the advise of php polyfill, I will keep in mind in the case we need to perform additional updates. 🙏

Getting back to the code discussion:

The real problem here is to avoid trimming the periods at the beginning/end of the parts. Since the getElementByPath function is itended to process each part as a valid string. Having empty positions or a position wihtn a single dot won't give the expected result.

For example:

  • ['foo', 'bar'] will give the expected result.
  • ['foo', '', 'bar'] won't give the expected result due the empty position.
  • ['foo', 'bar', '.'] won't give the expected result due the period at position 2.
  • ['.foo', 'bar.'] will give the expected result.

The simpler solution is to keep the period concatenated to the key part, and prevent the period occupate a single possition in the array. Otherwise, we will have to add extra processing inside the getElementByPath function.

Having this in mind, the code proposed above has small variations on the expected result.

Let me give the examples:

input expected output with current fix output with simpler code
abc.abc ['abc', 'abc'] ['abc', 'abc'] ['abc', 'abc']
bcd.bcd. ['bcd', 'bcd.'] ['bcd', 'bcd.'] ['bcd', 'bcd', '.']
.cde.cde. ['.cde', 'cde.'] ['.cde', 'cde.'] ['.', 'cde', 'cde', '.']
.def.def ['.def', 'def'] ['.def', 'def'] ['.','def', 'def']

If the proposed solution is hard to read, maybe, we can create named variables for every if statement.

{
$parts = explode('.', $key);
$partsCount = \count($parts);

$result = [];
$buffer = '';

foreach ($parts as $index => $part) {
if (0 === $index && '' === $part) {
$buffer = '.';

continue;
}

if ($index === $partsCount - 1 && '' === $part) {
$buffer .= '.';
$result[] = $buffer;

continue;
}

if (isset($parts[$index + 1]) && '' === $parts[$index + 1]) {
$buffer .= $part;

continue;
}

if ($buffer) {
$result[] = $buffer.$part;
$buffer = '';

continue;
}

$result[] = $part;
}

return $result;
}
}