-
Notifications
You must be signed in to change notification settings - Fork 57
Added MiddlewarePipeInterface as abstraction layer for MiddlewarePipe #146
Added MiddlewarePipeInterface as abstraction layer for MiddlewarePipe #146
Conversation
MiddlewarePipe is a final class, so we need to have abstraction layer to make mocking easy Fixes #144
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very much a 👍 ; will merge in a bit and update the docs when I do.
Check if it implements MiddlewarePipeInterface and if all public methods (not special methods) are defined in MiddlewarePipeInterface.
@weierophinney - I've added unit test into this PR, please have a look |
test/MiddlewarePipeTest.php
Outdated
$pipeline = new MiddlewarePipe(); | ||
|
||
$r = new ReflectionObject($pipeline); | ||
$methods = $r->getMethods(\ReflectionMethod::IS_PUBLIC); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Import ReflectionMethod
, please.
test/MiddlewarePipeTest.php
Outdated
@@ -179,4 +182,31 @@ public function testHandleProcessesEnqueuedMiddleware() | |||
|
|||
$this->assertSame($response, $pipeline->handle($this->request)); | |||
} | |||
|
|||
public function testMiddlewarePipeIsInstanceOfMiddlewarePipeInterface() |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
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!
Thanks, @webimpress! |
MiddlewarePipe
is afinal
class, so we need to have abstraction layer to make mocking easyFixes #144