Project

General

Profile

Actions

Bug #94729

open

Method ArrayUtility::replaceAndAppendScalarValuesRecursive re-numbers associative arrays with non-consecutively numbered integer-only keys

Added by Stephan Jorek over 2 years ago. Updated 12 months ago.

Status:
Under Review
Priority:
Should have
Assignee:
-
Category:
-
Start date:
2021-08-05
Due date:
% Done:

0%

Estimated time:
TYPO3 Version:
10
PHP Version:
7.4
Tags:
Complexity:
Is Regression:
Yes
Sprint Focus:

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


Related issues 1 (1 open0 closed)

Is duplicate of TYPO3 Core - Bug #91236: Numeric array keys are not merged correctly in YAML importNew2020-04-29

Actions
Actions #1

Updated by Stephan Jorek over 2 years ago

  • Description updated (diff)
Actions #2

Updated by Stephan Jorek over 2 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
Actions #3

Updated by Gerrit Code Review over 2 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

Actions #4

Updated by Stephan Jorek over 2 years ago

  • Description updated (diff)
  • Private changed from Yes to No
Actions #5

Updated by Gerrit Code Review over 2 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

Actions #6

Updated by Gerrit Code Review over 2 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

Actions #7

Updated by Gerrit Code Review over 2 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

Actions #8

Updated by Gerrit Code Review over 2 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

Actions #9

Updated by Stephan Jorek over 2 years ago

  • Description updated (diff)
Actions #10

Updated by Stephan Jorek over 2 years ago

  • Related to Bug #91236: Numeric array keys are not merged correctly in YAML import added
Actions #11

Updated by Stephan Jorek over 2 years ago

  • Related to deleted (Bug #91236: Numeric array keys are not merged correctly in YAML import)
Actions #12

Updated by Stephan Jorek over 2 years ago

  • Is duplicate of Bug #91236: Numeric array keys are not merged correctly in YAML import added
Actions #13

Updated by Gerrit Code Review over 2 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

Actions #14

Updated by Gerrit Code Review over 2 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

Actions #15

Updated by Björn Jacob over 2 years ago

  • Category deleted (Form Framework)

Removed the category "Form Framework". This is not correct. Anyway, thanks for your contribution.

Actions #16

Updated by Gerrit Code Review over 2 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

Actions #17

Updated by Gerrit Code Review over 2 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

Actions #18

Updated by Gerrit Code Review over 2 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

Actions #19

Updated by Gerrit Code Review over 2 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

Actions #20

Updated by Gerrit Code Review over 2 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

Actions #21

Updated by Gerrit Code Review over 2 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

Actions #22

Updated by Gerrit Code Review over 2 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

Actions #23

Updated by Gerrit Code Review over 2 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

Actions #24

Updated by Gerrit Code Review over 2 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

Actions #25

Updated by Helmut Hummel over 2 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 simple array_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:

  1. Form definition issue would be fixed just be removing a custom implementation
  2. We get rid of a custom implementation of an array merge in general purpose code, in favour of introducing specifics in specific code
  3. 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
Actions #26

Updated by Gerrit Code Review over 2 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

Actions #27

Updated by Gerrit Code Review over 2 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

Actions #28

Updated by Gerrit Code Review 12 months 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

Actions #29

Updated by Gerrit Code Review 12 months 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

Actions #30

Updated by Gerrit Code Review 12 months 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

Actions

Also available in: Atom PDF