laravel / framework

The Laravel Framework.
https://laravel.com
MIT License
32.38k stars 10.97k forks source link

[L6.6.0] Breaking routing bug #30698

Closed neoteknic closed 4 years ago

neoteknic commented 4 years ago

Description:

There is a critical routing bug in L6.6.0 Working in L6.5.2 and less

Steps To Reproduce:

Two routes like this (routes/web.php) : Route::prefix('admin')->group(function() { Route::get('boutiques','AdminComparateurController@shops')->name('admin.shops'); Route::get('boutiques/{shop}','AdminComparateurController@shopId')->name('admin.shops.id')->where(['shop'=>'[0-9]+']); });

Go to the URL : /admin/boutiques In L6.6.0 error 500 : Missing required parameters for [Route: admin.shops.id] [URI: admin/boutiques/{shop}]

It take the boutiques/{shop} route instead of the first one, changing orders of the route : same bug It break the application !

brunogaspar commented 4 years ago

@neoteknic Can you check if https://github.com/laravel/framework/pull/30659 is the cuplrit? I haven't personally updated to 6.6.0 yet, but that seems the only relevant change as far as i could quickly tell from the changelog.

neoteknic commented 4 years ago

Ok it's the commit f610830bfce85090373ea4a3f87849949461fd67 in Routing/RouteUrlGenerator.php that break the route changing this fix the bug : return (empty($parameters) && ! Str::endsWith($match[0], '?}')) //return (! isset($parameters[0]) && ! Str::endsWith($match[0], '?}'))

neoteknic commented 4 years ago

@brunogaspar yes it's the #30659 is the culprit maybe revert ? (need better fix to #30659) : https://github.com/laravel/framework/commit/f610830bfce85090373ea4a3f87849949461fd67

brunogaspar commented 4 years ago

Ya, needs to be reverted it seems and yes, a better fix will need to be implemented i guess.

Hopefully @driesvints can revert it and do a patch release ASAP.

GuidoHendriks commented 4 years ago

How do you call route() in there?

neoteknic commented 4 years ago

I don' call route(), I just go to the url and it take the wrong url : example_url/{value} instead of example_url, so error (missing value) my routes are in a group prefix function, I did'nt tried without.

crynobone commented 4 years ago

example_url/{value} instead of example_url, so error (missing value)

Are you certain that's the case? Have you check with a fresh install of Laravel. because from the limited error shared here it does suggest that route('admin.shops.id') being used, and resolving routing doesn't use that.


https://github.com/laravel/framework/commit/f610830bfce85090373ea4a3f87849949461fd67

The referred commit also suggested that it is related class used for resolving/generating URL and not resolving routing.

GuidoHendriks commented 4 years ago

The exception you're getting is from the URL generator, not from resolving the route of a request. My bet is still that you're calling route('admin.shops.id', [...]) from AdminComparateurController@shops or the view that's loaded from there.

neoteknic commented 4 years ago

Oh ! I was calling rt('admin.shops.id',['id'=>$shop->id]) (rt() call route() with absolute=false) changed with : boutiques/{shop} rt('admin.shops.id',['shop'=>$shop->id]) and it's ok.

changed with : boutiques/{id} rt('admin.shops.id',['id'=>$shop->id]) and it's ok too.

previous ( < 6.6 ): boutiques/{shop} rt('admin.shops.id',['id'=>$shop->id]) was "wrong" but working.

my route is boutiques/{shop} not boutiques/{id} my var name was wrong but it used to work in previous version. So it's not really a bug but a more strict change that can break wrong variables names in route.

GuidoHendriks commented 4 years ago

There was a previous change in Laravel 6.x that requires the variable names to match, otherwise they're added to the query string. Before my change it wouldn't enforce the required parameter and result in the wrong url being returned (/boutiques/?id=123 for example).

neoteknic commented 4 years ago

@GuidoHendriks ok so this fix will just break bad coded app (variable names different between route file/route call). If people face this issue they just have to fix their app, It's ok for me, I can close it ?

GuidoHendriks commented 4 years ago

@neoteknic It makes sure you actually have parameters for your routes. While an error is inconvenient, it's in my opinion better than it silently giving you the wrong URL. Which happened to me, causing a bug for users. Guess this could be closed as it does what it's supposed to do.

driesvints commented 4 years ago

We indeed introduced this as a bug fix. If you have incorrectly generated routes they'll now bubble up so that's actually a good thing.

datapoesy commented 4 years ago

Is Laravel reliable anymore? This bug still persists.

devcircus commented 4 years ago

There's no bug. Read through the post. The behavior is correct.

hewerthomn commented 4 years ago

exact this point:

my route is boutiques/{shop} not boutiques/{id} my var name was wrong but it used to work in previous version. So it's not really a bug but a more strict change that can break wrong variables names in route.

devcircus commented 4 years ago

Yes. The change was made to correct a long-time issue that allowed incorrect code to work. It's been fixed now, so the "bad code" no longer works.

datapoesy commented 4 years ago

Very much appreciate your response; unfortunately, I still get the same error despite giving the right name: Form : {{ Form::model($cdmestimate, ['route' => 'frontend.user.cdmestimates.store','cdmestimate' => $cdmestimate,'method' => 'POST'])}} @csrf @method('PUT')

Route : Route::put('/cdmestimates/{cdmestimate}', [CdmestimateController::class, 'store'])->name('cdmestimates.store');

The Controller : public function store(Cdmestimate $cdmestimate) {//}

Error : Missing required parameters for [Route: frontend.user.cdmestimates.store] [URI: cdmestimates/{cdmestimate}].

hewerthomn commented 4 years ago

the param route is wrong: ['route' => 'frontend.user.cdmestimates.store','cdmestimate' => $cdmestimate]

try this way: ['route' => ['frontend.user.cdmestimates.store', ['cdmestimate' => $cdmestimate]]