PSR-15 - http-server-middleware [BC Break] #122

michalbundyra · 2017-11-13T11:21:25Z

BC Break

Dropped http-interop/http-middleware support and added http-interop/http-server-middleware.
This package requires PHP 7.0 so I've bumped version to PHP 7.1 in this PR.

Removed also functionality marked as deprecation.

- dropped http-interop/http-middleware support - dropped PHP 5.6 and 7.0 support

These tests are no longer valid, because now interfaces have defined return types and we got TypeError exception

asgrim · 2017-11-21T11:40:43Z

Probably worth linking to the discussion on this for anyone researching: https://discourse.zendframework.com/t/psr-15-compatibility-issues-and-proposal/378

weierophinney

I've reset the base branch to release-3.0.0 (which is currently matching develop), so once merged there, this will close correctly.

I've made a few comments of additional changes I think we should make before merging, however.

weierophinney · 2017-11-28T22:45:26Z

docs/book/creating-middleware.md

@@ -48,42 +47,23 @@ function (
 ## http-interop middleware

 You can also write middleware which implements interfaces from
-`http-interop/http-middleware`. Stratigility 2.1 supports all versions of


Instead of removing these bits and pieces, I think we should likely create a "v2" (and possibly "v1") tree in the docs that details features specific to those versions, and then indicate what major version the main doc tree targets. Otherwise, we get folks still on v2 wondering why syntax they copy from the manual does not work...

weierophinney · 2017-11-28T22:46:05Z

docs/book/migration/to-v2.md

@@ -231,6 +231,9 @@ Additionally, starting in version 2.0.0, `MiddlewarePipe` *will no longer implem
 http-interop/http-middleware `MiddlewareInterface`*. This has several
 repercussions.

+Stratigility 3.0.0 targets `http-interop/http-server-middleware`, and that version


This doesn't make sense in a "migration to v2" document!

Let's create a "migration to v3" document, too.

weierophinney · 2017-11-28T22:47:43Z

src/Delegate/CallableDelegateDecorator.php


 /**
 * Decorate callable delegates as http-interop delegates in order to process
 * incoming requests.
 */
-class CallableDelegateDecorator implements DelegateInterface
+class CallableDelegateDecorator implements RequestHandlerInterface


Is this class even necessary any more? With a new major version, this might be something we do away with.

Never mind, I see it being used in MiddlewarePipe::__invoke().

@weierophinney Instead of adding internal overhead in MiddlewarePipe to support these wrappers, I think it would be better to provide utility functions that does the wrapping at userland level. e.g. https://github.com/danizord/mid/blob/0.1/src/mid.php#L144-L146

@danizord I've spoke with @weierophinney and this is actually what we are doing, probably we will provide class wrappers or one class with multiple static methods. It will be done later on, probably with separate PR. Thanks for the suggestion.

Also, utility functions don't work well here, as we need to provide an implementation of an interface. Classes that accept the callback and implement the interface make sense. But we can address those in a later patch.

@weierophinney I mean an utility function that returns an anonymous object implementing the interface. It's a bit less verbose for consumers. Like that: https://github.com/danizord/mid/blob/0.1/src/mid.php#L144-L146

weierophinney · 2017-11-28T22:49:58Z

src/Middleware/CallableMiddlewareWrapper.php

@@ -44,19 +42,19 @@ public function __construct(callable $middleware, ResponseInterface $prototype)
    /**
     * Proxies to underlying middleware, using composed response prototype.
     *
-     * Also decorates the $delegator using the CallableMiddlewareWrapper.
+     * Also decorates the $handler using the CallableMiddlewareWrapper.


This sentence makes no sense. I think originally, it was saying that we wrapped the delegator in the CallableDelegatorWrapper. But we don't; we wrap it in a closure.

Let's remove the sentence.

weierophinney · 2017-11-28T22:53:51Z

src/MiddlewarePipe.php

@@ -31,7 +32,7 @@
 *
 * @see https://github.com/sencha/connect
 */
-class MiddlewarePipe implements ServerMiddlewareInterface
+class MiddlewarePipe implements MiddlewareInterface


I'm half-thinking we should have it implement RequestHandlerInterface as well. Doing so, hwoever, would require composing a response prototype internally so it can generate a response if none is produced by the pipeline. That could be done either via optional constructor argument, or a setter; if it is not present, and no response is produced, it would raise an exception.

We can address that at a later time, though.

@weierophinney I'd go further and remove the response prototype. Reaching the end of middleware pipeline is actually an exceptional case and should never happen. Instead I'd recommend using a NotFoundMiddleware at the end of pipeline.

weierophinney · 2017-11-28T22:56:30Z

src/Next.php

@@ -67,30 +65,6 @@ public function __construct(SplQueue $queue, DelegateInterface $nextDelegate = n
     *     not return a response.
     */
    public function __invoke(ServerRequestInterface $request)


Honestly, I think we should remove this. We don't use it internally, and with the requirement that PSR-15 middleware be used, folks should be aware that they shouldn't just invoke it, but rather call the handle() method.

If you do this, though, you'll also need to update the CallableDelegateWrapper, as it treats Next instances as callables.

@weierophinney

- per @weierophinney

@weierophinney

- per @weierophinney

Removed whole logic to wrap callbacks, interop-callbacks and double-pass callable middlewares

…ddleware Reverted back InvalidMiddlewareException class

weierophinney · 2017-11-29T21:30:16Z

src/Exception/InvalidMiddlewareException.php

+
+        return new self(
+            sprintf(
+                'Middleware must be callable, %s found',


This should change; it should indicate middleware must implement MiddlewareInterface.

@weierophinney

- per @weierophinney

PSR-15 - http-server-middleware [BC Break]

Close #122

michalbundyra added 3 commits November 13, 2017 10:53

Added http-interop/http-server-middleware suport

f6fcccf

- dropped http-interop/http-middleware support - dropped PHP 5.6 and 7.0 support

Removed invalid test cases

fca418d

These tests are no longer valid, because now interfaces have defined return types and we got TypeError exception

Removed deprecated functionality

4a1cb4d

michalbundyra mentioned this pull request Nov 13, 2017

Added http-server-middleware support #123

Closed

michalbundyra changed the base branch from master to develop November 13, 2017 11:23

weierophinney changed the base branch from develop to release-3.0.0 November 28, 2017 22:42

weierophinney suggested changes Nov 28, 2017

View reviewed changes

michalbundyra added 4 commits November 28, 2017 23:08

Added migration guide for v3

61203c0

- per @weierophinney

Removes redundant comment

2f08090

- per @weierophinney

Fixed v3 docs migration title

231f000

Updated PHP support section in v3 migration doc

d7ffe92

michalbundyra mentioned this pull request Nov 29, 2017

PSR-15 handler interface #116

Closed

michalbundyra added 8 commits November 29, 2017 18:23

MiddlewarePipe pipes only MiddlewareInstances

4557e40

Removed whole logic to wrap callbacks, interop-callbacks and double-pass callable middlewares

Removed CallableMiddlewareWrapper with tests

3173c5a

Removed tests for removed InvalidMiddlewareException

36e3ad3

Removed alias on MiddlewareInterface

2e94c03

Updated v3 migration with described all changes

abeb255

Updated documentation

deda929

Removed $responsePrototype property in MiddlewarePipe

237d680

Use Response() instead of EmptyResponse() in tests

cdd8497

weierophinney approved these changes Nov 29, 2017

View reviewed changes

michalbundyra added 2 commits November 29, 2017 21:28

Removed MiddlewareInterface typehing on MiddlewarePipe::pipe

6e27ed7

Throw InvalidMiddlewareException when pipe not MiddlewareInterface mi…

acf38ed

…ddleware Reverted back InvalidMiddlewareException class

weierophinney suggested changes Nov 29, 2017

View reviewed changes

Updated exception message

5a38e65

- per @weierophinney

weierophinney approved these changes Nov 29, 2017

View reviewed changes

weierophinney merged commit 5a38e65 into zendframework:release-3.0.0 Nov 29, 2017

weierophinney added a commit that referenced this pull request Nov 29, 2017

Merge pull request #122 from webimpress/feature/psr-15

aa85e4c

PSR-15 - http-server-middleware [BC Break]

weierophinney added a commit that referenced this pull request Nov 29, 2017

Adds CHANGELOG entries for #122

632783e

weierophinney added a commit that referenced this pull request Nov 29, 2017

Merge branch 'feature/122-psr-15' into release-3.0.0

01d3c89

Close #122

michalbundyra deleted the feature/psr-15 branch November 29, 2017 22:47

weierophinney mentioned this pull request Nov 30, 2017

Adds PSR-15 decorator classes for callable middleware #124

Merged

weierophinney added this to the 3.0.0alpha1 milestone Nov 30, 2017

weierophinney mentioned this pull request Nov 30, 2017

Provide versioned documentation #127

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PSR-15 - http-server-middleware [BC Break] #122

PSR-15 - http-server-middleware [BC Break] #122

michalbundyra commented Nov 13, 2017

asgrim commented Nov 21, 2017

weierophinney left a comment

weierophinney Nov 28, 2017

weierophinney Nov 28, 2017

weierophinney Nov 28, 2017

weierophinney Nov 28, 2017

danizord Nov 29, 2017

michalbundyra Nov 29, 2017

weierophinney Nov 29, 2017

danizord Nov 29, 2017

weierophinney Nov 28, 2017

weierophinney Nov 28, 2017

danizord Nov 29, 2017

weierophinney Nov 28, 2017

weierophinney Nov 28, 2017

weierophinney Nov 29, 2017

PSR-15 - http-server-middleware [BC Break] #122

PSR-15 - http-server-middleware [BC Break] #122

Conversation

michalbundyra commented Nov 13, 2017

asgrim commented Nov 21, 2017

weierophinney 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

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