Allow merging tasks

[prudloff-insite]

Q	A
Branch	master
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Documented?	no
Fixed tickets

We have two separate YAML files like this:

---
grumphp:
  tasks:
    composer: ~

---
imports:
  - resource: grumphp2.yml
grumphp:
  tasks:
    composer_normalize: ~

This does not work because the import does not merge the task array correctly.
If I dump it, the merged array looks like this:

tasks:
  composer: ~
  1: ~

And it will crash with this error:

PHP Fatal error:  Uncaught TypeError: Argument 1 passed to GrumPHP\Exception\TaskConfigResolverException::unknownTask() must be of the type string, int given, called in /home/pierre/workspace/drupal_d9/vendor/phpro/grumphp/src/Configuration/Compiler/TaskCompilerPass.php on line 42 and defined in /home/pierre/workspace/drupal_d9/vendor/phpro/grumphp/src/Exception/TaskConfigResolverException.php:11
Stack trace:
#0 /home/pierre/workspace/drupal_d9/vendor/phpro/grumphp/src/Configuration/Compiler/TaskCompilerPass.php(42): GrumPHP\Exception\TaskConfigResolverException::unknownTask()
#1 /home/pierre/workspace/drupal_d9/vendor/symfony/dependency-injection/Compiler/Compiler.php(94): GrumPHP\Configuration\Compiler\TaskCompilerPass->process()
#2 /home/pierre/workspace/drupal_d9/vendor/symfony/dependency-injection/ContainerBuilder.php(762): Symfony\Component\DependencyInjection\Compiler\Compiler->compile()
#3 /home/pierre/workspace/drupal_d9/vendor/phpro/grumphp/src/Configuration/ContainerBuilder.php(59): Symfony\Component\DependencyInjec in /home/pierre/workspace/drupal_d9/vendor/phpro/grumphp/src/Exception/TaskConfigResolverException.php on line 11

This PR makes sure the keys are kept when merging the arrays.

[veewee]

Thanks for the PR.

Not sure what the added useAttributeAsKey() does in this case.
If I read the docs correctly, it promotes an attribute in the child object as the name of the task.
But we don't have a name property + the content of the task is variable.

Can you point me in the right direction on this topic so that I can make a better decision on this PR?

[prudloff-insite]

useAttributeAsKey() is indeed primarily used to tell the config loader to use an attribute as the array key.
But it also has the side effect of making the config loader aware that the key has meaning and should not be dropped.

Without this, mergeValues() will drop the key here (because it thinks it is irrelevant): https://github.com/symfony/config/blob/a923cd33330f10d906dd9348f99f45992e61fbcb/Definition/PrototypedArrayNode.php#L305

However, the attribute does not have to be set, as normalizeValue() will take the existing key if the key attribute is null.

So this:

tasks:
  composer: ~

And this:

tasks:
  foo:
    name: composer

Will have the same meaning after this patch is applied.

[veewee]

Meaning that if there is a task out there with the "name" property, it will break the system?
If we are abusing this side-effect, isn't there a way to do the side effect without the main effect? (if that makes sense :) )

[prudloff-insite]

I used name but we could use a more specific attribute.

I found an upstream issue about this and this comment seems to confirm useAttributeAsKey() is the correct way to do this: symfony/symfony#18988 (comment)

[veewee]

Sorry it took me so long to come back to this.
Did some deep testing and it looks fine!

I do suggest naming it a bit more specific so that it wont confluct with any task specific config options.
(Even though name is currently not a parameter on any task)