-
Notifications
You must be signed in to change notification settings - Fork 94
[Symfony 6.2] Improve SecurityAttributeToIsGrantedAttributeRector #640
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
da078ce
to
52cdd1b
Compare
cc @wkania I think this is the best we can get :) |
I've noticed array of roles in the Symofny PR: symfony/symfony#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'])] |
From the code I see that we do have access to this value so we can modify it? Is parsing complexity the problem?
There are a lot cases with or/and etc so I would be against such changes. |
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. |
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 I see. That's probably out of scope of this attribute, right? |
The Attribute seems to support multiple roles, at least during it's first PR :) |
True, but maybe some logic could be extracted |
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 👍 |
Follow up to #622
Covered cases:
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 #393