search

phpro / grumphp

A PHP code-quality tool
MIT License
4.14k stars 430 forks source link

Allow merging tasks #829

Closed prudloff-insite closed 3 years ago

prudloff-insite commented 3 years ago
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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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: https://github.com/symfony/symfony/issues/18988#issuecomment-224234613