Bug #94729
openMethod ArrayUtility::replaceAndAppendScalarValuesRecursive re-numbers associative arrays with non-consecutively numbered integer-only keys
0%
Description
Hello Everybody,
as already indicated in [[http://forge.typo3.org/issues/91236]] under certain conditions the method …
\TYPO3\CMS\Core\Utility\ArrayUtility::replaceAndAppendScalarValuesRecursive
… sometimes re-numbers associative keys, which look like plain consecutively numbered (YAML-)list keys. Before TYPO3 version 10 this was not as obvious as it became in version 10 and above, because previous versions of the Form-Framework did not use the …
\TYPO3\CMS\Core\Configuration\Loader\YamlFileLoader
… as documented in [[https://docs.typo3.org/c/typo3/cms-core/master/en-us/Changelog/10.2/Feature-84203-UnifyFormSetupYAMLLoading.html]]. But of course it was/is also an issue in version below 10, but not in the form-framework.
Before getting into detail, here is a working solution: [[https://review.typo3.org/c/Packages/TYPO3.CMS/+/70246]]
How can this be explained and reproduced?¶
First of all I will describe the State of YAML-Loading in the Form-Framework before TYPO3 Version 10. Let's assume we have the following (simplified) setup:
Typoscript¶
plugin.tx_form.settings.yamlConfigurations { 10 = EXT:form/Configuration/Yaml/FormSetup.yaml 20 = EXT:my_custom_extension/Configuration/Yaml/FormSetup.yaml }
EXT:form/Configuration/Yaml/FormSetup.yaml
¶
TYPO3:
CMS:
Form:
########### FORMEDITOR CONFIGURATION ###########
prototypes:
standard:
########### DEFAULT FORM ELEMENT DEFINITIONS ###########
formElementsDefinition:
Form:
formEditor:
editors:
900:
selectOptions:
10:
value: ''
label: 'formEditor.elements.Form.editor.finishers.EmptyValue.label'
20:
value: 'EmailToSender'
label: 'formEditor.elements.Form.editor.finishers.EmailToSender.label'
30:
# …
40:
# …
50:
# …
60:
# …
# …
EXT:my_custom_extension/Configuration/Yaml/FormSetup.yaml
¶
TYPO3:
CMS:
Form:
prototypes:
standard:
formElementsDefinition:
Form:
formEditor:
editors:
900:
# Example:
# Hide the EmailToSender finisher, as
# the customer never wants to use it,
# even not by accident
20: null
Processing of the Form-Framework YAML-files in TYPO3 ≤ v9¶
The Yaml-Parser turns both YAML-files into Arrays and both arrays are merged together using array_replace_recursive
, see [[https://github.com/TYPO3/typo3/blob/9.5/typo3/sysext/form/Classes/Mvc/Configuration/YamlSource.php#L90-L94]]
The result is something like:
TYPO3:
CMS:
Form:
prototypes:
standard:
formElementsDefinition:
Form:
formEditor:
editors:
900:
selectOptions:
10:
value: ''
label: 'formEditor.elements.Form.editor.finishers.EmptyValue.label'
# Here is the 20: null from above
20: null
30:
# …
40:
# …
50:
# …
60:
# …
Processing of the Form-Framework YAML-files in TYPO3 ≥ v10¶
When we assume the exact same setup as explained above, nothing really changes, and everything works as expected, see [[https://github.com/TYPO3/typo3/blob/master/typo3/sysext/form/Classes/Mvc/Configuration/YamlSource.php#L69-L77]]. The only difference is, that this time the YAML::parse
is not used directly, but indirectly through the YamlLoader
, which introduces the import
feature, see [[https://github.com/TYPO3/typo3/blob/master/typo3/sysext/form/Classes/Mvc/Configuration/YamlSource.php#L136-L139]]. Hence the result is the same as shown above.
A slightly different setup in TYPO3 ≥ v10 triggering the error¶
Typoscript¶
# same as above plugin.tx_form.settings.yamlConfigurations { 10 = EXT:form/Configuration/Yaml/FormSetup.yaml 20 = EXT:my_custom_extension/Configuration/Yaml/FormSetup.yaml }
EXT:form/Configuration/Yaml/FormSetup.yaml
¶
# the same as above
TYPO3:
CMS:
Form:
########### FORMEDITOR CONFIGURATION ###########
prototypes:
standard:
########### DEFAULT FORM ELEMENT DEFINITIONS ###########
formElementsDefinition:
Form:
formEditor:
editors:
900:
selectOptions:
10:
value: ''
label: 'formEditor.elements.Form.editor.finishers.EmptyValue.label'
20:
value: 'EmailToSender'
label: 'formEditor.elements.Form.editor.finishers.EmailToSender.label'
30:
# …
40:
# …
50:
# …
60:
# …
# …
EXT:my_custom_extension/Configuration/Yaml/FormSetup.yaml
¶
# Now we're using an import here!
import:
- { resource: "./Finishers/EmailToSender.yaml" }
TYPO3:
CMS:
Form:
prototypes:
…
EXT:my_custom_extension/Configuration/Yaml/Finishers/EmailToSender.yaml
¶
# The same content, as used in FormSetup.yaml previously
TYPO3:
CMS:
Form:
prototypes:
standard:
formElementsDefinition:
Form:
formEditor:
editors:
900:
# Example:
# Hide the EmailToSender finisher, as
# the customer never wants to use it,
# even not by accident
20: null
The (broken) Result¶
The result is now neither the same as above, nor correct anymore. It looks like:
# the same as above
TYPO3:
CMS:
Form:
prototypes:
standard:
formElementsDefinition:
Form:
formEditor:
editors:
900:
selectOptions:
# Here is the 20: null from above
0: null
10:
value: ''
label: 'formEditor.elements.Form.editor.finishers.EmptyValue.label'
20:
value: 'EmailToSender'
label: 'formEditor.elements.Form.editor.finishers.EmailToSender.label'
30:
# …
40:
# …
50:
# …
60:
# …
Processing of the Form-Framework YAML-file imports
in TYPO3 ≥ v10¶
So what happened? The reason for this, is that the YamlLoader
treats the import
differently, as the YamlSource
implementation does. The latter uses, as mentioned above, array_replace_recursive
, whereas the import
of the EmailToSender.yaml
fragment-file into the FormSetup.yaml
file is using ArrayUtility::replaceAndAppendScalarValuesRecursive
to merge the two arrays, see [[https://github.com/TYPO3/typo3/blob/master/typo3/sysext/core/Classes/Configuration/Loader/YamlFileLoader.php#L164-L166]]. For understanding, it is important to know, that the array of the FormSetup.yaml
is merged into the array of the EmailToSender.yaml
-file, so the array of the EmailToSender.yaml
is the first argument in the ArrayUtility::replaceAndAppendScalarValuesRecursive
method-call, see previous link.
ArrayUtility::replaceAndAppendScalarValuesRecursive
¶
The purpose of this method is to merge two arrays like array_replace_recursive
does, with one exception - if the first argument (target array or one if it's subpaths) is a simple (YAML-) list, the key does not get replaced. Instead the source-value-list (array) is appended to the target-value-list (array) using a simple array_merge($target, $source)
, see [[https://github.com/TYPO3/typo3/blob/master/typo3/sysext/core/Classes/Utility/ArrayUtility.php#L940-L943]]. And as mentioned in [[https://www.php.net/manual/en/function.array-merge.php]]:
Values in the input arrays with numeric keys will be renumbered with incrementing keys starting from zero in the result array.
This is where the renumbered keys are created.
// …
// Simple lists get merged / added up
if (!self::isAssociative($array1)) {
return array_merge($array1, $array2);
}
// …
Remember: $array1
is the array from to import, so in the ArrayUtility::isAssociative
the branch containing the 20: null
declaration, looks like not being associative.
ArrayUtility::isAssociative
¶
The current implementation of this method, assumes an array to be associative, when at least one key is a string, see [[https://github.com/TYPO3/typo3/blob/master/typo3/sysext/core/Classes/Utility/ArrayUtility.php#L924-L927]]
// …
public static function isAssociative(array $array): bool
{
return count(array_filter(array_keys($array), 'is_string')) > 0;
}
// …
Is the YAML-declaration 20: null
associative? Yes - we want to override exactly this key. Does the array contain a string as the key? No … now one could argument, to declare the key simply as a string, like '20': null
. But this also does not work, due to special treatment of integer-like string-keys in arrays, which automatically are turned into integers. Here is an example to demonstrate this behaviour in the PHP 7.0 to 8.0:
for PHP in php{70,71,72,73,74,80} ; do
echo "$PHP:"
$PHP -r 'var_dump(array_keys(["10" => null]));'
done
php70:
Command line code:1:
array(1) {
[0] =>
int(10)
}
php71:
Command line code:1:
array(1) {
[0] =>
int(10)
}
php72:
Command line code:1:
array(1) {
[0] =>
int(10)
}
php73:
Command line code:1:
array(1) {
[0] =>
int(10)
}
php74:
Command line code:1:
array(1) {
[0] =>
int(10)
}
php80:
Command line code:1:
array(1) {
[0] =>
int(10)
}
So somehow, we must find a way to identify this case as being associative, while not losing the ability to identify simple lists. What is the difference between the simple list …
[
'value 1',
'value 2',
'value 3',
'value 4'
]
… and the following associative list, with integer-only keys ?
[
10 => 'value',
20 => 'value',
30 => 'value',
40 => 'value'
]
Obviously the keys in the latter array are not numbered consecutively. So the solution to the bug, is to assume an array to be associative, when either one of its keys is a string or all keys are not numbered consecutively (or when the array is empty). The first condition can even be even left out, as real string keys are no integers. So the final solution is:
// …
public static function isAssociative(array $array): bool
{
return !empty($array) && array_keys($array) !== range(0, count($array) - 1);
}
// …
Someone working on the TYPO3-Core had the same idea, see [[https://github.com/TYPO3/typo3/blob/master/typo3/sysext/backend/Classes/Form/Container/SingleFieldContainer.php#L327]]. The updated ArrayUtility
Unit-Tests in the initially linked Gerrit-Review prove, that it is sufficient to just determine if all keys are not numbered consecutively.
Thanks, for your patience!
Stephan
Updated by Stephan Jorek over 3 years ago
- Subject changed from Method ArrayUtility::replaceAndAppendScalarValuesRecursive re-numbers associative but integer-only keys to Method ArrayUtility::replaceAndAppendScalarValuesRecursive re-numbers associative arrays with non-consecutively numbered integer-only keys
Updated by Gerrit Code Review over 3 years ago
- Status changed from New to Under Review
Patch set 1 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/70246
Updated by Stephan Jorek over 3 years ago
- Description updated (diff)
- Private changed from Yes to No
Updated by Gerrit Code Review over 3 years ago
Patch set 2 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/70246
Updated by Gerrit Code Review over 3 years ago
Patch set 3 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/70246
Updated by Gerrit Code Review over 3 years ago
Patch set 4 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/70246
Updated by Gerrit Code Review over 3 years ago
Patch set 5 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/70246
Updated by Stephan Jorek over 3 years ago
- Related to Bug #91236: Numeric array keys are not merged correctly in YAML import added
Updated by Stephan Jorek over 3 years ago
- Related to deleted (Bug #91236: Numeric array keys are not merged correctly in YAML import)
Updated by Stephan Jorek over 3 years ago
- Is duplicate of Bug #91236: Numeric array keys are not merged correctly in YAML import added
Updated by Gerrit Code Review over 3 years ago
Patch set 6 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/70246
Updated by Gerrit Code Review over 3 years ago
Patch set 7 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/70246
Updated by Björn Jacob over 3 years ago
- Category deleted (
Form Framework)
Removed the category "Form Framework". This is not correct. Anyway, thanks for your contribution.
Updated by Gerrit Code Review over 3 years ago
Patch set 8 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/70246
Updated by Gerrit Code Review over 3 years ago
Patch set 9 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/70246
Updated by Gerrit Code Review about 3 years ago
Patch set 10 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/70246
Updated by Gerrit Code Review about 3 years ago
Patch set 11 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/70246
Updated by Gerrit Code Review about 3 years ago
Patch set 12 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/70246
Updated by Gerrit Code Review about 3 years ago
Patch set 13 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/70246
Updated by Gerrit Code Review about 3 years ago
Patch set 14 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/70246
Updated by Gerrit Code Review about 3 years ago
Patch set 15 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/70246
Updated by Gerrit Code Review about 3 years ago
Patch set 16 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/70246
Updated by Helmut Hummel about 3 years ago
First of: Thanks for the detailed report and analysis!
Stephan Jorek wrote:
The purpose of this method is to merge two arrays like
array_replace_recursive
does, with one exception - if the first argument (target array or one if it's subpaths) is a simple (YAML-) list, the key does not get replaced. Instead the source-value-list (array) is appended to the target-value-list (array) using a simplearray_merge($target, $source)
IMHO the main issue here is, to not straight up use array_replace_recursive (or array_merge_recursive) for resolving imports of yaml files, but applying a custom merge algorithm in general purpose code.
Merging "simple lists", but replacing associative keys is required for ckeditor config to be applied cleanly. Determining a simple list in a different way is required for the form definition parsing.
Even if we're going to fix the latter, there will be use cases coming up, where overriding values from lists might be a requirement.
Which merging strategy is the right one, therefore depends on the concrete use case and on users intent when trying to add something to or override something from a default configuration.
The point that I am trying to make is, that we should consider deciding for a particular merge strategy (merge or replace) and adapt the use cases to be able to express the configuration intent with this merge strategy in place.
e.g. if we use array_replace_recursive
(which I would suggest doing), slightly change ckeditor yaml files to be written like
list-property: list-in-first-yaml: [ 'a', 'b' ]
list-property: list-in-second-yaml: [ 'c', 'd' ]
And change the PHP merging code in ckeditor to flatten list-property to a simple list.
By doing so, we would have multiple benefits:
- Form definition issue would be fixed just be removing a custom implementation
- We get rid of a custom implementation of an array merge in general purpose code, in favour of introducing specifics in specific code
- We get the feature for free, that users can decide whether they want to add something to a list or override values from an imported list
Updated by Gerrit Code Review about 3 years ago
Patch set 17 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/70246
Updated by Gerrit Code Review almost 3 years ago
Patch set 18 for branch main of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/70246
Updated by Gerrit Code Review over 1 year ago
Patch set 19 for branch main of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/70246
Updated by Gerrit Code Review over 1 year ago
Patch set 20 for branch main of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/70246
Updated by Gerrit Code Review over 1 year ago
Patch set 21 for branch main of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/70246
Updated by Gerrit Code Review about 1 month ago
Patch set 22 for branch main of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/70246