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

Stacks are middlewares too #4

Merged
merged 4 commits into from
Nov 23, 2016

Conversation

schnittstabil
Copy link
Contributor

@schnittstabil schnittstabil commented Nov 22, 2016

This is a consistent continuation of #2 and allows reusing Stack as ServerMiddlewareInterface in other Middleware containers:

// I've used the splat operator:
$stack = new Stack($middlewareOne, $middlewareTwo);

$middlewares = [
   // …
   new AwesomePsrMiddleware(),
   // …
   $stack,
   // …
];

// some foreign stack container:
$foreign = new MiddlewareContainer($middlewares, …);
$foreign->dispatch($request);

See #6 for deatils.

@shadowhand
Copy link
Contributor

I think I prefer the idea that a stack can also be a middleware. That allows for mixing of different dispatching systems (if they also follow the same methodology) and allows mixing groups of middleware that may be packaged together.

However, the order of middleware is sometimes important and this may cause surprises. I need to think about it some more.

Any thoughts @equip/contributors ?

*/
public function __construct(array $middleware, callable $default)
public function __construct(array $middleware, DelegateInterface $lastDelegate)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not correct. The default should not be a delegate, because this is perfectly acceptable for $default:

$default = [$responseFactory, 'createResponse'];

{
array_map([$this, 'append'], $middleware);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You lost this, which destroys the ability to ensure type safety of middleware added by the constructor.

$delegate = new Delegate($this->middleware, $default);

return $delegate->process($request);
return $this->process($request, new CallableToDelegateAdapter($default));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, this is not right. The default is not a delegate.

@schnittstabil schnittstabil force-pushed the stacks-are-middlewares-too branch 4 times, most recently from d9dce30 to 4a48fa0 Compare November 22, 2016 15:03
@schnittstabil
Copy link
Contributor Author

schnittstabil commented Nov 22, 2016

However, the order of middleware is sometimes important and this may cause surprises.

That is true, but not in this case. A container manages a group of middlewares, thus the order is only important within that container. For middleware stacks, where middlewares are called from last to first, it works like that:

$stack = new Stack(...[
    $middleware5,
    $middleware4,
    new Stack(...[
        $middleware3,
        $middleware2,
    ]),
    $middleware1,
    $middleware0,
]);

If one mixes containers, e.g. with pipes, where middlewares are called from first to last, we get:

$stack = new Pipe(...[
    $middleware0,
    $middleware1,
    new Stack(...[
        $middleware3,
        $middleware2,
    ]),
    $middleware4,
    $middleware5,
]);

It looks confusing, but it is the expected behavior. The order within the pipe and stack is correct according to their semantics.

@schnittstabil
Copy link
Contributor Author

@shadowhand: lgtm – ready for review.

@schnittstabil
Copy link
Contributor Author

But I just realized, that the Equip\Dispatch\Stack is actually a Pipe, not a Stack!

*
* @return ResponseInterface
*/
public function process(ServerRequestInterface $request, DelegateInterface $nextContanierDelegate)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo: nextContanierDelegate should be nextContainerDelegate

@@ -50,9 +50,9 @@ $default = function (RequestInterface $request) {
return new Response();
};

$stack = new Stack($middleware, $default);
$stack = new Stack(...$middleware);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not really sure about this change. What does splat buy us here? From a syntax standpoint, it is more to type out. From a functionality standpoint, all it does it splat and unsplat the arguments, which consumes ticks unnecessarily.

*/
public function process(ServerRequestInterface $request, DelegateInterface $nextContanierDelegate)
{
$delegate = new Delegate($this->middleware, new DelegateToCallableAdapter($nextContanierDelegate));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, we're getting off track here. I want to see two changes, made as separate PRs:

  • move $default to the constructor as the first argument
  • implement ServerMiddlewareInterface in Stack

Copy link
Contributor Author

@schnittstabil schnittstabil Nov 22, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see – but that doesn't make sense in my opinion, e.g:

$inner = new Stack($default1, ...$middleware);

$outer = new Stack($default2, ...[
    // …
    $inner,
    $nextMiddleware,
    // …
]);

$outer->dispatch($request);

It is not only because we have to create two $default functions/instances. It is also confusing, because it is not obvious, which $default will be called. $default1, $default2 or both?

Instead I suggest:

$inner = new Stack(...$middleware);

$outer = new Stack(...[
    // …
    $inner,
    $nextMiddleware,
    // …
]);

$outer->dispatch($request, $default);

Thus, I think it is more obvious that $inner delegates the request to $outer, which delegates the request to $nextMiddleware.

Of course, in last version the splat became useless…

@shadowhand
Copy link
Contributor

Equip\Dispatch\Stack is actually a Pipe, not a Stack!

A totally valid point. Let's address that after #3 and #4 are finished.

@shadowhand
Copy link
Contributor

Okay cool. I'm going to take this as-is and then make some tweaks. Will make PR for you to check out.

@shadowhand shadowhand merged commit 24134cd into equip:master Nov 23, 2016
shadowhand added a commit that referenced this pull request Nov 23, 2016
Stack is actually a middleware pipe.

DelegateToCallableAdapter is actually a proxy.

Refs #4
shadowhand added a commit that referenced this pull request Nov 23, 2016
Stack is actually a middleware pipe.

DelegateToCallableAdapter is actually a proxy.

Refs #4
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

2 participants