-
Notifications
You must be signed in to change notification settings - Fork 24
DowngradeMbStrContainsRector #331
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
|
This needs map opposite fixtures on rector-src on PR: so we have similar functionality |
samsonasik
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs map opposite fixtures on rector-src on PR:
so we have similar functionality
$isMatch = str_contains(mb_substr('abc', $offset), 'a');Convert to $isMatch = mb_strpos('abc', 'a', $offset) !== false;Am I right? |
|
Yes, ensure the value is match with upgrade code, the similar logic/service can be used. |
| { | ||
| public function run() | ||
| { | ||
| $isMatch = str_contains('😊abc', 'a'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am thinking if the mb_strpos() only when needle is exact string and has multibyte, the original str_contains() to strpos() already converted on DowngradeStrContainsRector, so should be a logic to verify that the needle has multibye, eg:
strlen($needle) !== mb_strlen($needle)this, however, I prefer to not be part of downgrade php 8.0 set, as dealing with multibyte can be "there on purpose", user that needs exact change str_contains -> mb_strpos needs to enable manually as use and know the risk.
| { | ||
| public function run() | ||
| { | ||
| return str_contains('abc', 'a'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only when needle has multibyte value as well:
| return str_contains('abc', 'a'); | |
| return str_contains('😊abc', '😊a'); |
| { | ||
| public function run() | ||
| { | ||
| return mb_strpos('abc', 'a') !== false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only when needle has multibyte value as well:
| return mb_strpos('abc', 'a') !== false; | |
| return mb_strpos('😊abc', '😊a') !== false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok
| { | ||
| public function run() | ||
| { | ||
| $isMatch = !str_contains('abc', 'a'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these fixtures are invalid, as no multibyte in needle, only downgrade to mb_strpos when needle has multibyte char
$isMatch = !str_contains('😊abc', 😊'a');
config/set/downgrade-php80.php
Outdated
| DowngradePropertyPromotionRector::class, | ||
| DowngradeNonCapturingCatchesRector::class, | ||
| DowngradeStrContainsRector::class, | ||
| DowngradeMbStrContainsRector::class, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unregister this, there is no "mb_str_contains", only str_contains, that means that utilize strpos can be on purpose.
let user manually use if needed, dealing with user that may don't have mbstring extension or use strpos on purpose :)
user that use mb_ functions know why they are using the mb_ functions :)
| if (! $funcCall instanceof FuncCall) { | ||
| return null; | ||
| } | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
early skip first class callable here:
if ($funcCall->isFirstClassCallable()) {
return null;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (!$funcCall->isFirstClassCallable()) {
return null;
}Correct ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no, should be below if (! $funcCall instanceof FuncCall) {:
if (! $funcCall instanceof FuncCall) {
return null;
}
if ($funcCall->isFirstClassCallable()) {
return null;
}| { | ||
| public function run() | ||
| { | ||
| $isMatch = str_contains('😊abc', '😊a'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add test fixture to skip no multibyte needle:
$isMatch = str_contains('abc', 'a');There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added the string without multibyte needle.
| if (! $this->isName($haystack->name, 'mb_substr')) { | ||
| return null; | ||
| } | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
skip first class callable haystack as well.
| * | ||
| * @see \Rector\Tests\DowngradePhp80\Rector\FuncCall\DowngradeMbStrContainsRector\DowngradeMbStrContainsRectorTest | ||
| */ | ||
| final class DowngradeMbStrContainsRector extends AbstractRector |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer to rename to something like: DowngradeStrContainsWithMultibyteNeedleRector since there is no mb_str_contains, make clearer it actually use when needle has multibyte.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok
|
After some checks, this rule is not needed, strpos is fine, since it doesn't verify position, only check non-falsy result, so strpos was fine as is, even needle contains multibyte, see I am closing this as that reasoning. Thank you for understanding. |
No description provided.