MbStrFunctionsFixer - Add "str_split" => "mb_str_split" mapping [PHP7.4] #4534
Conversation
There was a problem hiding this comment.
Sorry @SpacePossum , I had to revert your PR.
can you take over this one, please?
| $this->functions = array_filter( | ||
| self::$functionsMap, | ||
| static function (array $mapping) { | ||
| return \function_exists($mapping['alternativeName']); |
There was a problem hiding this comment.
this is work way to do the check. during runtime, we have ext-mb polyfilled, thus we alternativeName always exists
vide #4380 (comment)
vide #4529
There was a problem hiding this comment.
There was a problem hiding this comment.
Looking nice!
Yet, to be honest, i'm not sure is it the way we want to follow - implicit handling of decision "should we transform this function or not" by detecting isInternal or current runtime (that still can be different than end environment), or explicit configuration (like we do eg for PHPUnit - we aim version X or Y, regardless what is installed in repo we are fixing).
What do you think, @Slamdunk ?
There was a problem hiding this comment.
I'm gonna talk about this particular Fixer only: this Fixer aims to not forget anywhere any non-multi-byte string comparison. Why should anyone could want to fix one function but not the others?
by detecting
isInternalor current runtime (that still can be different than end environment)
That would be a user's fault: this tool it is intended to be run with the same binary used by the production, so no worry to me about this point
explicit configuration (like we do eg for PHPUnit - we aim version X or Y, regardless what is installed in repo we are fixing)
As a user myself of PHP-CS-Fixer the fact that PHPUnit related fixers are by explicit configuration represents just a lack of the tool, not a cool feature. What would be the purpose of having PHPUnit 999 installed and tells PHP-CS-Fixer to to transition the code from PHPUnit 1 to PHPUnit 2? The installed version and targeted transition one should always be the same to me.
Here I don't question what was done: it is a safety net for PHP-CS-Fixer maintainers and it's good for now; just talking about theory here 😉
There was a problem hiding this comment.
Why should anyone could want to fix one function but not the others?
vide issue reported - because he does not had the other function (mb_str_split) and code execution after fixing was crashing for him (We had installed polyfill, he didn't).
for phpunit - well, one can has it installed as global dependency or phar file, and we cannot autodetect version.
agree - if we expect to run on same runtime (which we do), then it's not an issue if we don't modify the runtime on the fly (by adding pollyfills). then, maybe isInternal is good enough for us here.
|
Fix proposed to @SpacePossum : https://github.com/SpacePossum/PHP-CS-Fixer/pull/24 |
|
TBH I didn't see the need for the revert as now we still have the same issue with all other function calls, just not the new one added. The additional guard for the internal check sounds good to me, not sure if it out performs using a look up in the |
well, not that much users of PHP CS Fixer already using PHP 7.4, and for all of them that are not using it nor the polyfill, we were breaking the result file. |
|
@keradus anything else you want here? |
This reverts commit d35c431.
…f functions to fix
SpacePossum
changed the title
|
Thank you @keradus. |
…keradus, Slamdunk) This PR was merged into the 2.15 branch. Discussion ---------- Redo PHP7.4 - Add "str_split" => "mb_str_split" mapping This reverts commit d35c431. Reapply #4380 by reverting #4533 Commits ------- 083d062 mb_*: rely on ReflectionFunction::isInternal to detect correct list of functions to fix 3fea39f Revert "Revert PHP7.4 - Add \"str_split\" => \"mb_str_split\" mapping"
|
thanks @Slamdunk as well :D |
This reverts commit d35c431.
Reapply #4380 by reverting #4533