Allow HEAD and OPTIONS requests for any matched route #413

weierophinney · 2016-12-15T18:48:48Z

Updates zend-expressive-router dependency to 1.3.2+, and related new versions of router implementations to those supporting that version.

This patch also introduces two new classes, Zend\Expressive\Middleware\ImplicitHeadMiddleware and ImplicitOptionsMiddleware. These can be placed after the routing middleware, but before the dispatch middleware, in order to handle HEAD and OPTIONS requests when the matched Route does not explicitly support them. In each case, they trigger only when:

the method is the one they are specifically interested in;
the request composes a RouteResult attribute;
the RouteResult composes a Route instance (per the changes in zend-expressive-router and the various implementations);
the Route instance indicates that the method is only supported implicitly.

Additionally, for routes supporting GET requests, the ImplicitHeadMiddleware will dispatch the next middleware in the layer, using GET as the request method, and ensure the returned response has an empty body.

This approach ensures that if a route explicitly supports the given method, then the matched middleware is dispatched as normal.

Fixes #398

michalbundyra

@weierophinney It looks good for me. I haven't test it yet, just reviewed. Are you going to include some tests from #398 as well or they are redundant now?

michalbundyra · 2016-12-15T18:57:05Z

src/Application.php

+        callable $middleware,
+        ServerRequestInterface $request,
+        ResponseInterface $response,
+        callable $next


Param $next is missing in PHPDoc.

Fixed; thanks!

michalbundyra · 2016-12-15T19:01:07Z

src/Application.php

+     * Returns an empty 200 response with an Allow header.
+     *
+     * @param Router\Route $route
+     * @return ResponseInterface


Method always returns Response.

Fixed, thanks!

michalbundyra · 2016-12-15T19:03:21Z

test/DispatchMiddlewareTest.php

+        $test = $app->dispatchMiddleware($request, $response, $delegate);
+
+        $this->assertInstanceOf(Response::class, $test);
+        $this->assertEquals(200, $test->getStatusCode());


Should we use constant also here?

Yep; fixed, thanks!

danizord · 2016-12-15T19:14:21Z

@weierophinney what about shipping them as standalone, optional middlewares?

weierophinney · 2016-12-15T21:42:20Z

@weierophinney what about shipping them as standalone, optional middlewares?

Considering it, but need to figure out how that would work within the Application instance. (Same issue I have with the routing/dispatch middleware, to a degree.)

weierophinney · 2016-12-15T21:45:53Z

Are you going to include some tests from #398 as well or they are redundant now?

Reviewing this now. I merged your changes from that PR locally, and have a ton of failures, and I'm trying to analyze if they're due to the changes I've made, or invalid expectations. I'll keep you posted.

weierophinney · 2016-12-15T22:05:53Z

@webimpress I'm reviewing the tests, and the main issue is that those that trigger the implicit HEAD/OPTIONS usage were checking for a populated response body, when it should not be; updating those tests to look for an empty string for the body makes them pass.

The one I'm struggling with right now is testNotAllowedMethodWhenNoHttpMethodsSet when used with the Aura.Router. Evidently, in v3, if you do not specify any HTTP methods, these routes match any. The solution is to look for empty($route->allows) on the matched route. I'm going to push a 1.1.2 release of that library that does that, and all of your tests from #398 will then pass (with the changes I made for testing the response body contents). As such, I'll close #398 once I've pushed the updates to this PR.

michalbundyra · 2016-12-15T22:09:49Z

Thanks, @weierophinney !

weierophinney · 2016-12-15T23:36:35Z

@danizord —

what about shipping them as standalone, optional middlewares?

I've separated them out at this point locally. For the implicit options middleware, it's not a problem at all, as it's only working with the route, which is obtainable from the route result, which is obtainable as a request option.

For the implicit head middleware, things get more dicey, as we need the matched middleware in the case that the route allows GET, but HEAD is only implied. This requires passing it as a constructor argument, which means it must be composed as part of routing or dispatching middleware, since this is typically marshaled from a container.

