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

[8.x] Change loadRoutesFrom to accept group $attributes #34866

Merged
merged 1 commit into from Oct 19, 2020

Conversation

imanghafoori1
Copy link
Contributor

@imanghafoori1 imanghafoori1 commented Oct 17, 2020

When we define routes it is often needed to include a couple of middleware groups (especially web or api) on the loaded routes in the files.

This PR makes it possible to refactor out the Route::group call in the manually loaded route files and make them flatter while maintaining the proper route-caching.
Plus, it brings the $route object back and makes it consistent with default route files.

  • Let me note that calling Route::group($attr, $path); directly in the boot method (instead of loadRoutesFrom) requires knowledge of how the route caching is done in that particular version of laravel or they will not get cached.

Performance hit:

On production, it should have zero-hit on performance, since the routes are cached.

Before

// boot method
$this->loadRoutesFrom($somePath);
// my_routes.php 
Route::group(['middleware' => 'web'], function () {
    Route::get(...);
    Route::post(...);
    ...
});

After:

// boot method
$this->loadRoutesFrom($somePath, ['middleware' => 'web']);
// my_routes.php 
// Route::group(['middleware' => 'web'], function () {
Route::get(...);
Route::post(...);
...
//});

Backward-compatibility:

This is mostly backward compatible except for method signature issues, which arise when overriding methods in subclasses, which is very rare.

  • The imported Route facade is already in the illuminate\support package.

@taylorotwell taylorotwell merged commit e719250 into laravel:8.x Oct 19, 2020
7 checks passed
@imanghafoori1 imanghafoori1 deleted the load_routes branch Oct 19, 2020
@rs-sliske
Copy link

@rs-sliske rs-sliske commented Oct 20, 2020

this breaks with mll-lab/laravel-graphql-playground

i expect the response is going to be to fix that library but just letting people know :(

@imanghafoori1
Copy link
Contributor Author

@imanghafoori1 imanghafoori1 commented Oct 20, 2020

Do they have an override of the changed method?
what the error is?
@rs-sliske

@rs-sliske
Copy link

@rs-sliske rs-sliske commented Oct 20, 2020

i havent actually looked at their code yet but i think they are overriding the method, i tried to run composer update in a project im working on and got an error of "[2020-10-20 16:49:45] local.ERROR: Declaration of MLL\GraphQLPlayground\GraphQLPlaygroundServiceProvider::loadRoutesFrom($path): void should be compatible with Illuminate\Support\ServiceProvider::loadRoutesFrom($path, array $attributes = Array) {"exception":"[object] (ErrorException(code: 0): Declaration of MLL\GraphQLPlayground\GraphQLPlaygroundServiceProvider::loadRoutesFrom($path): void should be compatible with Illuminate\Support\ServiceProvider::loadRoutesFrom($path, array $attributes = Array) at /app/vendor/mll-lab/laravel-graphql-playground/src/GraphQLPlaygroundServiceProvider.php:49)"

@voydz
Copy link

@voydz voydz commented Oct 20, 2020

@rs-sliske is right, I already submitted a PR for it here mll-lab/laravel-graphql-playground#46

But this changes breaks more than someone might actually thought of. For example nuwave/lighthouse also suffers from this change. Making a PR for it right now.

@imanghafoori1
Copy link
Contributor Author

@imanghafoori1 imanghafoori1 commented Oct 20, 2020

Yeah, this error is due to override, but the fix for it is also very quick and easy I think.

@voydz
Copy link

@voydz voydz commented Oct 20, 2020

Of course its quick and easy, but it also sets back a lot of developers in their time after upgrading to this version. I am not aware that I had any problems after upgrading Laravel in any minor revision in the past.
Personally I think this change was a bit to much on the breaking-change-y side of things.

@imanghafoori1
Copy link
Contributor Author

@imanghafoori1 imanghafoori1 commented Oct 20, 2020

I think this change relates to package maintainers and not typical web app developers, and because something like travis normally emails them and warns package maintainers of failing builds, it does not take a lot of time for maintainers to do the quick fix, and the web app developers must not see any breakage after they upgrade. (since they also upgrade the installed packages.)

@imanghafoori1
Copy link
Contributor Author

@imanghafoori1 imanghafoori1 commented Oct 20, 2020

I think that understanding the reason that many package maintainers have overridden this method can be insightful.

@rodrigopedra
Copy link
Contributor

@rodrigopedra rodrigopedra commented Oct 21, 2020

@driesvints
Copy link
Member

@driesvints driesvints commented Oct 21, 2020

We've reverted this PR.

@imanghafoori1
Copy link
Contributor Author

@imanghafoori1 imanghafoori1 commented Oct 21, 2020

@driesvints
Will this be included in laravel 9.x?

@driesvints
Copy link
Member

@driesvints driesvints commented Oct 22, 2020

I don't recommend that as this seems to break too much (including Lumen).

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.

None yet

6 participants