rectorphp / rector-symfony

Rector upgrade rules for Symfony
http://getrector.com
MIT License
179 stars 86 forks source link

Symfony 6.1 - `AsCommand` addition not correct when using constants in name #638

Closed etshy closed 2 months ago

etshy commented 2 months ago

(I'll apologize here as I'll likely report a few bugs while I'm upgrading projects)

When using constants in a Command name like this

protected static $defaultName = 'app:'.Entity::COMMAND1.':process';

The name is not moved to the AsCommand attributes, making it not valid.

#[AsCommand(description: 'blabla')]

When it could be created like this

#[AsCommand(name: 'app:'.Entity::COMMAND1.':process', description: 'blabla')]

The name should take whatever is between $defaultName = and the semi-colon.

ps : I'm not sure why these commands was even made that way, but as they're dynamically run by other process I guess it was to be sure of the command to run

TomasVotruba commented 2 months ago

Thanks for reporting!

/cc @JohJohan

TomasVotruba commented 2 months ago

(I'll apologize here as I'll likely report a few bugs while I'm upgrading projects)

Btw, this is mostly welcomed :) we need testing in the wild and often polish existing rules based on reports like your. Thank you for your time and patience :+1:

TomasVotruba commented 2 months ago

Looking into this...

TomasVotruba commented 2 months ago

Fixed in: https://github.com/rectorphp/rector-symfony/pull/643 :+1:

You can give it a try now using dev-main version of Rector. There might be few more cases to be covered, so feel free to report :+1:

etshy commented 2 months ago

That was fast ! I'll try it soon on the next project I migrate

etshy commented 2 months ago

Agh, I can't test because dev-main of rector/rector-symfony need php:8.2 while we're still in php:8.1 (planning to update to php 8.4 when it's released in a few months).

I'll make a light symfony project at home to try it.

TomasVotruba commented 2 months ago

The dev-main of rector/rector. This is only meta package that is included in the release.

etshy commented 2 months ago

Good news, it works, thanks ! Not really important but for some reason it creates the name param after the description param now though.

TomasVotruba commented 2 months ago

@etshy Thanks for quick testing :+1:

Not really important but for some reason it creates the name param after the description param now though.

That's a bit annoying. Could you share exact test fixture where this happens via https://getrector.com/demo? I'll look into it

etshy commented 2 months ago

Sure. https://getrector.com/demo/c1d3944a-cc30-4236-a08e-9f5d02f14ea6 I made it as light as possible but still reproducing the issue.

Noticed the description method stays even if empty after refactoring too.

ps : This sandbox is amazing btw

samsonasik commented 2 months ago

it seems moving self::command to class attribute cause error, is that the issue, ref https://3v4l.org/p9206

samsonasik commented 2 months ago

hm.., when COMMAND constant exists, it seems still working https://3v4l.org/1lS5n

samsonasik commented 2 months ago

so, the issue is calling self::command instead of self::COMMAND ?

working https://3v4l.org/1lS5n not working (constant not exists) https://3v4l.org/qTKeQ

TomasVotruba commented 2 months ago

@etshy

ps : This sandbox is amazing btw

Awesome to hear that :pray:

@samsonasik Thanks for checking. Could you add fix for this one so name is always first?

etshy commented 2 months ago

Sorry for the mistake about the constant case in demo, my bad. That doesn't change the order of the param though and rector doesn't change the case

edit : here is an updated demo : https://getrector.com/demo/e205ab89-c170-411b-b9eb-f6b1fdb039d8

samsonasik commented 2 months ago

@etshy what the issue, is order argument matter in symfony?

etshy commented 2 months ago

Oh no like I said it's not that important, just generating a weak warning on phpstorm. Nothing mandatory or whatever.

samsonasik commented 2 months ago

@TomasVotruba I think it is fine as is due to multiple rules apply and it doesn't change the functionality

JohJohan commented 2 months ago

I can also confirm that this will be input output for the testfixutre @TomasVotruba wrote:

<?php

namespace Rector\Symfony\Tests\Symfony61\Rector\Class_\CommandPropertyToAttributeRector\Fixture;

class WithConstantInName extends \Symfony\Component\Console\Command\Command
{
    private const NAME = 'infix';

    /** @var string|null */
    public static $defaultName = 'start' . self::INVALID;
    public static $defaultDescription = 'defaultDescription';
}

?>
-----
<?php

namespace Rector\Symfony\Tests\Symfony61\Rector\Class_\CommandPropertyToAttributeRector\Fixture;

#[\Symfony\Component\Console\Attribute\AsCommand(name: 'start' . self::INVALID, description: 'defaultDescription')]
class WithConstantInName extends \Symfony\Component\Console\Command\Command
{
    private const NAME = 'infix';
}

?>

so description is added after name. Probably due to the fact setDescription is used than.

TomasVotruba commented 2 months ago

I see. It happens when 2 rules are run together:

Looking into it...

TomasVotruba commented 2 months ago

Here is my attempt :) https://github.com/rectorphp/rector-symfony/pull/644