Upgrade to programmatic pipeline and the new error handler #119

geerteltink · 2016-11-20T15:30:23Z

This PR adds support for zendframework/zend-expressive#396.

TODO after zend-expressive 1.1 is released:

Remove 1.1.x-dev branch from zend-expressive version in composer.json

geerteltink · 2016-11-20T15:40:59Z

@weierophinney I've started with updating the skeleton to zend-expressive 1.3 and programmatic configuration, It seems to work.

- Remove useless tests for PHP lowest dependencies. Composer always creates the project with the latest dependencies, so there is no need to test the lowest dependencies. - Remove deprecated --install option for composer - Rename $CHECK_CS to $EXECUTE_CS_CHECK as in the latest stratigility package

weierophinney

Looks good! A few minor changes requested, mainly around importing classes and/or namespaces into some of the generated files to make them more readable. The bigger question I have is around what prompted the removal of the lowest/locked/latest testing strategy.

weierophinney · 2016-12-12T15:11:06Z

.travis.yml

-        - DEPS=latest
-    - php: 5.6
-      env:
-        - DEPS=lowest


What's prompting removal of the lowest/latest/locked strategy?

If I remember correctly there are dependency issues when the composer.lock file is committed. Or at least there were warnings. That explains the missing locked strategy.

I even have my doubts if "lowest" is useful. When creating a new project always the latest versions are installed.

Seems that I've removed the "lowest". I'll revert that one.

Also do we need hhvm builds? They fail anyway.

weierophinney · 2016-12-12T15:11:52Z

README.md

