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

Add support for PSR-15 middlewares #2379

Open
wants to merge 13 commits into
base: 4.x
from

Conversation

Projects
None yet
6 participants
@bnf
Contributor

bnf commented Jan 10, 2018

PSR-15 middlewares can be registered using the existing API:
App::add() and Routable::add()
Those methods are extended to accept objects that implement
the MiddlewareInterface (or class names that refer to classes
which implement the MiddlewareInterface).

We do not add new entry points for middleware registration, as the type
of the middleware can be detecting through the MiddlewareInterface
which all PSR-15 middlewares implement.

Both, classes that do not implement MiddlewareInterface, and closures will
be handled as single pass middleware – as before.
That means there's no support for closures with a PSR-15 single-pass
signature.

The detection of the middleware type is implemented
in the callable resolver. While that might look like
something which is out of scope for the CallableResolver, reasons are:

  1. The internal slim middleware stack stays at is – it handles callables only.
    (and in cannot be changed anyway as long double-pass middlewares
    need to be supported. Double-pass middlewares expect a Response object
    that needs to be passed along the middleware stack. Therefore we'd need to
    wrap PSR-15 middleware's in any case – which is what this change provides)
  2. the CallableResolver resolves (in terms for transforms) a callable
    from PSR-15 style middleware.

TODO: Add more tests for:

  • Adapter\SinglePassMiddleware
  • Routable with PSR-15 Middlewares

Resolves #2050

@coveralls

This comment has been minimized.

coveralls commented Jan 10, 2018

Coverage Status

Coverage increased (+0.1%) to 95.044% when pulling f5ec341 on bnf:4x-psr-15-v2 into 1306570 on slimphp:4.x.

@coveralls

This comment has been minimized.

coveralls commented Jan 10, 2018

Coverage Status

Coverage increased (+0.1%) to 95.044% when pulling 8958174 on bnf:4x-psr-15-v2 into 1306570 on slimphp:4.x.

@geggleto geggleto requested review from akrabat and geggleto Jan 10, 2018

@geggleto geggleto added this to the 4.0 milestone Jan 10, 2018

@geggleto

This comment has been minimized.

Contributor

geggleto commented Jan 10, 2018

Thanks :) I will take a good look at this today or tomorrow, I know akrabat is at a conference this week so he might be able to get around to it next.

@coveralls

This comment has been minimized.

coveralls commented Jan 10, 2018

Coverage Status

Coverage increased (+0.1%) to 95.044% when pulling 405bd08 on bnf:4x-psr-15-v2 into 1306570 on slimphp:4.x.

@coveralls

This comment has been minimized.

coveralls commented Jan 11, 2018

Coverage Status

Coverage increased (+0.1%) to 95.044% when pulling 0c45802 on bnf:4x-psr-15-v2 into 1306570 on slimphp:4.x.

bnf added some commits Jan 10, 2018

Add support for PSR-15 style middlewares
PSR-15 middlewares can be registered using the existing API:
`App::add()` and `Routable::add()`
Those methods are extended to accept objects that implement
the MiddlewareInterface (or class names that refer to classes
which implement the MiddlewareInterface).

We do not add new entry points for middleware registration, as the type
of the middleware can be detecting through the MiddlewareInterface
which all PSR-15 middlewares implement.

Both, classes that do not implement MiddlewareInterface, and closures will
be handled as single pass middleware – as before.
That means there's no support for closures with a PSR-15 single-pass
signature.

The detection of the middleware type is implemented
in the callable resolver. While that might look like
something which is out of scope for the CallableResolver, reasons are:
a) The internal slim middleware stack stays at is – it handles callables only.
   (and in cannot be changed anyway as long double-pass middlewares
   need to be supported. Double-pass middlewares expect a Response object
   that needs to be passed along the middleware stack. Therefore we'd need to
   wrap PSR-15 middleware's in any case – which is what this change provides)
b) the CallableResolver resolves (in terms for transforms) a callable
   from PSR-15 style middleware.

TODO: Add more tests for:
 - Adapter\SinglePassMiddleware
 - Routable with PSR-15 Middlewares

Closes: #2050
Closes: #2051
travis: Force hhvm PHP 7 mode, but allow failures
hhvm in PHP 7 mode is currently imcompatible with composer.
@coveralls

This comment has been minimized.

coveralls commented Jan 12, 2018

Coverage Status

Coverage increased (+0.09%) to 95.029% when pulling de89805 on bnf:4x-psr-15-v2 into bd6ef95 on slimphp:4.x.

