[Removing] Add RemoveAttributeRector#7142
Conversation
|
This rector rule is inspired by this deprecation of an attribute on methods only. |
mttsch
force-pushed
the
feature/RemoveAttributeRector
branch
from
9cd53c4 to
0ea96d0
Compare
mttsch
force-pushed
the
feature/RemoveAttributeRector
branch
from
0ea96d0 to
2a86654
Compare
mttsch
force-pushed
the
feature/RemoveAttributeRector
branch
from
38ff884 to
efa3610
Compare
|
@mttsch Could you avoid these force pushes? I'm having problems with reviews of these PRs. |
| */ | ||
| public function refactor(Node $node): ?Node | ||
| { | ||
| $nodeTypes = [$node::class]; |
There was a problem hiding this comment.
This looks like a hack and prone to be overlapped, the node should covered on visited per definition on getNodeTypes() already, better to add a failing fixture first before this, so we can see possible solution without like this
There was a problem hiding this comment.
@samsonasik The use case here is if you an attribute that supports parameters and properties but will not support parameters anymore in the future (deprecation), thus it must be removed from a promoted property. The issue is that attributes on promoted properties act as attributes on the property and the constructor parameter.
There was a problem hiding this comment.
That should be handled on ClassMethod level, check its AttrGroups and params, in that case, set position of ClassMethod before Param on getNodeTypes(), then you can remove directly under ClassMethod, or use ClassLike instance have __construct method, and remove the child data.
You can revert this hack logic, then sort getNodeTypes() per possible visited node. If, and if needed to clear child, do from the top to down of node, eg: ClassLike -> getProperty/getMethod __construct, remove target.
There was a problem hiding this comment.
Sorry, but I cannot follow your comment to understand what you mean. Can you may-be re-phrase it?
There was a problem hiding this comment.
You can remove the tweak, then we can see the test error to see the better solution.
Nodes are visited from parent to child, if it sorted in order on getNodeTypes() per use case, you can check its child or refactor it child and return the node itself if it has changed.
There was a problem hiding this comment.
@samsonasik I removed the specific code for promoted properties and one code examples fails.
But I still do not understand how the order of node visits is relevant here…
|
I'm looking into this rule and the original use case. What other places we'll use this rule in? |
|
@TomasVotruba Attributes are relatively new so that I think for now, there has not been such a huge need for this case but it might evolve in the future, thus why create a specific rule and we could also have a generalized rule that can also be used in the future by this project for other cases or by third-party rector users for their specific cases. |
|
@mttsch That might be the case, or may not. We start with explicit rules and only if repeated often we switch to generic. I'll close this and re-open if we'll have bigger simple. Thanks for understanding |
|
This pull request has been automatically locked because it has been closed for 150 days. Please open a new PR if you want to continue the work. |
3 participants
No description provided.