Provide support for zend-stratigility 2.2.2 #293

thexpand · 2018-09-11T22:32:50Z

Travis was failing on latest builds on all platforms. I've ran composer update and came across a deprecation error when I ran the unit tests. Callable double-pass middleware must be decorated in order to work with the new version of zend-stratigility.
That's what this hotfix is all about - it includes the updated Composer files and the decorated middleware.

src/MiddlewareListener.php

froschdesign · 2018-09-12T05:26:39Z

src/MiddlewareListener.php

@@ -146,6 +147,19 @@ private function createPipeFromSpec(
                throw InvalidMiddlewareException::fromMiddlewareName($middlewareName);
            }

+            // Decorate double-pass middleware
+            if (is_callable($middlewareToBePiped)) {
+                $r = $this->getReflectionFunction($middlewareToBePiped);


Please use always meaningful names for variables. (loops are an exception)

But do we really need a reflection here? Smells like a hack.
A check of the interfaces and a simple method_exists is maybe enough.

This has been taken from zend-stratigility as a way of finding whether the middleware is a double-pass one, but I'll change it (it seemed like an over-complication to me, too).

This has been taken from zend-stratigility…

Ah, good to know.

it seemed like an over-complication to me, too

If the interface Interop\Http\ServerMiddleware\MiddlewareInterface is not present but the current middleware is callable, then there is also no check for the parameters.
And the decorator DoublePassMiddlewareDecorator includes also a test of the result.

I've applied changes. Now we check whether the callable is missing the process method or if it does not implement the Interop\Http\ServerMiddleware\MiddlewareInterface, then wraps it, using the decorator.

froschdesign · 2018-09-12T06:06:58Z

Related to #290

froschdesign · 2018-09-12T09:28:16Z

src/MiddlewareListener.php

+            if (is_callable($middlewareToBePiped)
+                && (
+                    ! $middlewaresToBePiped instanceof MiddlewareInterface
+                    || ! method_exists($middlewaresToBePiped, 'process')


These changes should covered by unit tests.

A new test testSuccessfullyDispatchesDoublePassMiddleware?

For example.

I've added the test, but as I come to think about the if condition, doesn't it have to be x && !y && !z instead of x && (!y || !z).. the first one makes more sense than the second one. Otherwise middlewares that are not an instance of MiddlewareInterface, but have the process method would be decorated, as well. Hm...

geerteltink

I've been digging a bit deeper into the failed tests and this PR. What you are basically doing is decorating the middleware in the tests to prevent the deprecation notice from stratigility. Doing it this way it will prevent notices for users as well. By the time zend-mvc is updated to support stratigility 3.0 their applications are not working anymore and they never saw a notice.

I think the correct way is to actually fix the middleware listener tests.

zend-mvc/test/MiddlewareListenerTest.php

Lines 62 to 69 in a8d4568

    
           public function testSuccessfullyDispatchesMiddleware() 
        
           { 
        
               $event = $this->createMvcEvent('path', function ($request, $response) { 
        
                   $this->assertInstanceOf(ServerRequestInterface::class, $request); 
        
                   $this->assertInstanceOf(ResponseInterface::class, $response); 
        
                   $response->getBody()->write('Test!'); 
        
                   return $response; 
        
               });

As you can see that test uses the double pass middleware. This should be changed to use the PSR-12 Middleware interface:
public function process(ServerRequestInterface $request, RequestHandlerInterface $handler): ResponseInterface;

geerteltink · 2018-09-12T19:51:31Z

src/MiddlewareListener.php

 use Psr\Http\Message\ResponseInterface;
-use Psr\Http\Message\ServerRequestInterface as PsrServerRequestInterface;
+use Psr\Http\Message\ResponseInterface as PsrResponseInterface;


Psr\Http\Message\ResponseInterface is imported twice?

It was there before the changes, but I fixed it anyway.

geerteltink · 2018-09-13T05:52:27Z

To summarize the discussion we had last night on Slack:

Currently all zend-mvc latest tests fail because of a deprecation notice in stratigility 2.2. The notices are there for a reason and shouldn't be suppressed. See here and here.
Stratigility 3.0 uses PSR-15
Stratigility 2.2 has the DoublePassMiddlewareDecorator which is needed to properly suppress the deprecation notice.
Stratigility 2.1 has support for http-interop/http-middleware 0.5.0 via a polyfill.
Stratigility 2.0 has support for http-interop/http-middleware version to 0.4.1

zend-mvc is tested against stratigility ^2.0.1. The deprecation notices were added in 2.2.0.

Since stratigility 3 requires php ^7.1 and zend-mvc requires ^5.6 || ^7.0, support for PSR-15 is out of the question. We tried to make expressive 2 compatible with all different middleware interfaces and it became pretty messy.

I suggested this solution:

Release a zend-mvc 2.7.16 hotfix that restricts stratigility in composer require-dev to "zendframework/zend-stratigility": ">=2.0.1 <2.2" and fix the tests.
Release zend-mvc 2.8.0 with support for "zendframework/zend-stratigility": "^2.2.2" and use the DoublePassMiddlewareDecorator in the MiddlewareListenerTest to fix the current failing latest tests.

/cc @weierophinney

froschdesign · 2018-09-13T06:19:44Z

@xtreamwayz
Sorry, maybe I missed something. But why 2.7. and 2.8?

thexpand · 2018-09-13T07:19:53Z

The master branch is currently on 3.1.1, so the hotfix will be released as a new patch version 3.1.2 and then the update to support stratigility 2.2.2 will be released as a new minor version 3.2.0.

weierophinney · 2018-09-13T13:31:47Z

I suggested this solution:

1. Release a zend-mvc `2.7.16` hotfix that restricts stratigility in composer require-dev to `"zendframework/zend-stratigility": ">=2.0.1 <2.2"` and fix the tests.

2. Release zend-mvc `2.8.0` with support for `"zendframework/zend-stratigility": "^2.2.2"` and use the `DoublePassMiddlewareDecorator` in the `MiddlewareListenerTest` to fix the current failing `latest` tests.

The v2 series of zend-mvc is no longer supported (see the LTS page for details). As such, we can ignore those versions entirely. Users will have to heed the notices and update their code accordingly.

For v3, the approach @thexpand outlines seems sensible.

geerteltink · 2018-09-13T18:24:33Z

Sorry, maybe I missed something. But why 2.7. and 2.8?

No, I missed something... the zend-mvc 3 release :D

boesing · 2018-09-17T14:55:18Z

So I guess this can be closed in favor of #295?

thexpand · 2018-09-17T15:08:46Z

@boesing This PR has nothing to do with #295
It is about providing support for stratigility 2.2.2.
As outlined above, this is the second step. The PR you are talking about is meant to only fix the failing tests.

boesing · 2018-09-17T15:59:31Z

@thexpand ah never mind, just saw the stratigility change and thought this was the the same composer change as in #295
So just ignore my comment.

Xerkus · 2019-02-07T06:11:35Z

I marked this PR as won't fix but will keep it open for now. Next week I will pull the stratigility support into satellite mvc package and reapply 2.2.2 changes there.

Xerkus · 2019-02-17T03:32:59Z

Alternative solution to be provided by #308

Bump the minimum version of zend-stratigility to 2.2.2

7f752a7

thexpand mentioned this pull request Sep 11, 2018

Make use of class names instead of strings for services #292

Closed

froschdesign suggested changes Sep 12, 2018

View reviewed changes

froschdesign added enhancement documentation needed labels Sep 12, 2018

thexpand added 3 commits September 12, 2018 11:43

Update doc block header comment

08d3e49

Optimize and re-order use statements

dcdc19e

Decorate callable double-pass middleware to fix deprecation errors

2d54d6e

thexpand force-pushed the hotfix/double-pass-middleware branch from 41f6194 to 2d54d6e Compare September 12, 2018 08:50

froschdesign added the unit test needed label Sep 12, 2018

froschdesign suggested changes Sep 12, 2018

View reviewed changes

thexpand added 2 commits September 12, 2018 15:21

Add test for dispatching double-pass middleware

9d4bfb1

Update header doc block comment

b8fff0c

geerteltink reviewed Sep 12, 2018

View reviewed changes

thexpand added 2 commits September 12, 2018 23:47

Remove code for decorating double pass middleware

9adc92c

Remove duplicated import of ResponseInterface

6c0d188

thexpand mentioned this pull request Sep 13, 2018

Hotfix/failing latest deps tests #295

Merged

thexpand changed the title ~~Hotfix - callable double pass middleware~~ Provide support for zend-stratigility 2.2.2 Sep 17, 2018

froschdesign mentioned this pull request Sep 19, 2018

add requirement of zend-stratigility to documentation #290

Closed

Xerkus added the wontfix label Feb 4, 2019

Xerkus closed this Feb 17, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Provide support for zend-stratigility 2.2.2 #293

Provide support for zend-stratigility 2.2.2 #293

thexpand commented Sep 11, 2018

froschdesign Sep 12, 2018

thexpand Sep 12, 2018

froschdesign Sep 12, 2018 •

edited

Loading

thexpand Sep 12, 2018

froschdesign commented Sep 12, 2018

froschdesign Sep 12, 2018

thexpand Sep 12, 2018

froschdesign Sep 12, 2018

thexpand Sep 12, 2018

geerteltink left a comment •

edited

Loading

geerteltink Sep 12, 2018

thexpand Sep 12, 2018

geerteltink commented Sep 13, 2018 •

edited

Loading

froschdesign commented Sep 13, 2018

thexpand commented Sep 13, 2018

weierophinney commented Sep 13, 2018

geerteltink commented Sep 13, 2018

boesing commented Sep 17, 2018

thexpand commented Sep 17, 2018 •

edited

Loading

boesing commented Sep 17, 2018

Xerkus commented Feb 7, 2019

Xerkus commented Feb 17, 2019

	public function testSuccessfullyDispatchesMiddleware()
	{
	$event = $this->createMvcEvent('path', function ($request, $response) {
	$this->assertInstanceOf(ServerRequestInterface::class, $request);
	$this->assertInstanceOf(ResponseInterface::class, $response);
	$response->getBody()->write('Test!');
	return $response;
	});

Provide support for zend-stratigility 2.2.2 #293

Provide support for zend-stratigility 2.2.2 #293

Conversation

thexpand commented Sep 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

froschdesign Sep 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

froschdesign commented Sep 12, 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

geerteltink left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

geerteltink commented Sep 13, 2018 • edited Loading

froschdesign commented Sep 13, 2018

thexpand commented Sep 13, 2018

weierophinney commented Sep 13, 2018

geerteltink commented Sep 13, 2018

boesing commented Sep 17, 2018

thexpand commented Sep 17, 2018 • edited Loading

boesing commented Sep 17, 2018

Xerkus commented Feb 7, 2019

Xerkus commented Feb 17, 2019

froschdesign Sep 12, 2018 •

edited

Loading

geerteltink left a comment •

edited

Loading

geerteltink commented Sep 13, 2018 •

edited

Loading

thexpand commented Sep 17, 2018 •

edited

Loading