Ideally, these would operate between the routing and dispatch middleware. However, I do not want to force end-users to add these to their pipeline. As such, still debating; I'll try and make a decision in the next day or two.

danizord · 2016-12-16T12:31:25Z

@weierophinney

For the implicit head middleware, things get more dicey, as we need the matched middleware in the case that the route allows GET, but HEAD is only implied. This requires passing it as a constructor argument, which means it must be composed as part of routing or dispatching middleware, since this is typically marshaled from a container.

If we pipe ImplicitHeadMiddleware before dispatch middleware, we don't need to know the routed middleware, we just call $response = $delegate->process(); to run the routed middleware.

Ideally, these would operate between the routing and dispatch middleware. However, I do not want to force end-users to add these to their pipeline. As such, still debating; I'll try and make a decision in the next day or two.

Mhmm, why not? We already have some basic functionally such as ServerUrlMiddleware and BodyParamsMiddleware that are piped manually, and I like this approach because it makes these behaviors explicit and optional, so developers are not wondering why the framework did something automatically.

weierophinney · 2016-12-19T15:02:35Z

@danizord Thought about this over the weekend, and I agree with you. I've updated the patch to provide optional middleware, Zend\Expressive\Middleware\ImplicitHeadMiddleware and ImplicitOptionsMiddleware, each of which may be slip-streamed between routing and dispatch middleware.

In the skeleton, we can enable these in the default pipeline. I will also likely add some support for them into the tooling for creating a pipeline from existing configuration.

I've pushed the changes at this time, but need to document them. Otherwise, the code is ready for review.

weierophinney · 2016-12-19T15:50:09Z

I've rebased this patch to remove the commits that were updating the dispatch middleware to add the implicit HEAD/OPTIONS support.

michalbundyra · 2016-12-19T15:51:00Z

src/Application.php

@@ -19,6 +20,7 @@
 use Zend\Diactoros\Response\SapiEmitter;
 use Zend\Diactoros\ServerRequest;
 use Zend\Diactoros\ServerRequestFactory;
+use Zend\Diactoros\Stream;


No longer used in that class.

michalbundyra · 2016-12-19T15:51:44Z

src/Application.php

+    /**
+     * HTTP methods that are implicitly allowed for any matched request.
+     */
+    const HTTP_METHODS_IMPLICIT = [


Seems to be unused. Do we still need it?

Updates zend-expressive-router dependency to 1.3.2+, and related new versions of router implementations to those supporting that version. This patch adds new optional middleware, `Zend\Expressive\Middleware\ImplicitHeadMiddleware` and `ImplicitOptionsMiddleware`, that can be placed between the routing and dispatch middleware in order to handle HEAD and OPTIONS requests when they are only implicitly supported by the matched route. Each only triggers on the request method they are interested in, and then only when: - a route result is present in the request - the route result contains a route - and the route indicates the method is only implicitly supported For the implicit HEAD support, it will also check to see if GET is supported before returning a response; if so, it dispatches the next middleware layer, providing an updated request using GET as the method, and ensures that the response returned has an empty body. This patch also incorporates tests from zendframework#398, with updates for the following: - Response bodies for implicit HEAD and OPTIONS requests should be empty. - zend-expressive-aurarouter failed until version 1.1.3.

`Generator` now does the following: - Adds dependency configuration for the implicit HEAD and OPTIONS middleware to `config/autoload/programmatic-pipeline.global.php`. - Adds entries for the implicit HEAD and OPTIONS middleware to the pipeline immediately following the routing middleware. This is to support the changes in zendframework/zend-expressive#413, and should not be merged or tagged before that functionality has been released in Expressive 1.1.

weierophinney · 2016-12-19T16:28:57Z

@xtreamwayz This is another one to consider for the skeleton: the ImplicitHeadMiddleware and ImplicitOptionsMiddleware should be part of the default pipeline, and occur between the routing and dispatch middleware. This could occur either before or after the UrlHelperMiddleware; the patch I have for the tooling puts it before, but that's because I cannot know for certain what else is present between the two.

moderndeveloperllc · 2016-12-19T17:01:14Z

@weierophinney I see these are using the __invoke() pattern. Considering that Stratigility 1.3.x is required, would it be better to use the http-interop DelegateInterface pattern, or are y'all not wanting to switch over to that in Expressive right now?

weierophinney · 2016-12-19T17:06:14Z

I see these are using the __invoke() pattern.

There are changes pending for a 0.4.0 release of http-middleware that will impact the signatures (new namespace, and the DelegateInterface now typehints against ServerRequestInterface instead of RequestInterface), and since I do not know how soon that will drop, I'm waiting to update until a release is made. Once it has, I'll be needing to make changes to Stratigility as well, before we even update Expressive.

weierophinney · 2016-12-19T17:08:07Z

I see these are using the __invoke() pattern.

Also, Stratigility provides functionality for wrapping double-pass middleware such as this to work with http-middleware, so it can still be utilized in that context; Expressive will be (eventually) doing this out-of-the-box once moving to a Stratigility version that only supports http-middleware. However, that will not happen until PSR-15 is accepted.

moderndeveloperllc · 2016-12-19T17:17:13Z

@weierophinney Good to know. I guess that's Stratigility 2.0 that will be going http-middleware only. FYI, it looks like 0.4.0 dropped a couple of days ago. Not a huge fan of going from Interop\Http\Middleware\ServerMiddlewareInterface to Interop\Http\ServerMiddleware\MiddlewareInterface, but I can understand why.

weierophinney · 2016-12-19T17:28:13Z

Not a huge fan of going from

That was prompted by me, actually. 😄 Before this, we had:

A generic Interop\Http\Middleware interface, with
a ServerMiddlewareInterface that depended on a
DelegateInterface, which depended originally on
RequestInterface, but later was updated to depend on
ServerRequestInterface

In other words, huge inconsistencies in naming that, I felt, would lead to difficulties understanding the roles each played. I provided a number of options forward, and, ultimately, most liked having a different namespace for the server-side middleware artifacts.

As you note, however, it causes us some issues with upgrading. I wasn't aware 0.4.0 had dropped, I need to see what the latest changes are, and make sure there are none others pending before we start adapting support for it.

weierophinney · 2016-12-19T17:32:53Z

Hmmm... there's one open pull request on http-middleware that could still have ramifications, as it would result in at least a rename of DelegateInterface to RequestHandlerInterface, and potentially a rename of that interface's process() method to __invoke(). As such, I need to wait until that pull request is resolved before we move forward on Stratigility. In the meantime, as noted above, we can implement __invoke() safely.

michaelmoussa

👍 LGTM

Documents the new middleware, both in a new feature section, and within the migration guide.

malukenho · 2016-12-20T19:20:41Z

src/Middleware/ImplicitHeadMiddleware.php

+    private function getResponse()
+    {
+        if ($this->response) {
+            return $this->response;


Why not check and assign it on __contructor?

There's no need to create one if the middleware does not need to return a response in the first place.

- Filenames cannot contain extra `.` characters; mkdocs will not find them if they are present. - Formatting errors due to nested bullets and attempts to have multiple paragraphs.

Close #413 Close #398

Replaces section on #413 under `Changes` headline.

michalbundyra reviewed Dec 15, 2016

View reviewed changes

weierophinney added this to the 1.1.0 milestone Dec 15, 2016

weierophinney added bug enhancement labels Dec 15, 2016

weierophinney mentioned this pull request Dec 15, 2016

HEAD and OPTIONS requests #398

Closed

weierophinney force-pushed the feature/implicit-head-options branch from 33789f5 to c49a520 Compare December 19, 2016 15:49

michalbundyra reviewed Dec 19, 2016

View reviewed changes

weierophinney force-pushed the feature/implicit-head-options branch from c49a520 to 833cdbe Compare December 19, 2016 15:53

weierophinney mentioned this pull request Dec 19, 2016

Inject implicit HEAD, OPTIONS middleware into generated pipeline zendframework/zend-expressive-tooling#7

Merged

weierophinney requested review from michaelmoussa and geerteltink December 19, 2016 20:13

danizord approved these changes Dec 20, 2016

View reviewed changes

michaelmoussa approved these changes Dec 20, 2016

View reviewed changes

Documented changes for zendframework#413

59b2914

Documents the new middleware, both in a new feature section, and within the migration guide.

malukenho suggested changes Dec 20, 2016

View reviewed changes

Fixes navigation and formatting

3268a6d

- Filenames cannot contain extra `.` characters; mkdocs will not find them if they are present. - Formatting errors due to nested bullets and attempts to have multiple paragraphs.

weierophinney merged commit 3268a6d into zendframework:develop Dec 20, 2016

weierophinney added a commit that referenced this pull request Dec 20, 2016

Added CHANGELOG for #413

f09e421

weierophinney added a commit that referenced this pull request Dec 20, 2016

Merge branch 'feature/implicit-head-options' into develop

6a5e710

Close #413 Close #398

weierophinney deleted the feature/implicit-head-options branch December 20, 2016 19:34

weierophinney mentioned this pull request Dec 20, 2016

Lightweight module system with config manager zendframework/zend-expressive-skeleton#54

Merged

7 tasks

weierophinney added a commit that referenced this pull request Jan 12, 2017

Added CHANGELOG for #422

0e32052

Replaces section on #413 under `Changes` headline.

weierophinney modified the milestones: 1.1.0, 2.0.0 Jan 30, 2017

michalbundyra mentioned this pull request Feb 9, 2017

1.1 Implicit Methods Middleware are not available anymore #436

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow HEAD and OPTIONS requests for any matched route #413

Allow HEAD and OPTIONS requests for any matched route #413

weierophinney commented Dec 15, 2016 •

edited

michalbundyra left a comment

michalbundyra Dec 15, 2016

weierophinney Dec 15, 2016

michalbundyra Dec 15, 2016

weierophinney Dec 15, 2016

michalbundyra Dec 15, 2016

weierophinney Dec 15, 2016

danizord commented Dec 15, 2016

weierophinney commented Dec 15, 2016

weierophinney commented Dec 15, 2016

weierophinney commented Dec 15, 2016

michalbundyra commented Dec 15, 2016

weierophinney commented Dec 15, 2016

danizord commented Dec 16, 2016 •

edited

weierophinney commented Dec 19, 2016

weierophinney commented Dec 19, 2016

michalbundyra Dec 19, 2016

michalbundyra Dec 19, 2016

weierophinney commented Dec 19, 2016

moderndeveloperllc commented Dec 19, 2016

weierophinney commented Dec 19, 2016 •

edited

weierophinney commented Dec 19, 2016

moderndeveloperllc commented Dec 19, 2016

weierophinney commented Dec 19, 2016

weierophinney commented Dec 19, 2016

michaelmoussa left a comment

malukenho Dec 20, 2016

weierophinney Dec 20, 2016

Allow HEAD and OPTIONS requests for any matched route #413

Allow HEAD and OPTIONS requests for any matched route #413

Conversation

weierophinney commented Dec 15, 2016 • edited

michalbundyra left a 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

danizord commented Dec 15, 2016

weierophinney commented Dec 15, 2016

weierophinney commented Dec 15, 2016

weierophinney commented Dec 15, 2016

michalbundyra commented Dec 15, 2016

weierophinney commented Dec 15, 2016

danizord commented Dec 16, 2016 • edited

weierophinney commented Dec 19, 2016

weierophinney commented Dec 19, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weierophinney commented Dec 19, 2016

moderndeveloperllc commented Dec 19, 2016

weierophinney commented Dec 19, 2016 • edited

weierophinney commented Dec 19, 2016

moderndeveloperllc commented Dec 19, 2016

weierophinney commented Dec 19, 2016

weierophinney commented Dec 19, 2016

michaelmoussa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weierophinney commented Dec 15, 2016 •

edited

danizord commented Dec 16, 2016 •

edited

weierophinney commented Dec 19, 2016 •

edited