Rector: ArgumentAdderRector incorrectly nullifies variable

[u01jmg3]

Bug Report

| Subject | Details |
| :------------- | :-------------------|
| Rector version | v0.8.7@869441e |
| Installed as | composer dependency |

Rector\Generic\Rector\ClassMethod\ArgumentAdderRector is nullifying a variable used in a method call that does not allow null. The use of a trait also seems to affect things.

<?php

namespace App\Http\Controllers\Auth;

use Illuminate\Foundation\Auth\SendsPasswordResetEmails;
use Illuminate\Http\Request;

class ForgotPasswordController
{
    use SendsPasswordResetEmails;

    public function sendResetLink(Request $request)
    {
        $email    = null;
        $response = $this->broker()->sendResetLink($email);

-        return $this->sendResetLinkResponse($request, $response);
+        return $this->sendResetLinkResponse(null, $response);
    }

    protected function sendResetLinkResponse(Request $request, $response)
    {
        $status = trans('passwords.sent', ['email' => $this->email]);

        return back()->with('status', $status);
    }
}

Minimal PHP Code Causing Issue

• https://getrector.org/demo/458eea3d-99f6-4085-9e43-1468b8864add

  • Although this demo does not reproduce the error (because of the use of trait), it is caused by SetList::LARAVEL_57

• Here is the output I get on the command line which is more useful:

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

1) app/Http/Controllers/Auth/ForgotPasswordController.php

    ---------- begin diff ----------
--- Original
+++ New
@@ -14,7 +14,7 @@
         $email    = null;
         $response = $this->broker()->sendResetLink($email);

-        return $this->sendResetLinkResponse($request, $response);
+        return $this->sendResetLinkResponse(null, $response);
     }

     protected function sendResetLinkResponse(Request $request, $response)
    ----------- end diff -----------

Applied rules:

 * Rector\Generic\Rector\ClassMethod\ArgumentAdderRector
 * Rector\Generic\Rector\ClassMethod\ArgumentRemoverRector

 [OK] Rector is done! 1 file has been changed.

Expected Behaviour

Rector should not be nullifying the variable

[TomasVotruba]

Thanks for reporting issue.

We need a failing demo link so we can fix it. The code should be framework independent and isolated. The bug is isually 1-5 lines of PHP code.

You can add both own trait + class into input, e.g. like this:
https://getrector.org/demo/cd6b9276-33ed-4e5d-aa63-ad3e6819b5f4#result

[u01jmg3]

Unfortunately I cannot create a demo of the issue even doing what you have suggested (as Rector no longer reports the problem if I move the trait inline).

Locally, I have copied and pasted the trait inline within my controller and Rector no longer reports a change.

How else can I demo the issue? The issue is still present but not if I use the trait inline.

[TomasVotruba]

Trait should not be removed, but be present in the code with its content.

Another way is to create a Github repository with your minimal code, Rector as dependency and reproducing Gitub Action run that fails. We can fix that

[u01jmg3]

_Trait should not be removed, but be present in the code with its content._

Yep, I just copied the trait so it was inline in preparation for updating my online demo but as I said doing this causes Rector to no longer report a problem making it difficult to come up with a single file reproducing the issue.

As suggested, repo created: https://github.com/u01jmg3/rector-demo
The trait I use comes from Laravel UI (which is one of my Composer dependencies): https://github.com/laravel/ui/blob/3.x/auth-backend/SendsPasswordResetEmails.php

I am not familiar with running Rector as a Github Action. Do you have a workflow I can adapt?

[TomasVotruba]

a problem making it difficult to come up with a single file reproducing the issue.

Yes, that always difficult at start :) but when you narrow it down to the breaking line, it's usually matter of 5-30 minutes to fix the issue for us.

As suggested, repo created

Thank you :+1:

The trait I use comes from Laravel UI

The trait logic needs to be decoupled from the framework and narrowed down the only method that breaks the code.

The composer.json should contains only the rector, nothing external.

I am not familiar with running Rector as a Github Action. Do you have a workflow I can adapt?

Basically the one we have here, just instead of list, use the process with paths:

https://github.com/rectorphp/rector/blob/52ccfc05270ec4e2cd039818017cde89ac9768da/.github/workflows/rector.yaml#L25

[u01jmg3]

_The trait logic needs to be decoupled from the framework and narrowed down the only method that breaks the code._

I can't decouple it entirely otherwise Rector stops reporting the change

_The composer.json should contain only the rector, nothing external._

Reduced to just the one dependency

---

See https://github.com/u01jmg3/rector-demo/runs/1145393948

[u01jmg3]

Solution for now is to use:

/** @noRector \Rector\Generic\Rector\ClassMethod\ArgumentAdderRector */
return ($response === Password::RESET_LINK_SENT) ? $this->sendResetLinkResponse($request, $response) : $this->sendResetLinkFailedResponse($request, $response);

[TomasVotruba]

Thank for detailed test, it helped me to find the weak spot

[u01jmg3]

Okay to delete the test repo? What was the fix?

[TomasVotruba]

The fix was to skip the argument, if is already set https://github.com/rectorphp/rector/pull/4382

Okay to delete the test repo?

Yes, thank you :+1: