Env variables don't get expanded in task definitions

[indykoning]

Q	A
Version	GrumPHP 2.6.0
Bug?	yes
New feature?	no
Question?	no
Documentation?	yes

Following the docs on using environment variables and parameters doesn't actually function as expected.
The example seems to work, however setting the variable to false still has the config enabled.

i've debugged it by using it in a task and dumping the task definition in the code. Which results in:

GrumPHP\Task\Config\Metadata^ {#659
  -metadata: array:5 [
    "blocking" => true
    "task" => ""
    "label" => ""
    "priority" => 0
    "enabled" => "env_19eeff9b12d8f08c_bool_GRUM_COMPOSER_NORMALIZE_ENABLED_b9f45633a4808b98cb74867bcd84c333"
  ]
}

instead of the expected bool value, it is a string identifying the variable to be resolved.
This feels similar to symfony/symfony#45868

My configuration

# grumphp.yml
parameters:
    env(GRUM_COMPOSER_NORMALIZE_ENABLED): 'false'
    env(GRUM_COMPOSER_NORMALIZE_VERBOSE): 'false'
    
grumphp:
    tasks:
        composer_normalize:
            metadata:
                enabled: '%env(bool:GRUM_COMPOSER_NORMALIZE_ENABLED)%'
            indent_size: 4
            indent_style: "space"
            no_update_lock: false
            verbose: '%env(bool:GRUM_COMPOSER_NORMALIZE_VERBOSE)%'

Steps to reproduce:

# Generate empty folder
mkdir tmp
cd tmp
git init
echo "vendor" > .gitignore
pbpaste > grumphp.yml
composer require --dev phpro/grumphp

# Run GrumPHP:
git add -A && git commit -m"Test"
# or
./vendor/bin/grumphp run

Result:

Composer normalize is still excecuted instead of not excecuting at all

[veewee]

Thanks for reporting, I can confirm this is a bug but haven't found a solution yet.
Feel free to dive into the specifics to figure out what is going wrong here.

What I've found so far is that the \GrumPHP\Configuration\Compiler\TaskCompilerPass is being executed before the env vars are being replaced inside \Symfony\Component\DependencyInjection\Compiler\ResolveEnvPlaceholdersPass when executing $container->compile(true); inside \GrumPHP\Configuration\ContainerBuilder::buildFromConfiguration().

This results in the task instances that are registered inside the container to have this env_19eeff9b12d8f08c_bool_GRUM_COMPOSER_NORMALIZE_ENABLED_b9f45633a4808b98cb74867bcd84c333 placeholder instead of the actual replaced value.

[kira0269]

Hello there,

I encountered the same issue and I think I found the problem and a possible solution.

The actual problem is the \GrumPHP\Configuration\Compiler\TaskCompilerPass retrieves config from the container then directly instantiates objects with it. At this point, the configuration is not resolved (neither env vars nor parameters), so the created objects contain placeholders and not the resolved values. The \Symfony\Component\DependencyInjection\Compiler\ResolveEnvPlaceholdersPass will resolve env var placeholders, but on all definitions through the container. It will not resolve all object attributes.

The \Symfony\Component\DependencyInjection\Compiler\ResolveEnvPlaceholdersPass is configured to resolve %env(XXX)% at the very end because environment variables are supposed to be used at runtime, and not at compile time (see the comment of Nicolas Grekas here).

A possible solution is to move all object instantiation logic from the TaskCompilerPass to the TaskConfigurator.
The TaskCompilerPass MUST manipulates services definitions, not the services instances.

Also, if you really want to use come config during build time and take it in count in TaskCompilerPass, you should indicate in the documentation that those parameters can not be based on env vars.

Please fin below the changes I made to unlock the situation (the only residual behavior to adapt is the "enable/disable task" thing):

Changes in the compiler pass:

// TaskCompilerPass
namespace GrumPHP\Configuration\Compiler;

// ...

class TaskCompilerPass implements CompilerPassInterface
{