@@ -79,7 +79,7 @@ scripts** with `--no-scripts`, otherwise it will remove the installer and all
 tests.

 ```bash
-$ composer install --no-scripts
+$ composer update --no-scripts


Why update and not install? Don't we want developers to use the same set of deps we've tested against?

Since there is no composer.lock file it doesn't really matter if install or update is used. Just in case there is a leftover composer.lock file for whatever reason, I prefer update so it does the same in all situations.

EDIT: Thinking about this, we definitely want update so the local composer.lock is ignored and always the latest dependencies are used.

weierophinney · 2016-12-12T20:44:00Z

composer.json

        "roave/security-advisories": "dev-master",
-        "zendframework/zend-expressive": "^1.0",
+        "zendframework/zend-expressive": "^1.1.x-dev || ^1.1",


the 1.1.x-dev constrating should probably read ^1.1.x-dev@dev. Additionally, since || returns the first that matches, switch the order of the two to ensure the stable release is selected once available.

OH!!! I've got Travis powers :D

PHP 7 build was still failing. After removing the cache and restarting the build it was successful.

weierophinney · 2016-12-12T21:31:58Z

config/autoload/dependencies.global.php

+                Zend\Expressive\Container\ErrorResponseGeneratorFactory::class,
+
+            Zend\Expressive\Middleware\NotFoundHandler::class =>
+                Zend\Expressive\Container\NotFoundHandlerFactory::class,


Let's import these classes, or, at the least, the Middleware and Container subnamespaces; that'll make these shorter and more readable.

weierophinney · 2016-12-12T21:32:59Z

config/pipeline.php

+// The handlers in each middleware attached this way will see a URI with that PATH SEGMENT STRIPPED !!!
+// - $app->pipe('/api', $apiMiddleware);
+// - $app->pipe('/docs', $apiDocMiddleware);
+// - $app->pipe('/files', $filesMiddleware);


This bit should be above the pipeRoutingMiddleware() call, as segregated middleware will usually have its own routing.

weierophinney · 2016-12-12T21:34:28Z

src/App/Action/HomePageAction.php

        }

        if (!$this->template) {
            return new JsonResponse([
                'welcome' => 'Congratulations! You have installed the zend-expressive skeleton application.',
-                'docsUrl' => 'zend-expressive.readthedocs.org',
+                'docsUrl' => 'https://docs.zendframework.com/zend-expressive/',


Nice catch on all of these documentation links!

weierophinney · 2016-12-12T21:45:27Z

src/ExpressiveInstaller/Resources/config/routes-minimal-zf2-router.php

-        //     'middleware' => App\Action\HomePageAction::class,
-        //     'allowed_methods' => ['GET'],
-        // ],
-    ],


I find the changes in the various router configuration files interesting, as I noticed earlier when drafting the information on how to handle module-specific routing that the routes do not change between implementations unless there are dynamic segments in play. None of the default routes have those! (In other words: nice work!)

weierophinney · 2016-12-12T21:47:17Z

src/ExpressiveInstaller/Resources/config/routes-minimal.php

+ * $app->post('path', MiddlewareAction::class, 'route.name');
+ * $app->put('path', MiddlewareAction::class, 'route.name');
+ * $app->patch('path', MiddlewareAction::class, 'route.name');
+ * $app->delete('path', MiddlewareAction::class, 'route.name');


I'd likely give some concrete examples here, instead of using the same path, class, and name for each, to make it more clear that each middleware handles a specific HTTP method/path combination.

weierophinney · 2016-12-12T21:47:36Z

src/ExpressiveInstaller/Resources/templates/plates-home-page.phtml

@@ -15,7 +15,7 @@
 <div class="row">
    <div class="col-md-4">
        <h2>
-            <a href="https://zendframework.github.io/zend-expressive/getting-started/features/" target="_blank">
+            <a href="https://docs.zendframework.com/zend-expressive/getting-started/features/" target="_blank">


Thanks for fixing these documentation URIs!

weierophinney · 2016-12-12T22:19:02Z

test/ExpressiveInstallerTest/RoutersTest.php

@@ -78,7 +79,7 @@ public function testRouter(
            $expectedRouter,
            $config['dependencies']['invokables'][Router\RouterInterface::class]
        );
-        $this->assertEquals($expectedRoutes, $config['routes']);
+        //$this->assertEquals($expectedRoutes, $config['routes']);


Why is this commented out, instead of removed outright?

@weierophinney

"since || returns the first that matches, switch the order of the two to ensure the stable release is selected once available" - @weierophinney

@weierophinney

per @weierophinney

Without the `--no-scripts` flag the installer starts and it removes itself and there is nothing left to contribute to.

At least version 2.3.1 is required for `Generic.Arrays.DisallowLongArraySyntax`.

Upgrade to programmatic pipeline and the new error handler

Close #119

weierophinney · 2017-01-09T23:15:03Z

Thanks, @xtreamwayz!

geerteltink added this to the 1.1.0 milestone Nov 20, 2016

geerteltink added enhancement WIP labels Nov 20, 2016

geerteltink mentioned this pull request Nov 28, 2016

Switch question order: Let user choose DI container first. #120

Closed

geerteltink added 12 commits December 9, 2016 20:22

Add programmatic pipeline configuration

79b679a

Remove obsolete comment about deprecated notices

d7f6a58

Happy new year! 🎆

e98f2cd

Update doc links to https://docs.zendframework.com/

bcf038f

Update supported PHP versions (5.6, 7.0, 7.1)

c5be3c3

Update PHPUnit to 5.6+

1b9adab

Add different config/routes.php versions for minimal and full install

87a3139

Update tests

b43fa45

Update links to zend packages docs

aeb7f6d

Add Whoops 2.0 error handler

9471679

Update ErrorHandler tests

2d1a184

geerteltink removed the WIP label Dec 9, 2016

Use zend-expressive 1.1.x-dev for now

4549fe8

weierophinney suggested changes Dec 12, 2016

View reviewed changes

geerteltink added 10 commits December 13, 2016 12:23

Use the stable constrain before the dev constrain

f429dd8

"since || returns the first that matches, switch the order of the two to ensure the stable release is selected once available" - @weierophinney

Import Middleware and Container subnamespaces for readability

5f54f96

per @weierophinney

Bring back the lowest/latest testing strategy

5081bd1

Make sure the --no-scripts flag is used by contributors

19ad2e6

Without the `--no-scripts` flag the installer starts and it removes itself and there is nothing left to contribute to.

Fix ErrorHandler namespace

a9f88d9

Increase PHP CodeSniffer constrain

cd271b2

At least version 2.3.1 is required for `Generic.Arrays.DisallowLongArraySyntax`.

Move route specific piped middleware before the router starts

eae96a4

Use imports for middleware

d64bb3f

Add concrete route examples

8d21c87

Remove obsolete commented out code

e98d7b5

geerteltink added 2 commits December 13, 2016 17:18

Add test for OptionalPackages::copyFile()

9d16c6b

Remove 'mikey179/vfsStream' dev dependency during installation

df7d997

geerteltink self-assigned this Dec 13, 2016

weierophinney approved these changes Jan 9, 2017

View reviewed changes

weierophinney merged commit df7d997 into zendframework:develop Jan 9, 2017

weierophinney added a commit that referenced this pull request Jan 9, 2017

Merge pull request #119 from xtreamwayz/feature/programmatic

205314b

Upgrade to programmatic pipeline and the new error handler

weierophinney added a commit that referenced this pull request Jan 9, 2017

Merge branch 'feature/119' into develop

edb0717

Close #119

weierophinney mentioned this pull request Jan 9, 2017

Lightweight module system with config manager #54

Merged

7 tasks

geerteltink deleted the feature/programmatic branch January 10, 2017 17:59

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade to programmatic pipeline and the new error handler #119

Upgrade to programmatic pipeline and the new error handler #119

geerteltink commented Nov 20, 2016 •

edited

Loading

geerteltink commented Nov 20, 2016

weierophinney left a comment

weierophinney Dec 12, 2016

geerteltink Dec 13, 2016

geerteltink Dec 13, 2016

geerteltink Dec 13, 2016

weierophinney Dec 12, 2016

geerteltink Dec 13, 2016 •

edited

Loading

weierophinney Jan 9, 2017

weierophinney Dec 12, 2016

geerteltink Dec 13, 2016

weierophinney Dec 12, 2016

weierophinney Dec 12, 2016

weierophinney Dec 12, 2016

weierophinney Dec 12, 2016

weierophinney Dec 12, 2016

weierophinney Dec 12, 2016

weierophinney Dec 12, 2016

weierophinney commented Jan 9, 2017

Upgrade to programmatic pipeline and the new error handler #119

Upgrade to programmatic pipeline and the new error handler #119

Conversation

geerteltink commented Nov 20, 2016 • edited Loading

geerteltink commented Nov 20, 2016

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

geerteltink Dec 13, 2016 • edited Loading

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

weierophinney commented Jan 9, 2017

geerteltink commented Nov 20, 2016 •

edited

Loading

geerteltink Dec 13, 2016 •

edited

Loading