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

[9.x] Fix default parameter bug #42942

Merged
merged 3 commits into from
Jun 24, 2022
Merged

[9.x] Fix default parameter bug #42942

merged 3 commits into from
Jun 24, 2022

Conversation

driesvints
Copy link
Member

This took me the better part of the day to figure out but I finally found a solution that resolves #42707. After the introduction of #42425 several bugs got introduced as well. This is the last remaining one (we know of so far).

Let me try to explain the reasoning here. In the compileParameterNames of the Route object we return all parameter names of a route. These are mapped in an index array from 0 to up. Because this method has no knowledge of what a default parameter is it incorrectly matches a default param instead of the correct regular param. What we need to do is filter out the default params here before we try to map the regular passed params that we feed to the route method. The Route object does not have any knowledge about these so we need to pass in to the route so it can filter the params out. Afterwards we can match our own params properly with the non-default ones.

This fix is a bit of a crutch and I'm not 100% sure it's the correct one but without more examples I cannot make sure it's going to cover all use cases. At least all tests pass now.

Fixes #42707

@mateusjunges
Copy link
Contributor

Just tested it locally and the route is generated as expected.

@taylorotwell
Copy link
Member

Feels a bit like we're entering into kinda rough territory here setting public properties before compiling, etc. I guess I will go ahead and merge this but if we run into anymore issues at all I think we should revert #42425

@taylorotwell taylorotwell merged commit 4d46bda into 9.x Jun 24, 2022
@taylorotwell taylorotwell deleted the fix-url-defaults branch June 24, 2022 17:47
@ghost
Copy link

ghost commented Jul 12, 2022

@driesvints @taylorotwell This causes issues with Octane and Url Defaults as described in this post I made on the laravel discord server:

I have routes like so:

/teams/{team}/products
/teams/{team}/settings

I have a middleware (prioritized before substitute bindings like in the docs) that sets the default value for a $team route parameter to be the value of the current route team:

URL::defaults(['team' => $request->team])

This works when I first navigate to /teams/myteam/products and anytime I refresh or navigate to that page after that. However if I try to go to /teams/myteam/settings (or any other route with {team} in it) then the $team parameter is now null

this only happens on octane (I'm using the latest version of laravel 9 and octane). I don't remember having this issue on laravel 8 so maybe something in the framework changed

denisdulici added a commit to akaunting/akaunting that referenced this pull request Jul 14, 2022
driesvints added a commit that referenced this pull request Jul 15, 2022
taylorotwell pushed a commit that referenced this pull request Jul 15, 2022
@sebastiaanluca
Copy link
Contributor

sebastiaanluca commented Jul 16, 2022

We upgraded Laravel, but one of our tests started failing. Running tests locally, without Octane enabled. Same issue as Cole.

In our test we're making a request, then follow the redirect returned from its response:

$this
    ->actingAs($user)
    ->followingRedirects()
    ->post(route('checkout_cart', $store))
    ->assertViewIs('summary');

The route is defined as:

Route::group(['domain' => config('app.domain'), 'prefix' => 'stores/{storeSlug}', 'middleware' => $middleware], function (): void {
    Route::get('cart/checkout/summary', ShowCartCheckoutSummary::class)->name('checkout_cart');
});

We're also defining a default in a middleware:

URL::defaults(['storeSlug' => $defaultStore->slug])

In in the initial request, the store (slug) is set (from the URL).

In the second request, coming from the response()->redirect() returned from the checkout controller, storeSlug is null. After some debugging, I believe this is due to URL::defaults() not being applied in a second request.

@driesvints
Copy link
Member Author

@sebastiaanluca we already reverted this pr and will be reverting all code related to the original pr as well soon. Tuesday's release will contain the fix.

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 this pull request may close these issues.

Route generation broken
4 participants