Skip to content
This repository has been archived by the owner on Jan 29, 2020. It is now read-only.

Added MiddlewarePipeInterface as abstraction layer for MiddlewarePipe #146

Merged
merged 6 commits into from
Jan 24, 2018
Merged

Added MiddlewarePipeInterface as abstraction layer for MiddlewarePipe #146

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
9dde7ec
Added MiddlewarePipeInterface as abstraction layer for MiddlewarePipe
michalbundyra Jan 23, 2018
9481159
Added test for MiddlewarePipe
michalbundyra Jan 23, 2018
97cf47a
Updated test - check if MiddlewarePipe is final
michalbundyra Jan 23, 2018
d5de60a
Import ReflectionMethod
michalbundyra Jan 24, 2018
7e9266a
Updated test name
michalbundyra Jan 24, 2018
c16cb6e
Added notes in migration to v3 docs about MiddlewarePipeInterface
michalbundyra Jan 24, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/MiddlewarePipe.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@
*
* @see https://github.com/sencha/connect
*/
final class MiddlewarePipe implements MiddlewareInterface, RequestHandlerInterface
final class MiddlewarePipe implements MiddlewarePipeInterface
{
/**
* @var SplQueue
Expand Down
17 changes: 17 additions & 0 deletions src/MiddlewarePipeInterface.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
<?php
/**
* @see https://github.com/zendframework/zend-stratigility for the canonical source repository
* @copyright Copyright (c) 2018 Zend Technologies USA Inc. (https://www.zend.com)
* @license https://github.com/zendframework/zend-stratigility/blob/master/LICENSE.md New BSD License
*/
declare(strict_types=1);

namespace Zend\Stratigility;

use Psr\Http\Server\MiddlewareInterface;
use Psr\Http\Server\RequestHandlerInterface;

interface MiddlewarePipeInterface extends MiddlewareInterface, RequestHandlerInterface
{
public function pipe(MiddlewareInterface $middleware) : void;
}
30 changes: 30 additions & 0 deletions test/MiddlewarePipeTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,13 @@
use Psr\Http\Message\ServerRequestInterface;
use Psr\Http\Server\MiddlewareInterface;
use Psr\Http\Server\RequestHandlerInterface;
use ReflectionClass;
use ReflectionObject;
use Zend\Diactoros\Response;
use Zend\Diactoros\ServerRequest as Request;
use Zend\Stratigility\Exception;
use Zend\Stratigility\MiddlewarePipe;
use Zend\Stratigility\MiddlewarePipeInterface;

class MiddlewarePipeTest extends TestCase
{
Expand Down Expand Up @@ -179,4 +182,31 @@ public function testHandleProcessesEnqueuedMiddleware()

$this->assertSame($response, $pipeline->handle($this->request));
}

public function testMiddlewarePipeIsInstanceOfMiddlewarePipeInterface()
Copy link
Member

Choose a reason for hiding this comment

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

I'd rename this to testMiddlewarePipeOnlyImplementsMiddlewarePipeInterfaceApi(), as that's what it's really doing.

Honestly, I'm not 100% certain we need this to be as thorough as it is; checking that MiddlewarePipe instanceof MiddlewarePipeInterface is likely enough for our purposes. There may be reasons to add additional public methods down the line, primarily to provide optional collaborators, that would invalidate this test otherwise.

Copy link
Member Author

Choose a reason for hiding this comment

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

@weierophinney

There may be reasons to add additional public methods down the line, primarily to provide optional collaborators, that would invalidate this test otherwise.

And this was the reason why I've created that test as thorough as it is. I think any public method added into that class, should be also defined in the abstraction later. This is the whole point of that PR.
If test fails, it will show us that something is changed, and we will have chance to think again if it's really needed there.

Copy link
Member

Choose a reason for hiding this comment

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

I'm convinced. Thanks for the feedback!

{
$pipeline = new MiddlewarePipe();

$r = new ReflectionObject($pipeline);
$methods = $r->getMethods(\ReflectionMethod::IS_PUBLIC);
Copy link
Member

Choose a reason for hiding this comment

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

Import ReflectionMethod, please.

$actual = [];
foreach ($methods as $method) {
if (strpos($method->getName(), '__') !== 0) {
$actual[] = $method->getName();
}
}
sort($actual);

$interfaceReflection = new ReflectionClass(MiddlewarePipeInterface::class);
$interfaceMethods = $interfaceReflection->getMethods(\ReflectionMethod::IS_PUBLIC);
$expected = [];
foreach ($interfaceMethods as $method) {
$expected[] = $method->getName();
}
sort($expected);

self::assertTrue($r->isFinal());
self::assertEquals($expected, $actual);
self::assertInstanceOf(MiddlewarePipeInterface::class, $pipeline);
}
}