[Symfony 6.2] Improve SecurityAttributeToIsGrantedAttributeRector

[TomasVotruba]

Follow up to https://github.com/rectorphp/rector-symfony/pull/622

Covered cases:

-#[Security("is_granted('ROLE_ADMIN')")]
+#[\Symfony\Component\Security\Http\Attribute\IsGranted(attribute: 'ROLE_ADMIN')]

-#[Security("is_granted('ROLE_ADMIN', someArea)")]
+#[\Symfony\Component\Security\Http\Attribute\IsGranted(attribute: 'ROLE_ADMIN', subject: 'someArea')]

I've looked at the other cases, but due to it's stringy nature, we can't parse it and covert it into different structures. This is the best we can automate, the rest would have to be dealt with manually.

Closes https://github.com/rectorphp/rector-symfony/issues/393

[TomasVotruba]

cc @wkania I think this is the best we can get :)

[TomasVotruba]

I've noticed array of roles in the Symofny PR: https://github.com/symfony/symfony/pull/46907

Would this help? Is this correct conversion?

-#[Security("is_granted('ROLE_ADMIN') or is_granted('ROLE_FRIENDLY_USER')")]
+#[IsGranted(attributes: ['ROLE_ADMIN', 'ROLE_FRIENDLY_USER'])]

[wkania]

I've looked at the other cases, but due to it's stringy nature, we can't parse it and covert it into different structures.

From the code I see that we do have access to this value so we can modify it? Is parsing complexity the problem?

Would this help? Is this correct conversion?

There are a lot cases with or/and etc so I would be against such changes.

[stefantalen]

I've noticed array of roles in the Symofny PR: https://github.com/symfony/symfony/pull/46907

Would this help? Is this correct conversion?

-#[Security("is_granted('ROLE_ADMIN') or is_granted('ROLE_FRIENDLY_USER')")]

+#[IsGranted(attributes: ['ROLE_ADMIN', 'ROLE_FRIENDLY_USER'])]

Using arrays for roles has been deprecated, from what I know each role needs its own #[IsGranted]

I've recently made some changes in a rule for that

[TomasVotruba]

Using arrays for roles has been deprecated, from what I know each role needs its own #[IsGranted] I've recently made some changes in a rule for that

What kind of changes? Maybe we can includ them here.

[TomasVotruba]

@wkania

From the code I see that we do have access to this value so we can modify it? Is parsing complexity the problem?

The parsing is not possible, because it's not PHP. It's only regex on string :) e.g: https://github.com/rectorphp/rector-symfony/pull/640/files#diff-56e5b788455db473092a5cc9b737f8301fe89a7c60863fd483d0fc71a84e8049R51

[stefantalen]

https://github.com/rectorphp/rector-symfony/issues/583

[TomasVotruba]

@stefantalen I see. That's probably out of scope of this attribute, right?

[TomasVotruba]

The Attribute seems to support multiple roles, at least during it's first PR :)

https://github.com/symfony/symfony/pull/46907/files#diff-60f77e573fc91a3cfc2540e301a990850e2de17216050c55ce1d8b0aaeeccd58R16

[stefantalen]

True, but maybe some logic could be extracted

[TomasVotruba]

I think without before/after tested in the wild, there would be lot of guessing how these work.

Merging as this best we can get at the moment :)

Thanks for feedback everyone :+1: