Implement programmatic pipeline features #396

weierophinney · 2016-11-11T22:40:16Z

This patch implements the Programmatic Pipelines RFC requirements.

TODO

Tooling support

The following tools were written in zendframework/zend-expressive-tooling to support user migrations to work with the above changes:

Tool for generating programmatic pipelines and routing from existing configuration.
Tool for migrating getOriginal*() calls to getAttribute(*) calls on the request object.
Tool for identifying error middleware in an existing application.

References

Programmatic Pipelines RFC
Add the new error handler classes #392 (original draft of error handler classes)
Stratigility 1.3/2.0 migration docs

weierophinney · 2016-11-17T16:33:46Z

Ready for review! Pinging:

@zendframework/community-review-team
@michaelmoussa
@xtreamwayz
@ezimuel

Thanks in advance!

Koopzington · 2016-11-17T16:48:34Z

composer.json

-        "cs-fix": "phpcbf",
-        "test": "phpunit",
+        "cs-check": "phpcs --colors",
+        "cs-fix": "phpcbf --colors",


--colors for phpcs and phpcbf is not needed since the new coding standard will have them enabled via the ruleset.xml

Good to know; I'll push that change shortly, after verifying the other build changes pass.

geerteltink

I've read the documentation changes so far. Later tonight or tomorrow night I'll go through the code.

I like vfsStream. We should used use it for the skeleton as well. It will make testing a lot cleaner and safer.

geerteltink · 2016-11-17T19:01:05Z

doc/book/reference/migration/to-v1.1.md

+- `delete($path, $middleware, $name = null)`
+- `route($path, $middleware, array $methods = null, $name = null)`
+
+Each returns a `Zend\Expressive\Router\Route` instance; this is useful if you


Thanx for mentioning this. No need anymore for routes via config.

This has actually been true all along... just not well documented.

geerteltink · 2016-11-17T19:18:59Z

doc/book/reference/migration/to-v1.1.md

+- Decorate the middleware in a `Zend\Stratigility\Middleware\CallableMiddlewareWrapper`
+  instance (which also requires a `$responsePrototype`).
+
+We recommend that you begin writing middleware to follow the http-interop


Maybe add an example here to clarify it a bit. I think it's something like this? (Disclaimer: from memory and untested)

<?php namespace App\Middleware; use Psr\Http\Message\ResponseInterface; use Psr\Http\Message\ServerRequestInterface; use Interop\Http\Middleware\ServerMiddlewareInterface; use Interop\Http\Middleware\DelegateInterface; class MyMiddleware implements ServerMiddlewareInterface { public function __construct() { } /** * @param Request $request * @param DelegateInterface $delegate * * @return Response */ public function process(ServerRequestInterface $request, DelegateInterface $delegate): ResponseInterface { // Do something (with the request) first // Call the next middleware and wait for the response $response = $delegate->process($request); // Do something (with the response) before returning the response // Return the response return $response; } }

Excellent feedback! I'll incorporate that shortly!

harikt · 2016-11-18T05:13:38Z

doc/book/reference/migration/to-v1.1.md

+  middleware_ using the caught exception as the `$err` argument.
+
+- The "Final Handler". This is a special middleware type with the signature
+  `function ServerRequestInterface $request, ResponseInterface $response, $err = null)`,


Probably missed a (

glen-84 · 2016-11-18T08:01:32Z

doc/book/reference/migration/to-v1.1.md

+
+  ```php
+  include 'config/pipeline.php';
+  include 'config/routes.php';


Shouldn't these use require?

Yeah, probably. Thanks for the suggestion; will implement shortly.

michalbundyra · 2016-11-18T11:22:52Z

test/ApplicationConfigInjectionTest.php

+namespace ZendTest\Expressive;
+
+use Closure;
+use InvalidArgumentException;


It is never used.

michalbundyra · 2016-11-18T11:23:23Z

src/ApplicationConfigInjectionTrait.php

+
+namespace Zend\Expressive;
+
+use ReflectionProperty;


It is never used.

michalbundyra · 2016-11-18T11:25:15Z

src/Container/ApplicationFactory.php

 use Zend\Diactoros\Response\EmitterInterface;
 use Zend\Expressive\Application;
+use Zend\Expressive\ApplicationUtils;
 use Zend\Expressive\Exception;


It is no longer used here, can be removed.

michalbundyra · 2016-11-18T11:26:17Z

test/Container/ErrorResponseGeneratorFactoryTest.php

+use Interop\Container\ContainerInterface;
+use PHPUnit_Framework_TestCase as TestCase;
+use Zend\Expressive\Container\ErrorResponseGeneratorFactory;
+use Zend\Expressive\Middleware\ErrorResponseGenerator;


It is never used.

It is - but using the ::class notation, for assertInstanceOf checks.

michalbundyra · 2016-11-18T11:27:56Z

test/Command/GenerateProgrammaticPipelineFromConfig/GeneratorTest.php

+use org\bovigo\vfs\vfsStream;
+use org\bovigo\vfs\vfsStreamDirectory;
+use PHPUnit_Framework_TestCase as TestCase;
+use Prophecy\Argument;


Unused, please remove.

michalbundyra · 2016-11-18T11:38:12Z

src/Container/ApplicationFactory.php

 use Zend\Diactoros\Response\EmitterInterface;
 use Zend\Expressive\Application;
+use Zend\Expressive\ApplicationUtils;


I can't see this class in the library. Where is it?

michalbundyra · 2016-11-18T11:38:44Z

src/Container/ApplicationFactory.php

 * - 'Zend\Diactoros\Response\EmitterInterface'. If missing, an EmitterStack is
 *   created, adding a SapiEmitter to the bottom of the stack.
 * - 'config' (an array or ArrayAccess object). If present, and it contains route
 *   definitions, these will be used to seed routes in the Application instance
 *   before returning it.
 *
- * When introspecting the `config` service, the following structure can be used
- * to define routes:
+ * Please see the `Zend\Expressive\ApplicationUtils` class for details on how


As above, I can't see this class in the library. Am I missing something?

michalbundyra · 2016-11-18T11:40:03Z

src/Command/GenerateProgrammaticPipelineFromConfig/Generator.php

+     */
+    private function createOptionValue($value, $indentLevel = 1)
+    {
+        if (is_array($value) || $value instanceof Traversable) {


Traversable should be imported or \Traversable here.

michalbundyra · 2016-11-18T11:40:52Z

src/Middleware/WhoopsErrorResponseGenerator.php

+class WhoopsErrorResponseGenerator
+{
+    /**
+     * @var Whoops


Seems to be wrong type here.

michalbundyra · 2016-11-18T11:54:45Z

src/Command/GenerateProgrammaticPipelineFromConfig/Generator.php

+     * @param int $indentLevel
+     * @return string
+     */
+    private function formatOptions($options, $indentLevel = 1)


Is $options arg always an array? Can we add type hint here?

It could be an ArrayObject.

So then PHPDoc should be updated.

michalbundyra · 2016-11-18T12:07:59Z

doc/book/reference/migration/to-v1.1.md

+## http-interop
+
+Stratigility 1.3 provides the ability to work with [http-interop middleware
+0.2.0](https://github.com/http-interop/http-middleware/tree/ff545c87e97bf4d88f0cb7eb3e89f99aaa53d7a9).


Maybe better here link to tagged version:
https://github.com/http-interop/http-middleware/tree/0.2.0

michalbundyra · 2016-11-18T12:16:29Z

In Zend\Expressive\Application there are some methods and properties marked as:

@deprecated This property will be removed in v1.1.

and also:

@todo For 1.1, remove the RouteResultSubjectInterface implementation, and
      all deprecated properties and methods.

Is it gonna be done in this PR, or PR #376 will be merged before release 1.1?

michalbundyra · 2016-11-18T12:58:41Z

src/Command/GenerateProgrammaticPipelineFromConfig/Generator.php

+<?php
+/**
+ * Expressive middleware pipeline
+ */


Maybe will be nice to add here also:
@var \Zend\Expressive\Application $app
because then, in this file $app variable is gonna be used.
The same about TEMPLATE_ROUTES.

michalbundyra · 2016-11-18T13:04:29Z

Migration script:
I would expect that after using migration script my application will work straight away, but I've got error:

Fatal error: Uncaught exception 'Zend\Expressive\Exception\InvalidMiddlewareException' with message 'Unable to resolve middleware "string[Scalar]" to a callable' in my-app\vendor\zendframework\zend-expressive\src\MarshalMiddlewareTrait.php on line 65

This is because migration script is not adding:

    'dependencies' => [
        'invokables' => [
            OriginalMessages::class => OriginalMessages::class,

Into configuration, but:

$app->pipe(\Zend\Stratigility\Middleware\OriginalMessages::class);

is added into config/pipeline.php.

I think it should be done automatically (in migration script) or it should be noted in migration notes, that this action is required to be done manually.

michalbundyra · 2016-11-18T14:34:40Z

One more issue. This was my config based configuration - config/autoload/middleware-pipeline.global.php:

[...]
    'middleware_pipeline' => 
        'my-path' => [
            'path' => '/my-path',
            'middleware' => [
                ApplicationFactory::ROUTING_MIDDLEWARE,
                \App\Middleware\MyMiddleware::class,
                \App\Middleware\OtherMiddleware::class,
            ],
        ],

it was successfully converted to middleware pipeline (via migration script) - config/pipeline.php:

[...]
$app->pipe('/my-path', [
    \Zend\Expressive\Application::ROUTING_MIDDLEWARE,
    \App\Middleware\MyMiddleware::class,
    \App\Middleware\OtherMiddleware::class,
]);
$app->pipeRoutingMiddleware();
[...]

And then piping is failing, because pipe method from Application call prepareMiddleware method from Zend\Expressive\MarshalMiddlewareTrait with $middleware = "EXPRESSIVE_ROUTING_MIDDLEWARE", and we got fatal:

Fatal error: Uncaught exception 'Zend\Expressive\Exception\InvalidMiddlewareException' with message 'Unable to resolve middleware "string[Scalar]" to a callable' in my-app\vendor\zendframework\zend-expressive\src\MarshalMiddlewareTrait.php on line 65

Am I doing something wrong or there are missing some scenarios, when we have some routing/dispatch middlewares only for some paths?

Update:

Ok, I think I know. So - config/pipeline.php should be:

[...]
$app->pipeRoutingMiddleware();
$app->pipe('/my-path', [
    \App\Middleware\MyMiddleware::class,
    \App\Middleware\OtherMiddleware::class,
]);
[...]

The question is now: is my previous middleware-pipeline configuration wrong or the migration script is doing it wrongly?

michalbundyra · 2016-11-18T15:34:11Z

Another issue. "Method not allowed". I have routing, let say:

$app->post('/foo', \App\Action\Foo::class, 'foo');

So only POST request are allowed. If I try to do GET /foo I'm getting 200 response with no body.
I should get 405.

I think the problem is in \Zend\Expressive\Application::routeMiddleware:452 where we have:

        $result = $this->router->match($request);

        if ($result->isFailure()) {
            if ($result->isMethodFailure()) {
                $response = $response->withStatus(405)
                    ->withHeader('Allow', implode(',', $result->getAllowedMethods()));
                return $next($request, $response, 405); // <-------
            }
[...]

and then, the 3rd argument (405) is passed through \Zend\Stratigility\Next:119 where $err = 405. Error is not thrown (because $err is 405 not an instance of \Exception|\Throwable).
I can write here more, but I think it should be enough to investigate and fix this issue.

weierophinney · 2016-11-18T17:22:48Z

Is it gonna be done in this PR, or PR #376 will be merged before release 1.1?

I'll be merging #376 before release, which covers these.

weierophinney · 2016-11-18T17:35:13Z

Another issue. "Method not allowed". I

Should be resolved now; thanks for finding the issue!

weierophinney · 2016-11-18T17:40:22Z

And then piping is failing, because pipe method from Application call prepareMiddleware method from Zend\Expressive\MarshalMiddlewareTrait with $middleware = "EXPRESSIVE_ROUTING_MIDDLEWARE", and we got fatal

I think this is an edge case we never tested.

When you use configuration-based pipelines, as it loops through nested pipelines, if it encounters either of those special constants, it replaces the middleware with the appropriate callable ([$this, 'routeMiddleware] and [$this, 'dispatchMiddleware]).

However, in the MarshalMiddlewareTrait, we do not.

I'm going to see if I can create a test case and resolve this; I think it should be relatively easy to accomplish.

danizord

Hey @weierophinney, thanks for this awesome work! This will definitely make the pipeline way more explicit and clear :)

danizord · 2016-11-18T19:11:20Z

src/Container/ErrorHandlerFactory.php

+            ? $container->get(ErrorResponseGenerator::class)
+            : null;
+
+        return new ErrorHandler(new Response(), $generator);


@weierophinney any chance to make it possible to configure ErrorHandler listeners via config? Or should I override this factory to attach my custom listeners?

Use a delegator factory. 😉

geerteltink · 2016-11-18T20:50:17Z

@weierophinney I only see the config setting for the new error handling in the migration guide:
'zend-expressive' => ['raise_throwables' => true,],

Did you forgot $app->raiseThrowables(); or is the config setting preferred?

EDIT: Thinking about this, I think the config option would be preferred so an upgrade to v2.0 might go smoother since $app->raiseThrowables(); doesn't need to be removed.

geerteltink · 2016-11-19T17:41:20Z

doc/book/reference/migration/to-v1.1.md

+
+Stratigility 1.3 deprecates its internal request and response decorators,
+`Zend\Stratigility\Http\Request` and `Zend\Stratigility\Http\Response`,
+respsectively. The main utility of these instances was to provide access in


Typo: respectively

Fixed, thanks!

geerteltink · 2016-11-20T13:42:39Z

Also phpunit can be upgraded to 5.x since PHP 5.5 support is removed.

geerteltink · 2016-11-20T14:42:12Z

doc/book/reference/migration/to-v1.1.md

+programmatic/declarative statements. Specifically:
+
+- We recommend putting the pipeline declarations into `config/pipeline.php`.
+- We recommend putting the pipeline declarations into `config/routes.php`.


We recommend putting the pipeline declarations into `config/pipeline.php`. We recommend putting the routing declarations into `config/routes.php`.

Fixed, thanks!

weierophinney · 2016-11-20T17:28:07Z

Did you forgot $app->raiseThrowables(); or is the config setting preferred?

You can do either; I didn't document the method, as technically it's part of the Stratigility 1.3 API, and documented there.

weierophinney · 2016-11-20T18:15:40Z

Also phpunit can be upgraded to 5.x since PHP 5.5 support is removed.

Done in latest commit; thanks!

- per @webimpress

This patch adds `bin/expressive-tooling`, which will help you install and remove the zendframework/zend-expressive-tooling package.

- is_executable does not work correctly on Windows; use file_exists. - Grab $argv[1] instead of $argv[2]; default to an empty string.

…ckage Details how to install and/or uninstall the tooling, and the commands it exposes.

@malukenho

Per @malukenho

@malukenho

Per @malukenho

- Adds fig/http-message-util as a dependency - Updates `NotFoundHandler` to use `StatusCodeInterface::STATUS_NOT_FOUND` instead of `404`.

- per @webimpress

- per @xtreamwayz

Close #396

weierophinney mentioned this pull request Nov 11, 2016

Add the new error handler classes #392

Closed

weierophinney added enhancement WIP labels Nov 11, 2016

weierophinney added this to the 1.1.0 milestone Nov 11, 2016

weierophinney changed the title ~~[WIP] Implement programmatic pipeline features~~ Implement programmatic pipeline features Nov 17, 2016

weierophinney removed the WIP label Nov 17, 2016

Koopzington reviewed Nov 17, 2016

View reviewed changes

geerteltink reviewed Nov 17, 2016

View reviewed changes

harikt reviewed Nov 18, 2016

View reviewed changes

glen-84 reviewed Nov 18, 2016

View reviewed changes

michalbundyra reviewed Nov 18, 2016

View reviewed changes

danizord approved these changes Nov 18, 2016

View reviewed changes

weierophinney added the WIP label Nov 18, 2016

geerteltink reviewed Nov 19, 2016

View reviewed changes

geerteltink reviewed Nov 20, 2016

View reviewed changes

geerteltink mentioned this pull request Nov 20, 2016

Upgrade to programmatic pipeline and the new error handler zendframework/zend-expressive-skeleton#119

Merged

10 tasks

weierophinney added 17 commits December 8, 2016 10:40

s/var/param/ in constructor annotations

350568b

- per @webimpress

Added expressive-tooling command

81e7c93

This patch adds `bin/expressive-tooling`, which will help you install and remove the zendframework/zend-expressive-tooling package.

Provide more information on installed tools

998d0ba

Improvements to expressive-tooling script

242a056

- is_executable does not work correctly on Windows; use file_exists. - Grab $argv[1] instead of $argv[2]; default to an empty string.

Use PATH_SEPARATOR constant, instead of OS detection

0bb1b5c

Exit immediately after successful remove operation

7667199

Remove from require-dev (to avoid a prompt)

c093c89

Updated migration tooling to reference new zend-expressive-tooling pa…

68cb2dc

…ckage Details how to install and/or uninstall the tooling, and the commands it exposes.

Removed unnecessary parens from ternary

0553f3f

Per @malukenho

Remove conditional, and return result of comparison directly

deb4b6c

Per @malukenho

Use status code constants instead of integers

79843ff

- Adds fig/http-message-util as a dependency - Updates `NotFoundHandler` to use `StatusCodeInterface::STATUS_NOT_FOUND` instead of `404`.

Add trailing commas to array items

6bba927

- per @webimpress

Removed spaces within array definitions

d659857

- per @webimpress

Re-ordered import statements

e2be004

- per @webimpress

Refactored conditionals in argument constraint

3c405c9

- per @webimpress

whitespace and order of operation updates

8b0f965

- per @webimpress

use Fig\Http\Message\StatusCodeInterface for status codes

af52e19

- per @webimpress

weierophinney force-pushed the feature/programmatic branch from 86f136e to af52e19 Compare December 8, 2016 16:55

weierophinney added 2 commits December 8, 2016 10:58

Added trailing comma

7ecc10b

- per @webimpress

Added composer.cmd to list of binaries to search for

33ff374

- per @xtreamwayz

weierophinney merged commit 33ff374 into zendframework:develop Dec 8, 2016

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

Added CHANGELOG entries for #396

564b277

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

Merge branch 'feature/programmatic' into develop

48bea63

Close #396

weierophinney deleted the feature/programmatic branch January 30, 2017 20:11

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

weierophinney added a commit that referenced this pull request Feb 8, 2017

Added CHANGELOG for #432, #309, #373, #396, and #406

0fb7fe0

michalbundyra mentioned this pull request Feb 9, 2017

1.1 Programmatic middleware pipelines #437

Closed

Implement programmatic pipeline features #396

Implement programmatic pipeline features #396

Conversation

weierophinney commented Nov 11, 2016 • edited Loading

TODO

Tooling support

References

weierophinney commented Nov 17, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

geerteltink 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

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

michalbundyra commented Nov 18, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michalbundyra commented Nov 18, 2016

michalbundyra commented Nov 18, 2016 • edited Loading

michalbundyra commented Nov 18, 2016

weierophinney commented Nov 18, 2016

weierophinney commented Nov 18, 2016

weierophinney commented Nov 18, 2016

danizord left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

geerteltink commented Nov 18, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

geerteltink commented Nov 20, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weierophinney commented Nov 20, 2016

weierophinney commented Nov 20, 2016

weierophinney commented Nov 11, 2016 •

edited

Loading

michalbundyra commented Nov 18, 2016 •

edited

Loading

geerteltink commented Nov 18, 2016 •

edited

Loading