Added zend-expressive support - fixes #56 #61

froschdesign · 2017-03-10T17:59:35Z

This proposal does not change existing classes of the component, because I wanted an implementation for version 2 of zend-navigation.

Caution

This PR based on PHPUnit 5.7 || 6.0! See: #59

ping @weierophinney @RalfEggert @webimpress

froschdesign · 2017-03-10T18:03:09Z

Usage for a single container

dependencies.global.php

'dependencies' => [
    'factories'  => [
        Zend\Navigation\Navigation::class =>
            Zend\Navigation\Service\ExpressiveNavigationFactory::class,
    ],
    'delegators' => [
        Zend\View\HelperPluginManager::class => [
            Zend\Navigation\View\ViewHelperManagerDelegatorFactory::class,
        ],
    ],
    'factories' => [
        Zend\Navigation\Middleware\NavigationMiddleware::class =>
            Zend\Navigation\Middleware\NavigationMiddlewareFactory::class,
    ],
],

pipeline.php

$app->pipeRoutingMiddleware();
// …
$app->pipe(Zend\Navigation\Middleware\NavigationMiddleware::class);
// …
$app->pipeDispatchMiddleware();

Usage for multiple containers

dependencies.global.php

'dependencies' => [
    'abstract_factories' => [
        Zend\Navigation\Service\ExpressiveNavigationAbstractServiceFactory::class,
    ],
],

Notice

Do not use the ConfigProvider class, because at the moment the ConfigProvider only includes the configuration for zend-mvc-based applications!

michalbundyra · 2017-03-10T18:24:27Z

composer.json

-        "zendframework/zend-log": "^2.7.1",
-        "zendframework/zend-mvc": "^2.7.9 || ^3.0",
-        "zendframework/zend-router": "^3.0",
+        "zendframework/zend-config": "^3.1",


Shouldn't we have here also 2.6 version?

Right, but this is related to #59. I will this fixed there.

froschdesign · 2017-03-10T18:28:47Z

@webimpress
Can you look at the Middleware? Thanks!

michalbundyra · 2017-03-10T18:28:37Z

src/Middleware/NavigationMiddleware.php

@@ -0,0 +1,69 @@
+<?php
+


Missing license header

michalbundyra · 2017-03-10T18:29:12Z

src/Middleware/NavigationMiddleware.php

+use Zend\Navigation\AbstractContainer;
+use Zend\Navigation\Exception;
+use Zend\Navigation\Page\ExpressivePage;
+use Zend\Expressive\Router\RouteResult;


Please use alphabetical order of use statements.

michalbundyra · 2017-03-10T18:29:54Z

src/Middleware/NavigationMiddleware.php

+            if (! $container instanceof AbstractContainer) {
+                throw new Exception\InvalidArgumentException(
+                    'Invalid argument: container must be an instance of '
+                    . 'Zend\Navigation\AbstractContainer'


I'd use ::class here and sprintf.

michalbundyra · 2017-03-10T18:30:21Z

src/Middleware/NavigationMiddlewareFactory.php

@@ -0,0 +1,83 @@
+<?php
+


Missing license header.

michalbundyra · 2017-03-10T18:33:14Z

src/Middleware/NavigationMiddlewareFactory.php

+    /**
+     * Top-level configuration key indicating navigation configuration
+     *
+     * @var string


Shouldn't we use @const instead of @var for constants?

phpDocumentor

@const is not documented and supported.

PHP-Fig

You may use the @var tag to document the "Type" of the following "Structural Elements":

Constants, both class and global scope

Properties

Variables, both global and local scope

https://github.com/php-fig/fig-standards/blob/master/proposed/phpdoc.md#722-var

Ok fine. Please just note it is proposed standard, not yet accepted. It's a bit odd that constant has annotation variable, isn't it?

Please just note it is proposed standard, not yet accepted.

Right, therefore the hint to phpDocumentor. 😉
As far as I know, APiGen does the same like phpDocumentor.

It's a bit odd that constant has annotation variable, isn't it?

I do not know where comes from. But, if it is not supported, why should we use that? And if we follow the PSR-5 proposal, than the "type" of the value should be described, not the element itself.

@webimpress It's been this way forever in both phpDocumentor (which predates PSR-5 by 15 years; PSR-5 is largely an attempt to codify what phpDocumentor has been doing all along) and in IDE support; it's the only supported, recognized way of annotating constants. No tools recognize @const, unfortunately.

michalbundyra · 2017-03-10T19:05:39Z

test/Page/ExpressivePageTest.php

+    public function testGetHref()
+    {
+        $page = new ExpressivePage(
+            [


We can move it to line above, and then content can have one less indent and closing bracket could be with );

I can't find any documentation for this case.

PHPStorm does the same formatting. 😉

michalbundyra · 2017-03-10T19:06:26Z

test/Page/PageFactoryTest.php

+
+        AbstractPage::factory([
+            'type' => 'ZendTest\Navigation\TestAsset\InvalidPage',
+            'label' => 'My Invalid Page'


Please add trailing comma.

This is related to #59

michalbundyra · 2017-03-10T19:06:47Z

test/Page/PageFactoryTest.php

+        );
+
+        AbstractPage::factory([
+            'label' => 'My Invalid Page'


Please add trailing comma.

This is related to #59

michalbundyra · 2017-03-10T19:07:04Z

test/Service/ExpressiveNavigationAbstractServiceFactoryTest.php

@@ -0,0 +1,89 @@
+<?php
+


Missing license header.

michalbundyra · 2017-03-10T19:07:32Z

test/Service/ExpressiveNavigationFactoryTest.php

@@ -0,0 +1,106 @@
+<?php
+


Missing license header.

michalbundyra · 2017-03-13T21:44:54Z

src/Middleware/NavigationMiddleware.php

+<?php
+/**
+ * @see       https://github.com/zendframework/zend-navigation for the canonical source repository
+ * @copyright Copyright (c) 2016-2017 Zend Technologies USA Inc. (http://www.zend.com)


As it it new file, we should have here just 2017 year.

michalbundyra · 2017-03-13T21:45:20Z

src/Middleware/NavigationMiddleware.php

+use Interop\Http\ServerMiddleware\DelegateInterface;
+use Interop\Http\ServerMiddleware\MiddlewareInterface;
+use RecursiveIteratorIterator;
+use Psr\Http\Message\ServerRequestInterface;


Please use alphabetical order here.

michalbundyra · 2017-03-13T21:46:01Z

src/Middleware/NavigationMiddleware.php

+    /**
+     * @var AbstractContainer[]
+     */
+    private $containers;


Should we have here default value []?

michalbundyra · 2017-03-13T21:46:57Z

src/Middleware/NavigationMiddleware.php

+     * @param ServerRequestInterface $request
+     * @param DelegateInterface      $delegate
+     * @return \Psr\Http\Message\ResponseInterface
+     */


Can we use here @inheritDoc instead?

michalbundyra · 2017-03-13T21:50:03Z

test/Page/ExpressivePageTest.php

+
+    public function testGetHrefSetsHrefCache()
+    {
+


Please remove above empty line.

michalbundyra · 2017-03-13T21:57:00Z

src/Middleware/NavigationMiddleware.php

+    }
+
+    /**
+     * @inheritdoc


Please fix camel case, it should be @inheritDoc.

michalbundyra · 2017-03-13T21:59:43Z

test/Middleware/NavigationMiddlewareFactoryTest.php

@@ -0,0 +1,128 @@
+<?php
+


Still can't see it.

michalbundyra · 2017-03-13T21:59:59Z

test/Middleware/NavigationMiddlewareTest.php

@@ -0,0 +1,65 @@
+<?php
+


Missing license header in the file.

michalbundyra · 2017-03-13T22:00:22Z

test/Page/ExpressivePageTest.php

+<?php
+/**
+ * @see       https://github.com/zendframework/zend-navigation for the canonical source repository
+ * @copyright Copyright (c) 2016-2017 Zend Technologies USA Inc. (http://www.zend.com)


As it is new file, please update year to 2017 only.

froschdesign · 2017-03-15T20:26:32Z

ping @weierophinney

froschdesign · 2017-03-23T07:53:55Z

@weierophinney @RalfEggert

My suggestion for a release:

zend-navigation 2.9
zend-expressive-navigation 1.0

Later than:

zend-navigation 3.0
zend-expressive-navigation 2.0
zend-mvc-navigation 1.0

RalfEggert · 2017-03-23T08:45:55Z

👍

weierophinney · 2017-03-23T19:07:51Z

We can even split out zend-mvc-navigation early, and have it include the Page\Mvc class and all zend-view helpers; if it is registered later than zend-navigation and zend-view in the list of modules, then its own configuration takes precedence.

This would then also allow a 2.X version of zend-navigation to require that particular package in order to keep BC. v3 would remove it as a requirement altogether, and instead make it a suggestion. Similarly, a new 2.X version of zend-view could do the same to ensure we have the navigation helpers available, and v3 would make the package a suggestion instead of a requirement.

weierophinney · 2017-03-23T19:08:30Z

@froschdesign If you want to create a new repo extracting the new functionality, I can review and fork it into the zendframework organization when it's ready, and add it to packagist at that time.

froschdesign · 2017-03-23T21:39:32Z

@weierophinney

We can even split out zend-mvc-navigation early, and have it include the Page\Mvc class and all zend-view helpers

The view helpers in zend-mvc-navigation?!

weierophinney · 2017-03-23T22:55:08Z

The view helpers in zend-mvc-navigation?!

Indeed! It's where they really belong. 😀

froschdesign · 2017-03-23T22:59:32Z

Indeed! It's where they really belong.

And if I will use zend-expressive, then I must also install Page\Mvc?

mano87 · 2017-04-19T06:35:32Z

How does it go on?

weierophinney · 2017-04-20T16:05:08Z

@froschdesign — sorry for the late response:

And if I will use zend-expressive, then I must also install Page\Mvc?

For now, yes, to retain backwards compatibility. However, for a next major version of this component, we would not need to install the zend-mvc integration package.

weierophinney · 2017-04-20T16:05:47Z

@froschdesign I recall you saying you had done the work of separating at least the Expressive support, but I cannot recall where; could you direct me there?

froschdesign · 2017-04-28T20:17:40Z

@weierophinney
Maybe I misunderstood something, but I think these statements collide:

The view helpers in zend-mvc-navigation?!

Indeed! It's where they really belong.

vs.

However, for a next major version of this component, we would not need to install the zend-mvc integration package.

froschdesign · 2017-06-15T09:01:12Z

ping @weierophinney

Can we clarify the problem / question from my last comment? Thanks!

weierophinney · 2017-07-20T15:43:31Z

@froschdesign —

My idea is this:

Create a new package, zend-mvc-navigation that depends on zend-navigation, and contains the Zend\Navigation\Page\Mvc class and Zend\Navigation\View subnamespace. Mark it as conflicting with the zend-navigation v2 series (via the composer.json conflicts configuration section).
Create a new package, zend-expressive-navigation that depends on zend-navigation with the various integrations for working in zend-expressive. Mark this as working with either the v2 or v3 versions of zend-navigation ("zendframework/zend-navigation": "^2.9 || ^3.0").
Create a new minor version (2.9) of zend-navigation that marks the various zend-mvc integrations as deprecated.
Create a new major version (3.0) of zend-navigation that removes the zend-mvc integrations (and also bumps the PHP verson to 7.1, removes HHVM support, etc.). This package would suggest the other two packages.

Make sense?

froschdesign · 2017-07-20T16:04:07Z

@weierophinney

Make sense?

Sorry, not really. If I understand correctly and we will create all these packages, for a zend-expressive application I must install zend-expressive-navigation and zend-mvc-navigation?! (zend-mvc-navigation contains the view helpers a.k.a. Zend\Navigation\View)

weierophinney · 2017-07-20T16:41:00Z

Sorry, not really. If I understand correctly and we will create all these packages, for a zend-expressive application I must install zend-expressive-navigation and zend-mvc-navigation?! (zend-mvc-navigation contains the view helpers a.k.a. Zend\Navigation\View)

Okay, that means three packages, then:

zend-mvc-navigation for the Mvc page type
zend-view-navigation for the view helpers
zend-expressive-navigation for the Expressive page type(s)

Expressive users would install zend-navigation and zend-expressive-navigation, and, optionally, zend-view-navigation (depending on their template renderer). zend-mvc users would install zend-navigation, zend-mvc-navigation, and zend-view-navigation.

mano87 · 2017-12-20T07:39:04Z

How does it go on here? We would like to use this feature ...

froschdesign · 2017-12-20T10:14:05Z

@mano87

How does it go on here?

Still work in progress. (I have to solve another problem before that.)

We would like to use this feature ...

You can already use this branch: https://github.com/froschdesign/zend-navigation/tree/feature/expressive-support

@ALL
Thanks for your patience!

throny · 2018-02-28T08:24:08Z

@froschdesign
Thank you for your commitment so far!
Is a new zend-navigation release with expressive (3) support planned yet?

froschdesign · 2018-02-28T08:29:51Z

@throny

Is a new zend-navigation release with expressive (3) support planned yet?

I hope, but at the moment the priority for the documentation is higher. (see: zendframework/zf-mkdoc-theme#34 – this will help all components!)

lowtower · 2018-08-02T08:27:19Z

Hello,

a long time has gone without any (visible) progress!
Is there any chance to get this working and released anytime?

Cheers

froschdesign · 2018-08-03T09:31:19Z

@lowtower

a long time has gone without any (visible) progress!

This PR will not be the solution. We will create a separate package / repository for the expressive support and another one for zend-view.
Please have a look at: https://github.com/froschdesign/zend-expressive-navigation

Is there any chance to get this working and released anytime?

The current plan includes an alpha version of zend-navigation-zendview, which can be used with zend-expressive-navigation to output all kind of navigations.
When? At the moment I'm still on vacation.

But you can test the current version of zend-expressive-navigation and leave some comments in the issue tracker. Thanks!

RalfEggert · 2018-08-05T12:55:31Z

Added a quick-and-dirty quick start to the new component:

froschdesign/zend-expressive-navigation#2

malukenho · 2018-08-03T07:48:07Z

src/Middleware/NavigationMiddleware.php

+<?php
+/**
+ * @see       https://github.com/zendframework/zend-navigation for the canonical source repository
+ * @copyright Copyright (c) 2017 Zend Technologies USA Inc. (http://www.zend.com)


Shouldn't it be 2018?

No. 😉

See my comment above:

This PR will not be the solution. We will create a separate package / repository for the expressive support and another one for zend-view.
Please have a look at: https://github.com/froschdesign/zend-expressive-navigation

malukenho · 2018-08-03T07:50:57Z

src/Middleware/NavigationMiddlewareFactory.php

+        $containers = [];
+        foreach ($containerNames as $containerName) {
+            $containers[] = $container->get($containerName);
+        }


May it use array_map?

$containers = array_map([$container, 'get'], $containerNames);

froschdesign · 2018-10-09T12:43:19Z

Closing in favour of froschdesign/zend-expressive-navigation (WIP)

froschdesign added awaiting maintainer response enhancement labels Mar 10, 2017

froschdesign requested a review from weierophinney March 10, 2017 17:59

michalbundyra reviewed Mar 10, 2017

View reviewed changes

michalbundyra suggested changes Mar 10, 2017

View reviewed changes

michalbundyra reviewed Mar 13, 2017

View reviewed changes

froschdesign added 16 commits March 22, 2017 15:34

Test for zend-expressive support (without changing existing files)

a7bf8df

Added getter methods to ExpressivePage

027fe3d

Added missing setting for href cache

698c45a

Added unit test for new ExpressivePage class

27a4fed

Middleware can now handle multiple navigation containers

25b2ad9

Reworking the ExpressiveNavigationFactory

c0dc562

Added an abstract factory for mulitple navigations

71fa10c

Added unit tests for service factories

c4106ca

Added unit tests for middleware and factory

fb947b3

Added expressive router and zend-router in composer.json

254cb46

Updated class names in tests

0bf74ad

Added license header to new files

29a1517

Changed alphabetical order of use statements

3980471

Updated exception message creation

d3e1738

Added blank lines

0aa9c0f

Updated indentation in middleware test class

a670d8a

froschdesign mentioned this pull request Jun 14, 2017

Zend\Navigation does not work with Zend\Expressive #56

Open

lowtower mentioned this pull request Jun 15, 2017

Modify quickstart.md froschdesign/zend-navigation#1

Closed

weierophinney added this to In progress in Version 3 Apr 25, 2018

malukenho reviewed Aug 9, 2018

View reviewed changes

froschdesign removed this from In progress in Version 3 Oct 9, 2018

froschdesign closed this Oct 9, 2018

weierophinney mentioned this pull request Dec 31, 2019

Laminas\Navigation does not work with Mezzio laminas/laminas-navigation#2

Open

Added zend-expressive support - fixes #56 #61

Added zend-expressive support - fixes #56 #61

Conversation

froschdesign commented Mar 10, 2017 • edited

Caution

froschdesign commented Mar 10, 2017 • edited

Usage for a single container

Usage for multiple containers

Notice

Choose a reason for hiding this comment

froschdesign Mar 10, 2017 • edited

Choose a reason for hiding this comment

froschdesign commented Mar 10, 2017

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

froschdesign Mar 10, 2017 • edited

Choose a reason for hiding this comment

phpDocumentor

PHP-Fig

Choose a reason for hiding this comment

froschdesign Mar 10, 2017 • edited

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

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

froschdesign commented Mar 15, 2017

froschdesign commented Mar 23, 2017

RalfEggert commented Mar 23, 2017

weierophinney commented Mar 23, 2017

weierophinney commented Mar 23, 2017

froschdesign commented Mar 23, 2017

weierophinney commented Mar 23, 2017

froschdesign commented Mar 23, 2017

mano87 commented Apr 19, 2017

weierophinney commented Apr 20, 2017

weierophinney commented Apr 20, 2017

froschdesign commented Apr 28, 2017

froschdesign commented Jun 15, 2017

weierophinney commented Jul 20, 2017

froschdesign commented Jul 20, 2017

weierophinney commented Jul 20, 2017

mano87 commented Dec 20, 2017

froschdesign commented Dec 20, 2017 • edited

throny commented Feb 28, 2018

froschdesign commented Feb 28, 2018

lowtower commented Aug 2, 2018

froschdesign commented Aug 3, 2018

froschdesign commented Mar 10, 2017 •

edited

froschdesign commented Mar 10, 2017 •

edited

froschdesign Mar 10, 2017 •

edited

froschdesign Mar 10, 2017 •

edited

froschdesign Mar 10, 2017 •

edited

froschdesign commented Dec 20, 2017 •

edited