rectorphp
AnnotationToAttributeRector for ParamConverter is buggy
AnnotationToAttributeRector for SensioFrameworkExtraBundle's ParamConverter is buggy.
This is what happens to me
- @ParamConverter(name="parametername", class="App\Entity\MyEntity", options={"id"="identifier"})
+ #[ParamConverter(name: 'parametername', class: 'App\Entity\MyEntity', options: ['id' => 'identifier'])]
The Symfony docs https://symfony.com/bundles/SensioFrameworkExtraBundle/current/annotations/converters.html#usage say it should be
+ #[ParamConverter('parametername', class: 'App\Entity\MyEntity', options: ['id' => 'identifier'])]
Hi, what is the buggy part? It preserves original parameter names, that is correct.
Yes but that breaks the code. I've always thought that Rector supports instant upgrades without breaking a code. However if you claim this is not a bug and Rector can't/shouldn't handle this then OK.
How does it break the code? Could you share the reference to param converter class and the errors you get?
Yes, I do get HTTP 500 with the error "Unknown named parameter $name". https://github.com/sensiolabs/SensioFrameworkExtraBundle/blob/v6.1.5/src/Configuration/ParamConverter.php
It can't. It works for annotations because this is how annotations work with public properties. But migrating from annotations to attributes results in the problem described here because attributes use (named) constructor arguments and ParamConverted doesn't have one named name.
I see, the magic is back again :)
Do other Symfony/Sensio annotation have such implicit behavior? So we can deal with it at once.
I can't really say. I reported issue about ParamConverter because I encountered this in my project. The question is whether Rector can/should support this. AnnotationToAttributeRetctor is a generic rector and does not know about such edge cases. Should Rector support this awkward ParamConverter case? Or should not and then should be removed from the Sensio set? Or we can just display a warning for user in the rector run summary that manual changes are required for this class.
I would measure the work needed for both scenarios and then pick the optimal one. Everyone understands that opensource maintainers have a lot of work so "my" issue is not always the most important issue and if there is a quicker workaround we are fine. Replacing ParamConverter(name: 'parametername' with ParamConverter('parametername' is an easy regexp but a warning in console could be very helpful.
a warning in console could be very helpful.
and would show a user that Rector is aware of it's current limitation and people would not treat this as a bug (and not report issues :smile:) On the other hand one could ask if this is OK in terms of Rector's policy of instant upgrade.
I would measure the work needed for both scenarios and then pick the optimal one.
In the ParamConverter case, the factory could detect the class name, then foreach() args and remove named arg with value of "name" to null. It could be 5-lines solution.