Rector: BinaryOpBetweenNumberAndStringRector converts strings to integers in comparisons

Created on 19 Dec 2019 · 7Comments · Source: rectorphp/rector

Current Behaviour

[parsing] typo3/sysext/core/Classes/Resource/Utility/ListUtility.php
[refactoring] typo3/sysext/core/Classes/Resource/Utility/ListUtility.php
    [applying] Rector\Php71\Rector\Name\ReservedObjectRector
    [applying] Rector\Php71\Rector\Name\ReservedObjectRector
    [applying] Rector\Php71\Rector\Name\ReservedObjectRector
    [applying] Rector\Php71\Rector\Name\ReservedObjectRector
    [applying] Rector\Php56\Rector\FunctionLike\AddDefaultValueForUndefinedVariableRector
    [applying] Rector\Php70\Rector\FunctionLike\Php4ConstructorRector
    [applying] Rector\Php70\Rector\FunctionLike\ExceptionHandlerTypehintRector
    [applying] Rector\Php71\Rector\Name\ReservedObjectRector
    [applying] Rector\Php71\Rector\Name\ReservedObjectRector
    [applying] Rector\Php71\Rector\Assign\AssignArrayToStringRector
    [applying] Rector\MysqlToMysqli\Rector\Assign\MysqlAssignToMysqliRector
    [applying] Rector\Php70\Rector\List_\ListSplitStringRector
    [applying] Rector\Php71\Rector\Assign\AssignArrayToStringRector
    [applying] Rector\MysqlToMysqli\Rector\Assign\MysqlAssignToMysqliRector
    [applying] Rector\Php70\Rector\List_\ListSplitStringRector
    [applying] Rector\Php71\Rector\Assign\AssignArrayToStringRector
    [applying] Rector\MysqlToMysqli\Rector\Assign\MysqlAssignToMysqliRector
    [applying] Rector\Php70\Rector\List_\ListSplitStringRector
    [applying] Rector\Php71\Rector\FuncCall\RemoveExtraParametersRector
    [applying] Rector\Php70\Rector\MethodCall\ThisCallOnStaticMethodToStaticCallRector
    [applying] Rector\Php71\Rector\Name\ReservedObjectRector
    [applying] Rector\Php71\Rector\Assign\AssignArrayToStringRector
    [applying] Rector\MysqlToMysqli\Rector\Assign\MysqlAssignToMysqliRector
    [applying] Rector\Php70\Rector\List_\ListSplitStringRector
    [applying] Rector\Php71\Rector\FuncCall\RemoveExtraParametersRector
    [applying] Rector\Php70\Rector\MethodCall\ThisCallOnStaticMethodToStaticCallRector
    [applying] Rector\Php71\Rector\Name\ReservedObjectRector
    [applying] Rector\Php70\Rector\If_\IfToSpaceshipRector
    [applying] Rector\Php71\Rector\BinaryOp\BinaryOpBetweenNumberAndStringRector
    [applying] Rector\Php71\Rector\Assign\AssignArrayToStringRector
    [applying] Rector\MysqlToMysqli\Rector\Assign\MysqlAssignToMysqliRector
    [applying] Rector\Php70\Rector\List_\ListSplitStringRector
    [applying] Rector\Php71\Rector\FuncCall\RemoveExtraParametersRector
    [applying] Rector\Php53\Rector\FuncCall\DirNameFileConstantToDirConstantRector
    [applying] Rector\Renaming\Rector\Function_\RenameFunctionRector
    [applying] Rector\Php54\Rector\FuncCall\RemoveReferenceFromCallRector
    [applying] Rector\Php56\Rector\FuncCall\PowToExpRector
    [applying] Rector\MysqlToMysqli\Rector\FuncCall\MysqlFuncCallToMysqliRector
    [applying] Rector\MysqlToMysqli\Rector\FuncCall\MysqlPConnectToMysqliConnectRector
    [applying] Rector\Rector\Argument\SwapFuncCallArgumentsRector
    [applying] Rector\Php70\Rector\FuncCall\RandomFunctionRector
    [applying] Rector\Php70\Rector\FuncCall\MultiDirnameRector
    [applying] Rector\Php70\Rector\FuncCall\CallUserMethodRector
    [applying] Rector\Php70\Rector\FuncCall\EregToPregMatchRector
    [applying] Rector\Php70\Rector\FuncCall\RenameMktimeWithoutArgsToTimeRector
    [applying] Rector\Php71\Rector\Name\ReservedObjectRector
    [applying] Rector\Php71\Rector\FuncCall\RemoveExtraParametersRector
    [applying] Rector\Php70\Rector\MethodCall\ThisCallOnStaticMethodToStaticCallRector
    [applying] Rector\Php71\Rector\Name\ReservedObjectRector
    [applying] Rector\Php71\Rector\BinaryOp\BinaryOpBetweenNumberAndStringRector
    [applying] Rector\Php70\Rector\If_\IfToSpaceshipRector
    [applying] Rector\Php71\Rector\BinaryOp\BinaryOpBetweenNumberAndStringRector
    [applying] Rector\Php71\Rector\BinaryOp\BinaryOpBetweenNumberAndStringRector
    [applying] Rector\Php71\Rector\Assign\AssignArrayToStringRector
    [applying] Rector\MysqlToMysqli\Rector\Assign\MysqlAssignToMysqliRector
    [applying] Rector\Php70\Rector\List_\ListSplitStringRector
    [applying] Rector\Php71\Rector\BinaryOp\BinaryOpBetweenNumberAndStringRector
    [applying] Rector\Php71\Rector\BinaryOp\BinaryOpBetweenNumberAndStringRector
    [applying] Rector\Php71\Rector\BinaryOp\BinaryOpBetweenNumberAndStringRector
    [applying] Rector\Php71\Rector\Assign\AssignArrayToStringRector
    [applying] Rector\MysqlToMysqli\Rector\Assign\MysqlAssignToMysqliRector
    [applying] Rector\Php70\Rector\List_\ListSplitStringRector