public function handle(ServerRequestInterface $request): ResponseInterface
{
return call_user_func($this->next, $request, $this->response);

This comment has been minimized.

@weierophinney

weierophinney Jan 22, 2018

Since $this->next is `callable, and this PR requires PHP 7 and up, that means you can simplify the above to:

return ($this->next)($request, $this->response);

My understanding is that call_user_func is slower than PHP's native dereferencing in PHP 7, so the above will also be more performant.

This comment has been minimized.

@bnf

bnf Jan 23, 2018

Contributor

I used call_user_func since that was used throughout the Slim codebase. I think those occurrences should be changed in one go. Will push a patch for that.

@@ -8,8 +8,10 @@
*/
namespace Slim;
use Interop\Http\Server\MiddlewareInterface;

This comment has been minimized.

@weierophinney

weierophinney Jan 22, 2018

Just a heads-up: PSR-15 passed today, though we're waiting to announce it until the packages are on Packagist — though that should be within the next few hours after I write this. As such, you'll be able to require psr/http-server-middleware instead of http-interop/http-server-middleware, and the above will become Psr\Http\Server\MiddlewareInterface.

This comment has been minimized.

@bnf

bnf Jan 23, 2018

Contributor

Yes, I pushed a patch for the switch to psr/http-server-middleware.

* and is used only inside of the Slim application; it is not visible
* to—and should not be used by—end users.
*/
final class SinglePassMiddleware implements RequestHandlerInterface

This comment has been minimized.

@weierophinney

weierophinney Jan 22, 2018

Instead of SinglePassMiddleware, may I suggest StandardsBasedMiddleware or PsrMiddleware here?

I've noted in communicating with ZF folks that many are not clear what is meant by "single-pass" and "double-pass", despite us having documented it. Having a name that indicates the type of middleware in terms of where you might find a definition helps.

Also, I'd add an @see annotation with a link to the accepted standard (we'll have that on the fig-standards repo shortly).

This comment has been minimized.

@bnf

bnf Jan 23, 2018

Contributor

Well, I personally prefer a name that describes the functionality. With PsrMiddleware it's not that clear whether the middleware is implementing PSR 15 or (as in our case) adapts a PSR-15 middleware.
Anyway, I've no strong objections against PsrMiddleware so i'll push a patch that renames that one (consistent naming across frameworks is probably not that bad).

@@ -87,13 +87,13 @@ public function getCallableResolver()
/**
* Prepend middleware to the middleware collection
*
* @param callable|string $callable The callback routine
* @param MiddlewareInterface|callable|string $middleware The middleware routine

This comment has been minimized.

@weierophinney

weierophinney Jan 22, 2018

Have you imported this interface within this class file?

This comment has been minimized.

@bnf

bnf Jan 23, 2018

Contributor

I missed that one, will fix.

bnf added some commits Jan 22, 2018

Rename SinglePassMiddleware to PsrMiddleware
This way it's easier to understand that this middleware
is an adapter for the PSR 15 middlewares as it does not
presume that the term single pass middleware is known.

@bnf bnf changed the title from [RFC] Add support for PSR-15 style middlewares to Add support for PSR-15 middlewares Jan 23, 2018

.travis.yml Outdated
env: COMPOSER_ARGS='--prefer-lowest'
- php: nightly
allow_failures:
- php: hhvm

This comment has been minimized.

@akrabat

akrabat Feb 10, 2018

Member

Let's just take hhvm out - we're not supporting it in 4.x

bnf and others added some commits Feb 16, 2018

Remove testing of hhvm
We don't have the capacity to support HHVM any longer.

Cherry-picked from d13ef9a
Support using App as PSR-15 RequestHandler
Implementing the RequestHandlerInterface means the
Slim App is embeddedable to another PSR-15 middleware
stack.

@oscarotero oscarotero referenced this pull request Sep 10, 2018

Closed

Adding to Slim Framework #9

@akrabat

This comment has been minimized.

Member

akrabat commented Sep 30, 2018

I've been thinking about this a bit. While I appreciate that it's easier to keep the internals the same as Slim 3, I'd like to explore if we can make PSR-15 the default internally and "wrap" the current signatures. Going forward, this makes more sense to me as we probably won't support the current signatures in Slim 5.

Thoughts?

@l0gicgate

This comment has been minimized.

l0gicgate commented Oct 23, 2018

@bnf are you moving forward with this PR? I'm curious as I will tackle it after my next couple of PRs if you aren't.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment