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 pushMiddlewareToGroup not working on Provider in My Package #42004

Merged
merged 1 commit into from
Apr 20, 2022

Conversation

ilyutkin
Copy link
Contributor

  • Laravel Version: 9.8.1
  • PHP Version: 8.0.5
  • Database Driver & Version: mariadb

Description:

In continuation of bug #36604. I found the reason for this bug.
We want to dynamically connect Middleware in our package in the Service Provider:

public function boot(Router $router)
{
    // Push middleware to web group
    $router->pushMiddlewareToGroup('web', MyMiddleware::class);
}

But the Middleware is not executed if the package name starts with the letters a-k.

Reason:

In the /Illuminate/Foundation/Http/Kernel.php file, the syncMiddlewareToRouter method overwrites the Middleware list in the Router

protected function syncMiddlewareToRouter()
{
    $this->router->middlewarePriority = $this->middlewarePriority;

    foreach ($this->middlewareGroups as $key => $middleware) {
        $this->router->middlewareGroup($key, $middleware);
    }

    foreach ($this->routeMiddleware as $key => $middleware) {
        $this->router->aliasMiddleware($key, $middleware);
    }
}

In the /Illuminate/Routing/Router.php file the middlewareGroup method

public function middlewareGroup($name, array $middleware)
{
    $this->middlewareGroups[$name] = $middleware;

    return $this;
}

The syncMiddlewareToRouter method is called in prependToMiddlewarePriority which calls /laravel/sanctum/src/SanctumServiceProvider.php after my ServiceProvider

Service providers are named in alphabetical order. If the package name starts with letters a-k Middleware Group is overwritten. If the package name begins with the letters m-z, everything works.

Solution:

If rewriting the middleware group is not required, then you can change the middlewareGroup method in /Illuminate/Routing/Router.php

public function middlewareGroup($name, array $middlewares)
{
    foreach ($middlewares as $middleware)
        $this->pushMiddlewareToGroup($name, $middleware);

    return $this;
}

Please add to the framework if I'm right.

@taylorotwell
Copy link
Member

Please provide short, specific list of steps to recreate in a fresh Laravel application.

@taylorotwell taylorotwell marked this pull request as draft April 18, 2022 13:51
@taylorotwell
Copy link
Member

taylorotwell commented Apr 18, 2022

Please mark as ready for review when you have done so. ^

@ilyutkin
Copy link
Contributor Author

Repositiry https://github.com/ilyutkin/laravel branch 9.x
Last Commit ea9eb0286d9796b9b62751be4cde7a69f142793c

Added two packages ilyutkin/test and xman/test. Each service provider adds middleware.
I am displaying a list of middleware groups in each service provider and also in a controller

laravel
.

Again, the syncMiddlewareToRouter method is called in prependToMiddlewarePriority which calls /laravel/sanctum/src/SanctumServiceProvider.php after my IlyutkinTestProvider. And overwrites middleware groups.

@ilyutkin ilyutkin marked this pull request as ready for review April 18, 2022 16:27
@taylorotwell
Copy link
Member

Please provide short, specific list of steps to recreate in a fresh Laravel application.

Please do not provide a repository for me to clone down. Just a list of steps I can take in a new Laravel application.

@ilyutkin
Copy link
Contributor Author

Sorry, I got it.

  • Add a package starting with the letter a-k. For example packages/ironman/test.
  • The package service provider must load the middleware. $router->pushMiddlewareToGroup method.
  • Loaded middleware will not be in the controller.

If the package starts with the letter m-z everything works

@taylorotwell taylorotwell merged commit 05365f8 into laravel:9.x Apr 20, 2022
@GrahamCampbell GrahamCampbell changed the title Fix pushMiddlewareToGroup not working on Provider in My Package [9.x] Fix pushMiddlewareToGroup not working on Provider in My Package Apr 20, 2022
@taylorotwell
Copy link
Member

Reverting this since it breaks applications.

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.

2 participants