setDescription not changed for Commands MakeCommandLazyRector|CommandPropertyToAttributeRector

[johanadivare]

When i have a setDescription in my configure function it is not moved to static property for https://github.com/rectorphp/rector-symfony/blob/main/docs/rector_rules_overview.md#makecommandlazyrector and not moved when i enable https://github.com/rectorphp/rector-symfony/blob/main/docs/rector_rules_overview.md#commandpropertytoattributerector

[TomasVotruba]

Thank you for your report!

We'll need an isolated failing demo link from: http://getrector.org/demo, that way we can reproduce the bug.

[johanadivare]

it would look like this: https://getrector.org/demo/8bd44b67-1f32-4f36-b00a-d1b96a092506

But its says no changes while it is changing it for me like:

<?php

declare(strict_types=1);

namespace App\Command;

use Symfony\Component\Console\Command;
use Symfony\Component\Console\Input\InputInterface;
use Symfony\Component\Console\Output\OutputInterface;

final class TestCommand extends Command
{
    protected static $defaultName = 'cron:test';
    /**
     * {@inheritdoc}
     */
    protected function configure(): void
    {
        $this->setDescription('testing');
    }

    /**
     * {@inheritdoc}
     */
    protected function execute(InputInterface $input, OutputInterface $output): int
    {
        return 0;
    }
}

[TomasVotruba]

Thanks :+1: I see many sets there in rector.php.

It should be only single rule that does the changes, e.g.

$rectorConfig->rule(...::class);

Could you update it?

[johanadivare]

@TomasVotruba that would be https://getrector.org/demo/285433df-5e9e-4b91-b766-2c4ff42de81a. And if i only tun this rule i would get same result as mentioned in https://github.com/rectorphp/rector-symfony/issues/229#issuecomment-1222313485

Thanks for quick reaction!

[TomasVotruba]

I see it now, thanks :+1:

Will look into it

[TomasVotruba]

What Symfony version do you use?

Seem the issue is in wrong Command class name:

-use Symfony\Component\Console\Command;
+use Symfony\Component\Console\Command\Command;

See demo after this fix https://getrector.org/demo/f261ad24-5764-4143-a6f4-ad78bc1fa34b

[johanadivare]

Ah good catch!

But now my problem with https://getrector.org/demo/f261ad24-5764-4143-a6f4-ad78bc1fa34b is that there is still a setDescription i would expect a protected static property for it. Same case with the other config https://getrector.org/demo/eb3a6df7-061b-4001-9fa4-b1b25713110d.

Also when i use https://getrector.org/demo/5b8e8d18-26d2-4a72-bef4-629109c3a5c8 there is no change while i also expect them to change to attribute right?

[TomasVotruba]

Let's take it one by one. What content do you expected for first demo link?

[johanadivare]

I would expect something like:

<?php

declare(strict_types=1);

namespace App\Command;

use Symfony\Component\Console\Command;
use Symfony\Component\Console\Input\InputInterface;
use Symfony\Component\Console\Output\OutputInterface;

final class TestCommand extends Command
{
    protected static $defaultName = 'cron:test';
    protected static $defaultDescription = 'testing';
    /**
     * {@inheritdoc}
     */
    protected function configure(): void
    {
    }

    /**
     * {@inheritdoc}
     */
    protected function execute(InputInterface $input, OutputInterface $output): int
    {
        return 0;
    }
}

Maybe also removing configure because it is empty now but dont know if that is possible

[TomasVotruba]

I see. This was added in Symfony 5.3? https://symfony.com/blog/new-in-symfony-5-3-lazy-command-description

It seems they keep the setDescription() though: https://github.com/symfony/symfony/pull/39851/files#diff-eef83d0198d3a7daee9a487dbaad38bd11b7be712fd0c65ab3b344a97cdaad83

So not sure how the upgrade path should look like :thinking:

[johanadivare]

What do you mean with So not sure how the upgrade path should look like?

The reason why i want the description to be a static property to make the command lazy. When you run bin/console list and you haven't configured the description as a static property it will call the configure function. So in order for config name like MakeCommandLazyRectory to make sense is also to move the description as a static property.

[TomasVotruba]

• Should it look like this one? https://github.com/symfony/symfony/pull/39851/files#diff-eef83d0198d3a7daee9a487dbaad38bd11b7be712fd0c65ab3b344a97cdaad83

• Or the one you provided?

What is the different between those?

I'm not sure the description makes the command not-lazy. Because that would mean no commands were lazy before moving description.

The reason why i want the description to be a static property to make the command lazy. When you run bin/console list and you haven't configured the description as a static property it will call the configure function

Quoting the PR:

However, Symfony commands aren't fully lazy. If you run bin/console without arguments (or the equivalent bin/console list command) you see the entire list of commands registered in the application and their descriptions. This requires instantiating all commands, because their description is not lazy.

This explains a bit, that this feature is only concerned list command. I see :+1:

[johanadivare]

Ah yeah, but would it not make sense to also move the description to static property to make it consistent with command name, and when it is a static property you can skip the function call configure when running bin/console list

[TomasVotruba]

I do not use these kinds of command, so it's hard to image it for me. Could you share a final PHP code snippet, how it should look like?

[johanadivare]

Yeah i can imagine then. It would look like the example off my comment in https://github.com/rectorphp/rector-symfony/issues/229#issuecomment-1222433331 and like i mentioned there it could make sense to remove function configure as it is not empty but dont know if that is possible with Rector.

[johanadivare]

@TomasVotruba can you have a look at my comment when you have time https://github.com/rectorphp/rector-symfony/issues/229#issuecomment-1225238668

[TomasVotruba]

The next step here would be a failing test fixture via PR and then a fix.

[TomasVotruba]

You can check this PR for inspiration: https://github.com/rectorphp/rector-symfony/pull/238/files

Here is the docs on how to add the test fixture: https://github.com/rectorphp/rector/blob/main/docs/how_to_add_test_for_rector_rule.md

[JohJohan]

@TomasVotruba i added pull request: https://github.com/rectorphp/rector-symfony/pull/245

[JohJohan]

Fixed with https://github.com/rectorphp/rector-symfony/pull/245 I don't see the option to close the issue from GitHub app.

[johanadivare]

Ah i created the issue from my work account closed it.