Skip to content
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

RedirectController parameters pop #30639

Closed
clementmas opened this issue Nov 21, 2019 · 4 comments · Fixed by #30783
Closed

RedirectController parameters pop #30639

clementmas opened this issue Nov 21, 2019 · 4 comments · Fixed by #30783

Comments

@clementmas
Copy link
Contributor

clementmas commented Nov 21, 2019

  • Laravel Version: 6.5.2
  • PHP Version: 7.2.16

Description:

Since I upgraded to Laravel 6, the Route::redirect helper is throwing the error:

Argument 2 passed to Symfony\Component\HttpFoundation\RedirectResponse::__construct() must be of the type integer, null given

Steps To Reproduce:

On a fresh Laravel install, add these routes:

Route::domain('{username}.laravel.test')->group(function() {
    Route::get('/', function () {
        return view('welcome');
    });

    Route::redirect('/old', '/');
});

And add this binding in the RouteServiceProvider:

public function boot()
{
    parent::boot();

    Route::bind('username', function ($value, $route) {
        $route->forgetParameter('username');
    });
}

Now visiting test.laravel.test/old will throw an error.

Source of the bug:

The status and destination parameters of the RedirectController should not be retrieved using pop():

class RedirectController extends Controller
{
    public function __invoke(Request $request, UrlGenerator $url)
    {
        $parameters = collect($request->route()->parameters());

        $status = $parameters->pop();

        $destination = $parameters->pop();

        // [...]
    }
}

This is the dump of the $parameters with the username binding:

Collection {#228 ▼
  #items: array:3 [▼
    "destination" => "/"
    "status" => 302
    "username" => null
  ]
}

So $status = null and $destination = 302.

Now I know this is probably not a common scenario but pop() should be replaced with something like $parameters['status'] and $parameters['destination'] to avoid any bug.

Would you accept a PR to change this? Or am I missing something?

@driesvints
Copy link
Member

Hey there,

Can you first please try one of the support channels below? If you can actually identify this as a bug, feel free to report back and I'll gladly help you out and re-open this issue.

Thanks!

@clementmas
Copy link
Contributor Author

Hi Dries, I thought I already identified the source of the bug in my post. What exactly do you need to re-open this issue?
Unless I'm not supposed to use forgetParameter with subdomain parameters, then it is a bug.
Thanks

@driesvints
Copy link
Member

It doesn't seems likely that you need to call forgetParameter since it's an undocumented method. Can you please try a support channel?

@clementmas
Copy link
Contributor Author

Ok, I posted the issue on the Laracasts Forum.

As @snapey is saying, since the forgetParameter method is public and described in the API, then there's no reason it should lead to a bug.

That's why I suggested a PR to fix the code in RedirectController.
Either by adding ->parametersWithoutNulls() to the first line as @ismaile suggested or by replacing the parameters popping with a cleaner solution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants