Map path prefixes to middleware stacks #7

franzliedke · 2018-04-08T18:01:11Z

As proposed in #6.

Please have a look at the tests - they describe the intended feature set quite well.

Fixes #6.

shadowhand · 2018-04-08T18:18:10Z

composer.json

@@ -20,7 +20,9 @@
    },
    "require-dev": {
        "eloquent/phony-phpunit": "^3.0",
-        "phpunit/phpunit": "^6.0"
+        "league/container": "^2.0",
+        "phpunit/phpunit": "^6.0",


No need to require phpunit, phony-phpunit always requires it.

It's been there before. Do you want me to remove it?

Doh, of course it was. No need to change anything, composer.json changes should just be reverted.

shadowhand · 2018-04-13T15:33:56Z

src/Broker.php

+        return $this;
+    }
+
+    public function resolve($middleware): Middleware


I don't really like this being a public method. What are we gaining by having an interface for resolution, as opposed to a shared trait?

Not sure. I was thinking Interface Segregation Principle and "freeing" the RequestHandler (and now the PathDispatcher as well) from having to know about containers...

I don't think that actually helps here. The ContainerInterface is a more widely used standard and offers ISP without having to define a local interface that (effectively) does the exact same thing.

Fine. So I'll just duplicate the three lines of resolve() both in the RequestHandler (where they originally come from) and the PathDispatcher class?

Define a trait that is used by both classes. DRY.

shadowhand · 2018-04-13T15:35:58Z

tests/PathBrokerTest.php

+
+class PathBrokerTest extends TestCase
+{
+    use CanMock;


I see no reference to the implementation of this trait.

I am calling $this->process().

Oh you're reusing an existing trait. Been a while since I worked on this. 😉

shadowhand · 2018-04-13T15:36:34Z

tests/PathBrokerTest.php

+        // Execute
+        $this->broker->paths([
+            '/foo' => new stubs\ReturnsGivenString('something'),
+            '/bar' => stubs\ReturnsGivenString::class,


Please use phony for mocking instead of creating stubs.

But I am not mocking anything. (I don't care whether and how the things are called, I just check for the expected results.)

It's not necessary to use Phony call checks, I just don't want stubs created when a mock implementation works equally well without adding more files.

Okay, it's your call. Will change this soon-ish.

Hmm. I find it super painful having to mock a middleware that returns a mocked response with a mocked body. Here is what I came up with... Am I missing something?

/** * @return \Eloquent\Phony\Mock\Handle\InstanceHandle */ private function mockMiddlewareReturning(string $responseBody) { $stream = Phony::mock(StreamInterface::class); $stream->__toString->returns($responseBody); $response = Phony::mock(ResponseInterface::class); $response->getBody->returns($stream); $middleware = Phony::mock(MiddlewareInterface::class); $middleware->process->returns($response); return $middleware; }

This is why I like stubs: they are real implementations, and usually super straight-forward.

If the only downside is having one more file in the test code, is that really a downside? (The stub is small enough to be put into the same file, if you prefer that...)

Always an option to use an anonymous class if the mocking is too much work. Which I see is already being done in some places.

I would also argue that for many of these tests using Phony mocks and call checks would provide better overall testing because the expectations inside the middleware should remain consistent. For instance in PathPrefixingHandler the expectation is that UriInterface is going to be manipulated. The test should reflect this, rather than testing the output the middleware.

But I don't want to test how the PathDispatcher does its work, I just wan't to test that it does it. That means two things:

1.) Ensure it dispatches to the correct middleware. I do this by comparing the expected responses, depending on which request I pipe through the broker.
2.) Ensure that it provides a manipulated request object to the middleware. I do this by returning the path (which is the part that should be manipulated) in the middleware response so that I can verify on the "outside" (a.k.a. from the result of the operation).

Mocking and call checks would couple me to a specific implementation, as far as I can tell, and that's what I wanted to avoid. Can you provide an example of how you would mock this without these downsides?

Why don't you want to test how it works? The internals are more important than the output here.

Because (in the tests) I don't care about how it does its job. There are a thousand ways to implement that. What exactly would you like to see mocked?

shadowhand · 2018-04-13T15:37:25Z

composer.json

@@ -20,7 +20,9 @@
    },
    "require-dev": {
        "eloquent/phony-phpunit": "^3.0",
-        "phpunit/phpunit": "^6.0"
+        "league/container": "^2.0",


I like testing with real implementations (to show real example usage). I can change to use a mock / stub if you want.

So... okay to keep? Not 100% sure how to interpret your 👍

No, remove it please.

shadowhand · 2018-04-13T19:47:56Z

composer.json

@@ -20,7 +20,9 @@
    },
    "require-dev": {
        "eloquent/phony-phpunit": "^3.0",
-        "phpunit/phpunit": "^6.0"
+        "league/container": "^2.0",


No, remove it please.

shadowhand · 2018-04-13T19:48:30Z

tests/PathBrokerTest.php

+
+class PathBrokerTest extends TestCase
+{
+    use CanMock;


Oh you're reusing an existing trait. Been a while since I worked on this. 😉

shadowhand · 2018-04-13T19:55:03Z

src/Broker.php

+        return $this;
+    }
+
+    public function resolve($middleware): Middleware


I don't think that actually helps here. The ContainerInterface is a more widely used standard and offers ISP without having to define a local interface that (effectively) does the exact same thing.

shadowhand · 2018-04-13T19:55:52Z

src/PathDispatcher.php

+        return $handler->handle($request);
+    }
+
+    private function unprefixedRequest(Request $request, $prefix): Request


string $prefix ?

shadowhand · 2018-04-13T19:55:59Z

src/PathDispatcher.php

+        );
+    }
+
+    private function prefixedHandler(Handler $handler, $prefix): Handler


string $prefix?

shadowhand · 2018-04-13T19:56:10Z

src/PathPrefixingHandler.php

+    /** @var string */
+    private $prefix;
+
+    public function __construct(RequestHandlerInterface $wrapped, $prefix)


string $prefix ?

shadowhand · 2018-04-13T19:58:44Z

tests/PathBrokerTest.php

+        // Execute
+        $this->broker->paths([
+            '/foo' => new stubs\ReturnsGivenString('something'),
+            '/bar' => stubs\ReturnsGivenString::class,


Always an option to use an anonymous class if the mocking is too much work. Which I see is already being done in some places.

I would also argue that for many of these tests using Phony mocks and call checks would provide better overall testing because the expectations inside the middleware should remain consistent. For instance in PathPrefixingHandler the expectation is that UriInterface is going to be manipulated. The test should reflect this, rather than testing the output the middleware.

franzliedke · 2018-04-14T14:40:53Z

src/PathDispatcher.php

+            $uri->withPath(
+                substr($uri->getPath(), strlen($prefix))
+            )
+        );


@shadowhand How would you test this method works correctly, without passing in a real Request instance?

franzliedke · 2018-04-15T20:41:46Z

@shadowhand I have taken care of the requested changes in the source code.

Regarding the tests: I have spent more than an hour trying to test this in a useful way using stubs only, and did not succeed. Right now, we have green (and, IMO, useful) tests. Can I leave this part to you, if it is important to you?

franzliedke · 2018-04-20T10:10:55Z

Hmm, you know what... if you are unsure about this, I can just add the PathDispatcher to my user-land code. Now that I have written it anyway.

No hard feelings either way. 😉

shadowhand · 2018-04-20T14:22:43Z

Don't worry, I'll get to this very soon. Just had to finish out my work week. Expect a release with it by Monday.

franzliedke · 2018-04-20T14:30:07Z

No pressure.

franzliedke · 2018-04-20T16:07:14Z

Another idea: Hey @oscarotero, would you be interested in accepting such a middleware (see proposed functionality in issue #6) into your middlewares organization?

oscarotero · 2018-04-20T16:51:20Z

@franzliedke That's a feature that many dispatchers provide (stratigility, middleland and now broker). But if you feel that it can be more useful in a middleware format, I do not feel bad.

franzliedke · 2018-04-20T16:59:29Z

@oscarotero As far as I can tell, Middleland does not strip the path from the request object. Broker does not (yet) have the implementation. That leaves us with Stratigility, which I did not want to use, because I don't (yet) want to drop PHP 7.0 support.

Hence, I suggested to put this into a generic, reusable package instead. Should I create my own repo first and then submit it for inclusion into your organization?

oscarotero · 2018-04-20T17:08:11Z

@franzliedke Yes, you're right. Middleland does not strip the path prefix, but it can be done with middlewares/base-path. That brings more flexibility because you can choose to strip the prefix to some paths but not in others.

Anyway, If you want to create the package, you can use this template and then move it to the middlewares organization.

franzliedke · 2018-04-24T15:07:44Z

@oscarotero Please have a look at the repository and let me know whether you consider this for inclusion. (And also if there is some other, more official path - or route, eh? - where I should send my, uhm, request.)

shadowhand · 2018-04-24T15:36:44Z

@franzliedke would the proposed middleware replace this PR?

franzliedke · 2018-04-24T15:38:51Z

Yep. (Although it currently lacks one thing in comparison to this PR, which is lazy evaluation of prefixed middlewares, e.g. from the container.)

oscarotero · 2018-04-24T16:59:41Z

@franzliedke Great. I left you some suggestions here: middlewares/base-path-router#1

shadowhand · 2018-04-24T22:10:52Z

@franzliedke I really appreciate the work you did here. In the interest of having the most widely usable solution, I am going to close this out in favor of the middleware, which is a universal solution.

franzliedke added 2 commits April 8, 2018 19:55

New feature: Map path prefixes to middleware stacks

584ecb3

Fixes #6.

Extract middleware resolution to interface

5714c0e

franzliedke mentioned this pull request Apr 11, 2018

Use PSR-15 for middleware, update to zend-stratigility 3.x flarum/framework#1404

Closed

1 task

shadowhand requested changes Apr 13, 2018

View reviewed changes

shadowhand reviewed Apr 13, 2018

View reviewed changes

shadowhand requested changes Apr 13, 2018

View reviewed changes

franzliedke commented Apr 14, 2018

View reviewed changes

franzliedke added 3 commits April 15, 2018 22:29

Extract a trait to DRY up middleware resolution

3d1d9dd

Add typehints

8cf6a0b

Fix minimum version constraints

828060d

shadowhand closed this Apr 24, 2018

shadowhand mentioned this pull request Sep 12, 2018

Proposal: Mapping path prefixes to middleware stacks #6

Closed

Map path prefixes to middleware stacks #7

Map path prefixes to middleware stacks #7

Conversation

franzliedke commented Apr 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

franzliedke commented Apr 15, 2018

franzliedke commented Apr 20, 2018

shadowhand commented Apr 20, 2018

franzliedke commented Apr 20, 2018

franzliedke commented Apr 20, 2018

oscarotero commented Apr 20, 2018

franzliedke commented Apr 20, 2018

oscarotero commented Apr 20, 2018

franzliedke commented Apr 24, 2018

shadowhand commented Apr 24, 2018

franzliedke commented Apr 24, 2018

oscarotero commented Apr 24, 2018

shadowhand commented Apr 24, 2018