New ExtensionPathRector to replace ExtensionPathBase #244
Conversation
|
Please review, the merge conflict is in the use statements, which will move when earlier PR's are merged. These do not impact the change really. They are just annoying to deal with when doing multiple PR's. |
| $extensionType = $extensionTypeValueType->getValue(); | ||
| } | ||
|
|
||
| if (in_array($extensionType, [ |
There was a problem hiding this comment.
Is it possible $extensionType is not a string here? So !is_string in case the constant string value couldn't be detected, such as PHPStan considering it as a union (I don't know how or why.)
There was a problem hiding this comment.
I don't know, i've not audited the code in the Rectors. There might be an edge case imaginable, although i haven't really seen any feedback on this change in any of the regular channels.
When searching gitlab i see this comment nowhere, but since updatebot doesnt do comments this might not mean much.
There was a problem hiding this comment.
We must not be using PHPStan, because it would/should error on types here.
There was a problem hiding this comment.
Why not? Needle can be mixed and we check typesafe. So why error? We handle the non-string value "perfectly".
PHPStan is ran on this code.
There was a problem hiding this comment.
We're at PHPStan level 1
parameters:
level: 1
We can merge as is. It'd probably never error, but it could. We can fix later once PHPStan is bumped up
There was a problem hiding this comment.
Ah, oops. Yeah that is not very helpfull then :x
bbrala
force-pushed
the
feature/extention-path-base-rector
branch
from
7456ad2 to
c2b5821
Compare
Description
New ExtensionPathRector to replace ExtensionPathBase
To Test
Drupal.org issue
Provide a link to the issue from https://www.drupal.org/project/rector/issues. If no issue exists, please create one and link to this PR.