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

PSR-15 Middleware Support #2555

Merged
merged 75 commits into from Feb 22, 2019

Conversation

Projects
None yet
7 participants
@l0gicgate
Copy link
Contributor

commented Dec 20, 2018

This PR is for upgrading current middleware support from PSR-7 to PSR-15. Backward compatibility for PSR-7 middleware is still present. The order of middleware is being executed is still the same as Slim 3 (LIFO or Last In First out)

All the existing middleware helper methods remain unchanged:

  • App::add($middleware)
  • Route::add($middleware)
  • RouteGroup::add($middleware)

New typed methods have been added:

  • App::addMiddleware(MiddlewareInterface $middleware)
  • Route::addMiddleware(MiddlewareInterface $middleware)
  • RouteGroup::addMiddleware(MiddlewareInterface $middleware)

The compatible types of middleware are as follows:

  • Object implementing MiddlewareInterface
  • Class name string of a class that implements MiddlewareInterface, which will be resolved at the time of execution
  • A function name or Closure implementing this signature function (ServerRequestInterface $request, RequestHandlerInterface $handler) {...}

Deprecated Features

  • Double Pass Middleware with this signature function ($request, $response, $next) {...}

This PR effectively closes:

Status

  • Code Review & Discussion:
@coveralls

This comment has been minimized.

Copy link

commented Dec 20, 2018

Coverage Status

Coverage increased (+1.1%) to 98.542% when pulling 6fe1b98 on l0gicgate:Middleware into 0deb322 on slimphp:4.x.

@lordrhodos

This comment has been minimized.

Copy link

commented Jan 10, 2019

thank you @l0gicgate for the work. I have just started playing around a little bit with the 4.x branch and hit the middleware issue when setting up a first base application. Your PR fixed that 👍 🎉

After reading through the changes I have some remarks, event though I am not really into the Slim internals:

I would suggest to remove the addLegacy functions. From what I can tell Slim aims in its core for a clean and simple code base. Easy to read and understand as a developer with a focus on simplicity and performance. I love those things about it from what I learned so far. IMO adding the addLegacy functions just adds duplicate funcionality which is not needed. I do not think convenience support for BC updates justifies these methods. If people need to upgrade to 4.x they should do this manually by wrapping the legacy middleware. How to do this should be well documented and for BC convenience you could provide an update script/task that automates the wrapping.

Just my 0.0002 cents 😜

@l0gicgate

This comment has been minimized.

Copy link
Contributor Author

commented Jan 10, 2019

@lordrhodos thank you for reviewing this PR. These methods were put in place as per the pre-PR discussion with @akrabat. I think they need to be left in for the end user to be able to migrate easier, while this is still a BC change, I believe they still need to be there and deprecated on the next major release.

@akrabat

This comment has been minimized.

Copy link
Member

commented Jan 17, 2019

I think that add() needs to support DI lookup.

i.e. $app->add(Slim\Middleware\ContentLengthMiddleware::class); should work as it could be a key to the DI container which injects dependencies that I only want to resolve if this this middleware actually runs.

@l0gicgate

This comment has been minimized.

Copy link
Contributor Author

commented Jan 17, 2019

I think that add() needs to support DI lookup.

Okay so you want the DI lookup to be done right then and there inside App::add()? Because we can't do a deferred callable for this since it isn't an invokable.

@akrabat

This comment has been minimized.

Copy link
Member

commented Jan 17, 2019

I think that add() needs to support DI lookup.
Okay so you want the DI lookup to be done right then and there inside App::add()? Because we can't do a deferred callable for this since it isn't an invokable.

We might need a better deferred callable. We don't want to go backwards in terms of ability.

@l0gicgate

This comment has been minimized.

Copy link
Contributor Author

commented Jan 17, 2019

@akrabat I added support for deferred PSR-15 middleware instantiation as per your request.

@l0gicgate

This comment has been minimized.

Copy link
Contributor Author

commented Jan 23, 2019

@lordrhodos I ended up adding detection logic for PSR-7 middleware and removing the addLegacy() methods. You can now basically throw any type of middleware at the add() method and it will handle it.