    public function process(ContainerBuilder $container): void
    {
            // It's better to use synthetic services
            $taskConfigResolver = $this->buildTaskConfigResolver($availableTasks);
            $container->set('grumphp.task_config_resolver', $taskConfigResolver);

            // ...

            // Do not create objects, keep using config as array in service definitions 
            // so that the env vars placeholders can be resolved
            $metadataConfig = array_merge(
                ['priority' => $taskInfo['priority']],
                $metadataConfig
            );

            // TODO Services can not be removed from the container based on env vars (APP_ENV and APP_DEBUG are the exceptions
            //   Configure a "TaskResolver" which will contain this logic
            // Disabled tasks can be skipped
            // This allows to conditionally disable tasks through parameters or by an extension.
           // if (!$metadata->isEnabled()) {
           //     continue;
           // }

            // Configure task:
            $taskBuilder = new Definition($taskClass);
            $taskBuilder->setFactory([new Reference(TaskConfigurator::class), '__invoke']);

            // In order to be resolved, env vars used in definitions MUST be readable in other definitions or arrays or strings.
            $taskBuilder->setArguments([
                new Reference($taskId),
                new Reference('grumphp.task_config_resolver'),
                $taskName,
                $currentTaskName,
                $taskConfig,
                $metadataConfig,
            ]);
            $taskBuilder->addTag('configured.task');

            // ...
    }

Changes in the task factory:

namespace GrumPHP\Configuration\Configurator;

use GrumPHP\Configuration\Resolver\TaskConfigResolver;
use GrumPHP\Task\Config\LazyTaskConfig;
use GrumPHP\Task\Config\Metadata;
use GrumPHP\Task\Config\TaskConfig;
use GrumPHP\Task\Config\TaskConfigInterface;
use GrumPHP\Task\TaskInterface;

class TaskConfigurator
{
    public function __invoke(
        TaskInterface $task,
        TaskConfigResolver $taskConfigResolver, 
        string $taskName, 
        string $currentTaskName, 
        array $taskConfig, 
        array $metadataConfig,
    ): TaskInterface
    {
        $config = new LazyTaskConfig(
            function () use ($taskName, $taskConfigResolver, $currentTaskName, $taskConfig, $metadataConfig) {
                return new TaskConfig(
                    $taskName,
                    $taskConfigResolver->resolve($currentTaskName, $taskConfig),
                    new Metadata($metadataConfig)
                );
            }
        );

        return $task->withConfig($config);
    }
}

If you need some help, I'll try to make a PR.

[veewee]

That's some fine feedback @kira0269 !

So if I understand correctly, the main issue is in the fact that the LazyTaskConfig is containing information from the 'compiled' container and not the runtime contianer which has the environment variables resolved.

I'm thinking that following solution might be easier then:

Since the LazyTaskConfig is executed when the task first loads it's config (during running the tasks), it might make sense to grab the information again directly from the container. The CompilerPass has access to the container and can just grab it when loading the configuration. A possible problem it might introduce and should be avoided, is that the container is serialized into the async task. But that way, you always should have the runtime information available.

Cheching wether the tasks is enabled or not, could be done from within the TaskRunner class instead of from within the compiler pass. That way, it could also work with env vars.

[kira0269]

To reword my last comment:

The main issue is the TaskCompilerPass can not rely on env vars since en vars are not resolved at compile time.
The only job of it should be to manipulate Task service definitions, like:

$container->autowire($taskId, $taskClass)
    ->setFactory([new Reference(TaskConfigurator::class), '__invoke'])
    ->setArguments([
        $task,
        $taskConfig, // Config as array from the grumphp.yaml file
    ]);

Then, the configuration of the Task (available via a TaskManager, why not) should be executed at runtime in the TaskConfigurator

[kira0269]

Why do you mean by "the container is serialized into the async task" ?

I think a better way would be to dump it like in a Symfony app.

[veewee]

Owkay, I think I get what you are saying.

Implementation wise, there are already quite some classes and code just to get the task configured.
I think we should inspect changing little parts to make this possible over introducing a lot of new concepts.

One path that crosses my mind, is to change how the TaskConfigResolver works:

If we can make it a definition instead of a synthetic service and make sure all the raw configuration data needed is available in there through arguments, then the environment variable resolving should work fine. This goes for both the task configuration and metadata.

That way, most of the TaskCompilerPass can just keep on working as it does now minus the isEnabled part. That should got in the TaskRunner in order for it to work with env vars.

[kira0269]

The changes I proposed in my first comment resolved the issue.

But I didn't test the async task part.

[veewee]

@kira0269 I've created a quick draft of what change I had in mind:
#1177

This seems to be doing the trick as well. Your feedback is welcome.

(it's just a draft to get some early feedback - will finish it up in the coming x)