rectorphp / rector

Instant Upgrades and Automated Refactoring of any PHP 5.3+ code
https://getrector.com
MIT License
8.71k stars 686 forks source link

Incorrect behavior of SwapFuncCallArgumentsRector, RenameFunctionRector #7493

Closed JamesBollington closed 1 year ago

JamesBollington commented 2 years ago

Bug Report

Hi, I'm expecting here that mysql_query gets renamed mysqli_query and the parameters swapped. However all that's happening is the first parameter is deleted and the function isn't renamed. Unsure if I've maybe configured this incorrectly?

config is as follows (am using the demo):

`use Rector\Config\RectorConfig; use Rector\Php74\Rector\Property\TypedPropertyRector; use Rector\Set\ValueObject\SetList; use MysqlQueryMysqlErrorWithLinkRector;

return static function (RectorConfig $rectorConfig): void { // A. run whole set $rectorConfig->sets([ SetList::MYSQL_TO_MYSQLI, ]);

// B. or single rule
$rectorConfig->rule(TypedPropertyRector::class);

};`

Subject Details
Rector version last dev-main
Installed as composer dependency

Minimal PHP Code Causing Issue

See https://getrector.org/demo/e9378e6a-04dc-4ec5-9a47-58f744372868

<?php
        $sql_delivery_method = "SELECT * FROM DeliveryMethod";
        $query_delivery_method = mysql_query($sql_delivery_method, conn());
        $rownum_delivery_method = mysql_num_rows($query_delivery_method);
        $row_delivery_method = mysql_fetch_array($query_delivery_method);
        for ($i = 0; $i < $rownum_delivery_method; $i++) {
            echo "_delivery_method[" . $i . "] = new Array('" . $row_delivery_method['enquiry_delivery_no'] . "','" . $row_delivery_method['enquiry_delivery'] . "')\n";
            $row_delivery_method = mysql_fetch_array($query_delivery_method);
        }

Responsible rules

Expected Behavior

brianfreytag commented 1 year ago

I'm having the same issue with mysql_query -> mysqli_query doing weird stuff:

$sql = "SQLQUERY'";
- if (!mysql_query($sql, $link)) {
+ if (!mysql_query($link)) {
       die('Error: ' . mysql_error());
   }

You can see in this changeset

a) Does not convert mysql_query to mysqli_query b) Changes mysql_query($sql, $link) to mysql_query($link) c) Does not convert mysql_error to mysqli_error

Using the rector web-based tool, I think the issues comes up because $link is not defined in the file. It's in a file that's required a couple levels up:

As you can see, I'm getting $link through a require that precedes the inclusion of the file that the mysql call is located.

You can see how it converts differently at the following rector links:

Incorrect: https://getrector.com/demo/f51fb226-49cd-4404-a903-1cabd2bd21be Correct: https://getrector.com/demo/9992453f-fd29-4c3c-8d70-0e888ecf1df7

It doesn't seem to be able to handle the idea that a variable is defined outside of the file in question.

TomasVotruba commented 1 year ago

Mysql to Mysqli migration is based in procedural historical code. Based on your feedback in Stackoverflow comments, it causes more confusion and harm of false positive:

As much as we'd like to set this reliable and working, it's not ready yet to be helpful now and we better withdraw it: https://github.com/rectorphp/rector-src/pull/4448


How to refactor in more reliable way?