@lordrhodos

This comment has been minimized.

Copy link

commented Jan 23, 2019

I ended up adding detection logic for PSR-7 middleware and removing the addLegacy() methods.

@l0gicgate this sounds great in terms of providing backwards compatibility!!! I like the decision. Maybe you should discuss if PSR-7 support should be deprecated for a next major version 5 though, to slim down the code base.

@bnf bnf referenced this pull request Jan 24, 2019

Closed

Add support for PSR-15 middlewares #2379

0 of 2 tasks complete
Show resolved Hide resolved Slim/App.php Outdated
Show resolved Hide resolved Slim/App.php Outdated
Show resolved Hide resolved Slim/App.php Outdated

@l0gicgate l0gicgate force-pushed the l0gicgate:Middleware branch from 614486e to c4bdc0f Feb 11, 2019

@akrabat akrabat self-requested a review Feb 12, 2019

@akrabat

This comment has been minimized.

Copy link
Member

commented Feb 12, 2019

@bnf pointed out something important:

Going with a pure PSR-15 solution and adding a wrapper for double pass middlewares (as in #2555) results in (hard to detect) breaking behaviour

We have two choices here:

  1. pass a shared response object to all the PSR-7 middleware
  2. make it explicit to the upgrader that something has changed. e.g. $app->addPsr7Middleware()

Anyone have an opinion on which one?

@l0gicgate

This comment has been minimized.

Copy link
Contributor Author

commented Feb 12, 2019

  1. pass a shared response object to all the PSR-7 middleware

I think we're going to have to go with option #2. It is less user-friendly for migration but it increases the chances that they'll go read the docs and understand that PSR-7 is only partially supported.

@bnf

This comment has been minimized.

Copy link
Contributor

commented Feb 12, 2019

We have two choices here:

Or maybe we have a third one:

  • Drop support for legacy (in this thread sometimes also called PSR-7) middlewares at all

There is no technical need for Slim to bundle the incompatible (as seen from the BC view) implementation, as wrapping a PSR-15 middleware by injecting a new ResponseInterface for every middleware can be done outside slim for both, normal and lazy middlewares. (Bundling this support is technically really only necessary if we would try to stay compatible, as #2379 did.)

Advantages:

  • Slim stays clean (and well, slim)
  • It becmomes very clear that it is not Slim's fault when the legacy and non-recommended pattern (altering the ResponseInterface on the way down the middleware stack) now breaks (as the developer is aware of what the wrapping does, when using a wrapper class, as opposed to some magic wrapping that happens behind Slim's doors)
  • Upgrade documentation could provide a simple example of a PSR-15 middleware adapter which creates a new response. This will probably actually result in quicker adoption of the PSR-15 interfaces. Why? I think it's really easy to understand how to rewrite an existing middleware when looking at a simple wrapper/adapter class. (Well, there is one exception: when the response was altered you get a problem, but that's the intention: The developers need to detect this pattern upfront, and then rewrite the code, before getting into production (where is silently fails and becomes hard to debug).

I think we're going to have to go with option 2

Don't think s;, option 1 is valid: As this pull request is using a queue a stateful dispatcher is needed anyway ($this->stages), so adding another state (shared Response) would be possible.

PS: The name "PSR-7 middleware" is quite misleading (as there are no middlewares in PSR-7 and as PSR-15 middlewares do use PSR-7 interfaces, this sounds as if the new middlewares are referenced. Yes, naming is hard. Double vs Single pass is quite unknown. But maybe "legacy" or "classical" or "slimv3 middlewares" is better than?

@akrabat

This comment has been minimized.

Copy link
Member

commented Feb 12, 2019

  • Drop support for legacy (in this thread sometimes also called PSR-7) middlewares at all

I'm open to this if we can document what to do easily

@l0gicgate

This comment has been minimized.

Copy link
Contributor Author

commented Feb 12, 2019

@bnf ultimately I would like a cleaner codebase and I’m personally fine with dropping Double Pass middleware as well if @akrabat sees this as an option

I see Slim 4 as a clean slate, at some point we’re going to drop Double Pass middleware support so why not now?

The migration process from 3 to 4 already has a few BC changes, let’s break everything now instead of later and not have to carry the technical debt.

We’re going to have unhappy users no matter what we do. There’s nothing wrong with sticking with Slim 3.x if the advantages from Slim 4 don’t seem to outweigh the cons of having to refactor your code. I don’t think adoption will suffer all that much.

Also, in this PR I migrated all the core middleware from double pass to single pass and it wasn’t hard at all, it only took about an hour.

@akrabat

This comment has been minimized.

Copy link
Member

commented Feb 12, 2019

If we drop Slim3 middleware format do we support pseudo-PSR-15closures ?

i.e. can a user do this?

$app->add(function($request, $handler) {
    return $handler->handle($request):
});
Show resolved Hide resolved Slim/MiddlewareRunner.php Outdated
Show resolved Hide resolved Slim/MiddlewareRunner.php Outdated
Show resolved Hide resolved Slim/Route.php Outdated
Always uses MiddlewareRunner::add() to add middleware
This means that we use PHP 7's type checking and don't need to do it in
the loop on run.
Show resolved Hide resolved tests/RouteTest.php Outdated
@l0gicgate
Copy link
Contributor Author

left a comment

Looks good. Approved.

@l0gicgate
Copy link
Contributor Author

left a comment

Looks good. Approved.

@l0gicgate
Copy link
Contributor Author

left a comment

Looks good. Approved.

@l0gicgate

This comment has been minimized.

Copy link
Contributor

commented on 4bf9785 Feb 22, 2019

Looks good. Approved.

@l0gicgate

This comment has been minimized.

Copy link
Contributor

commented on 9aa8c6b Feb 22, 2019

I’m ashamed of myself for overlooking these mistakes. Approved.

@akrabat akrabat merged commit 9aa8c6b into slimphp:4.x Feb 22, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

akrabat added a commit that referenced this pull request Feb 22, 2019

@l0gicgate l0gicgate deleted the l0gicgate:Middleware branch Feb 22, 2019

bnf added a commit to bnf/Slim that referenced this pull request Feb 23, 2019

Refactor middleware routing into a stack based dispatch cycle
Improve certain things:
 * Do not hide container dependencies in the CallableResolverInterface
   getContainer method (this method is dropped now)
 * Use a middleware stack instead of a queue, as slim used to use before slimphp#2555
 * Use designated request handlers as kernel (as we know them from slim v3)
   instead of middlewares that ignore the second (RequestHandler) parameter
 * Do not recreate middlewares in route/route-group middleware combination,
   rather chain middleware runners, thanks to the (now) proper use of
   RequestHandlerInterface
 * Drop some helper middlewares, which is good as they were applicable only
   as internal middleware anyway. This logic is now handled in the middleware
   dispatcher for optimization reasons (because that handler
   create a RequestHandler wrapper for every middleware, it's most performant
   to include lazy/closure dispatch logic in the RequestHandler wrappers).
 * DispatchMiddleware (and it's corresponding test) is moved into the
   Router which now acts as a PSR-15 RequestHandler
 * Replace middleware related tests that test internal state (e.g. whether
   something is at the bottom of a list.) That is an implementation detail and
   should rather be verified by checking for proper operation
 * Rename MiddlewareRunner to MiddlewareDispatcher, as it actually dispatches
   middlewares now

Fixes: slimphp#2590

bnf added a commit to bnf/Slim that referenced this pull request Feb 23, 2019

Refactor middleware routing into a stack based dispatch cycle
Improve certain things:
 * Do not hide container dependencies in the CallableResolverInterface
   getContainer method (this method is dropped now)
 * Use a middleware stack instead of a queue, as slim used to use before slimphp#2555
 * Use designated request handlers as kernel (as we know them from slim v3)
   instead of middlewares that ignore the second (RequestHandler) parameter
 * Do not recreate middlewares in route/route-group middleware combination,
   rather chain middleware dispatchers, thanks to the (now) proper use of
   RequestHandlerInterface of the MiddlewareDispatcher which allows itself
   to be chained into another middleware stack
 * Drop some helper middlewares, which is good as they were applicable only
   as internal middleware anyway. This logic is now handled in the middleware
   dispatcher for optimization reasons (because that handler
   create a RequestHandler wrapper for every middleware, it's most performant
   to include lazy/closure dispatch logic in the RequestHandler wrappers).
 * DispatchMiddleware (and it's corresponding test) is moved into the
   Router which now acts as a PSR-15 RequestHandler
 * Replace middleware related tests that test internal state (e.g. whether
   something is at the bottom of a list.) That is an implementation detail and
   should rather be verified by checking for proper operation
 * Rename MiddlewareRunner to MiddlewareDispatcher, as it actually dispatches
   middlewares now

Fixes: slimphp#2590

bnf added a commit to bnf/Slim that referenced this pull request Feb 23, 2019

Refactor middleware routing into a stack based dispatch cycle
Improve certain things:
 * Do not hide container dependencies in the CallableResolverInterface
   getContainer method (this method is dropped now)
 * Use a middleware stack instead of a queue, as slim used to use before slimphp#2555
 * Use designated request handlers as kernel (as we know them from slim v3)
   instead of middlewares that ignore the second (RequestHandler) parameter
 * Do not recreate middlewares in route/route-group middleware combination,
   rather chain middleware dispatchers, thanks to the (now) proper use of
   RequestHandlerInterface of the MiddlewareDispatcher which allows itself
   to be chained into another middleware stack
 * Drop some helper middlewares, which is good as they were applicable only
   as internal middleware anyway. This logic is now handled in the middleware
   dispatcher for optimization reasons (because that handler
   create a RequestHandler wrapper for every middleware, it's most performant
   to include lazy/closure dispatch logic in the RequestHandler wrappers).
 * DispatchMiddleware (and it's corresponding test) is moved into the
   Router which now acts as a PSR-15 RequestHandler
 * Replace middleware related tests that test internal state (e.g. whether
   something is at the bottom of a list.) That is an implementation detail and
   should rather be verified by checking for proper operation
 * Rename MiddlewareRunner to MiddlewareDispatcher, as it actually dispatches
   middlewares now

Fixes: slimphp#2590

bnf added a commit to bnf/Slim that referenced this pull request Feb 23, 2019

Refactor middleware routing into a stack based dispatch cycle
Improve certain things:
 * Do not hide container dependencies in the CallableResolverInterface
   getContainer method (this method is dropped now)
 * Use a middleware stack instead of a queue, as slim used to use before slimphp#2555
 * Use designated request handlers as kernel (as we know them from slim v3)
   instead of middlewares that ignore the second (RequestHandler) parameter
 * Do not recreate middlewares in route/route-group middleware combination,
   rather chain middleware dispatchers, thanks to the (now) proper use of
   RequestHandlerInterface of the MiddlewareDispatcher which allows itself
   to be chained into another middleware stack
 * Drop some helper middlewares, which is good as they were applicable only
   as internal middleware anyway. This logic is now handled in the middleware
   dispatcher for optimization reasons (because that handler
   create a RequestHandler wrapper for every middleware, it's most performant
   to include lazy/closure dispatch logic in the RequestHandler wrappers).
 * DispatchMiddleware (and it's corresponding test) is moved into the
   Router which now acts as a PSR-15 RequestHandler
 * Replace middleware related tests that test internal state (e.g. whether
   something is at the bottom of a list.) That is an implementation detail and
   should rather be verified by checking for proper operation
 * Rename MiddlewareRunner to MiddlewareDispatcher, as it actually dispatches
   middlewares now

Fixes: slimphp#2590

bnf added a commit to bnf/Slim that referenced this pull request Feb 25, 2019

Refactor middleware routing into a stack based dispatch cycle
Improve certain things:
 * Do not hide container dependencies in the CallableResolverInterface
   getContainer method (this method is dropped now)
 * Use a middleware stack instead of a queue, as slim used to use before slimphp#2555
 * Use designated request handlers as kernel (as we know them from slim v3)
   instead of middlewares that ignore the second (RequestHandler) parameter
 * Do not recreate middlewares in route/route-group middleware combination,
   rather chain middleware dispatchers, thanks to the (now) proper use of
   RequestHandlerInterface of the MiddlewareDispatcher which allows itself
   to be chained into another middleware stack
 * Drop some helper middlewares, which is good as they were applicable only
   as internal middleware anyway. This logic is now handled in the middleware
   dispatcher for optimization reasons (because that handler
   create a RequestHandler wrapper for every middleware, it's most performant
   to include lazy/closure dispatch logic in the RequestHandler wrappers).
 * DispatchMiddleware (and it's corresponding test) is moved into the
   Router which now acts as a PSR-15 RequestHandler
 * Replace middleware related tests that test internal state (e.g. whether
   something is at the bottom of a list.) That is an implementation detail and
   should rather be verified by checking for proper operation
 * Rename MiddlewareRunner to MiddlewareDispatcher, as it actually dispatches
   middlewares now

Fixes: slimphp#2590

l0gicgate added a commit to l0gicgate/Slim that referenced this pull request Feb 28, 2019

bnf added a commit to bnf/Slim that referenced this pull request Feb 28, 2019

Refactor middleware routing into a stack based dispatch cycle
Improve certain things:
 * Do not hide container dependencies in the CallableResolverInterface
   getContainer method (this method is dropped now)
 * Use a middleware stack instead of a queue, as slim used to use before slimphp#2555
 * Use designated request handlers as kernel (as we know them from slim v3)
   instead of middlewares that ignore the second (RequestHandler) parameter
 * Do not recreate middlewares in route/route-group middleware combination,
   rather chain middleware dispatchers, thanks to the (now) proper use of
   RequestHandlerInterface of the MiddlewareDispatcher which allows itself
   to be chained into another middleware stack
 * Drop some helper middlewares, which is good as they were applicable only
   as internal middleware anyway. This logic is now handled in the middleware
   dispatcher for optimization reasons (because that handler
   create a RequestHandler wrapper for every middleware, it's most performant
   to include lazy/closure dispatch logic in the RequestHandler wrappers).
 * DispatchMiddleware (and it's corresponding test) is moved into the
   Router which now acts as a PSR-15 RequestHandler
 * Replace middleware related tests that test internal state (e.g. whether
   something is at the bottom of a list.) That is an implementation detail and
   should rather be verified by checking for proper operation
 * Rename MiddlewareRunner to MiddlewareDispatcher, as it actually dispatches
   middlewares now

Fixes: slimphp#2590

bnf added a commit to bnf/Slim that referenced this pull request Mar 1, 2019

Refactor middleware routing into a stack based dispatch cycle
Improve certain things:
 * Do not hide container dependencies in the CallableResolverInterface
   getContainer method (this method is dropped now)
 * Use a middleware stack instead of a queue, as slim used to use before slimphp#2555
 * Use designated request handlers as kernel (as we know them from slim v3)
   instead of middleware that ignore the second (RequestHandler) parameter
 * Do not recreate middleware in route/route-group middleware combination,
   rather chain middleware dispatchers, thanks to the (now) proper use of
   RequestHandlerInterface of the MiddlewareDispatcher which allows itself
   to be chained into another middleware stack
 * Drop some helper middleware, which is good as they were applicable only
   as internal middleware anyway. This logic is now handled in the middleware
   dispatcher for optimization reasons (because that handler
   create a RequestHandler wrapper for every middleware, it's most performant
   to include lazy/closure dispatch logic in the RequestHandler wrappers).
 * DispatchMiddleware (and it's corresponding test) is moved into the
   Router which now acts as a PSR-15 RequestHandler
 * Replace middleware related tests that test internal state (e.g. whether
   something is at the bottom of a list.) That is an implementation detail and
   should rather be verified by checking for proper operation
 * Rename MiddlewareRunner to MiddlewareDispatcher, as it actually dispatches
   middleware now

Fixes: slimphp#2590

@l0gicgate l0gicgate referenced this pull request Apr 25, 2019

Merged

Slim 4 Alpha Release #2665

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.