[printing] typo3/sysext/core/Classes/Resource/Utility/ListUtility.php


1 file with changes
===================

1) typo3/sysext/core/Classes/Resource/Utility/ListUtility.php

    ---------- begin diff ----------
--- Original
+++ New
@@ -38,7 +38,7 @@
         foreach ($folders as $folder) {
             $name = $folder->getName();
             $role = $folder->getRole();
-            if ($role !== FolderInterface::ROLE_DEFAULT) {
+            if ($role !== 0) {
                 $tempName = htmlspecialchars($lang->sL('LLL:EXT:core/Resources/Private/Language/locallang_mod_file.xlf:role_folder_' . $role));
                 if (!empty($tempName) && ($tempName !== $name)) {
                     // Set new name and append original name
    ----------- end diff -----------

Applied rules:

 * Rector\Php71\Rector\BinaryOp\BinaryOpBetweenNumberAndStringRector

As you can see, the BinaryOpBetweenNumberAndStringRector converts a class constant which represents a string to an integer. I guess, rector does not properly detect the operation here, being a comparison and not an assignment.

Minimal PHP Code Causing Issue

The affected file is part of the TYPO3 project, thus it can be found here:
https://github.com/TYPO3/TYPO3.CMS/blob/v10.2.2/typo3/sysext/core/Classes/Resource/Utility/ListUtility.php

The constant is defined in this class:
https://github.com/TYPO3/TYPO3.CMS/blob/v10.2.2/typo3/sysext/core/Classes/Resource/FolderInterface.php#L25

Expected Behaviour

I expect rector not to change the comparison at all.

bug

Source

alexanderschnitzler

All 7 comments

Thanks for great report, I'll look into it

TomasVotruba on 26 Dec 2019

I'm looking at it closely now. The $role is an integer, that's why the value is converted to 0.

It's an security issue you should fix asap.

I'll update the rule to skip non-variable expressions.

TomasVotruba on 26 Dec 2019

I see. So the inferred type of the left hand side of the comparison takes precedence over the type of the right side. The issue then is the wrong doc block of the getter getRole which returns a string actually.

I guess this can be closed then. If you want to implement that mentioned rule, I am with it but I realized the issue is due to our code base, not due to a malfunctioning rector. :)

alexanderschnitzler on 26 Dec 2019

👍1

Yes, exactly.

I recommend having coding standard and static analysis I'm you CI to prevent these errors.
Here are tips to start: https://www.tomasvotruba.cz/blog/2019/12/23/5-things-i-improve-when-i-get-to-new-repository/#5-fs-ps-pu-2-chars-shortcuts-for-tools-that-help-me

I've updated the rule.

TomasVotruba on 26 Dec 2019

Oh god, I didn't know that github would cross link and pollute this issue. Sorry for that.

alexanderschnitzler on 12 Jan 2020

😄1

No troubles, we're famous :P

TomasVotruba on 12 Jan 2020

I learned this the hard way too. I tried to save some Github referencing of issues, to I put the issue number in the commit. Then I added like 30 fixup commits and rebased/forcepushed and ... you know the rest :D

TomasVotruba on 12 Jan 2020

Was this page helpful?

0 / 5 - 0 ratings