[Translation] Prevent creating empty keys when key ends with a period #52005
Conversation
|
Hey! I see that this is your first PR. That is great! Welcome! Symfony has a contribution guide which I suggest you to read. In short:
Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change. When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor! I am going to sit back now and wait for the reviews. Cheers! Carsonbot |
|
Hey! Thanks for your PR. You are targeting branch "6.4" but it seems your PR description refers to branch "5.4 for bug fixes". Cheers! Carsonbot |
e3a062a to
1d41e2c
Compare
src/Symfony/Component/Translation/Tests/Util/ArrayConverterTest.php
Outdated
Show resolved
Hide resolved
fb42f23 to
57ce1f4
Compare
| @@ -96,4 +97,45 @@ private static function cancelExpand(array &$tree, string $prefix, array $node) | |||
| } | |||
| } | |||
| } | |||
|
|
|||
| private static function getKeyParts(string $key) | |||
There was a problem hiding this comment.
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;
}
There was a problem hiding this comment.
Thanks @welcoMattic. I love short code...
Nevertheless the shared code has 2 small disadvantages:
- 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.
- 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.
There was a problem hiding this comment.
for php version, you can leverage symfony/polyfill-php (find the one v that introduced the relevant function)
There was a problem hiding this comment.
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
There was a problem hiding this comment.
ho indeed cool if already included
There was a problem hiding this comment.
@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'));There was a problem hiding this comment.
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.
| // input | ||
| [ | ||
| 'foo.' => 'foo.', | ||
| 'bar.' => 'bar.', |
There was a problem hiding this comment.
| 'bar.' => 'bar.', | |
| '.bar' => '.bar', |
As you already test trailing period in 1 word-key on the previous line
There was a problem hiding this comment.
Right.
Both cases were intended to test no affectation with or without blank spaces, but, since the spaces are no affecting the output we will simplify reading by removing those (we can remove 2 entries lines 77 and, 78).
Next update will contain this cahnges.
[Translation] create getKeyParts() to keep periods at start or end of the key [Translation] Simplify test cases by removing blank spaces
57ce1f4 to
5b0cf25
Compare
|
Thank you @javleds. |
Small fix that prevents creating empty keys on translations when the translation key ends with a period, example:
'This is a validation.'Before the fix, the yaml output was:
After the fix we got:
This can be replicated by:
--webappflag.php bin/console translation:extract --force --format=yaml --as-tree=3 --prefix='' --clean enResults:
Before the fix:
After the fix:
