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

Route with optional params throws errors with Zend Router and View #186

Closed
RalfEggert opened this issue Nov 13, 2015 · 19 comments
Closed

Route with optional params throws errors with Zend Router and View #186

RalfEggert opened this issue Nov 13, 2015 · 19 comments
Assignees
Milestone

Comments

@RalfEggert
Copy link
Contributor

I am using Zend\Expressive with the Zend View and the Zend Router and have some problems with an optional route param.

[
    'name' => 'voting',
    'path' => '/voting[/:pos[/:neg]]',
    'middleware' => Application\Action\VotingAction::class,
    'allowed_methods' => ['GET'],
],

The routing works fine. But the usage of the URL view helper does not. It forces me to always path values for both optional params.

<?php echo $this->url('voting', array('pos' => '1', 'neg' => '1')) ?>

But if I pass no values for the optional params, I get a Zend\Mvc\Router\Exception\InvalidArgumentException with message Missing parameter "pos".

Is this a bug or is my route config wrong?

@weierophinney
Copy link
Member

You're accustomed to the ZF2 behavior. 😄

In ZF2, we inject the URL helpers with the RouteMatch, allowing it to use matched values unless overridden. To get similar behavior in Expressive, we'd need to inject helpers (or the TwigExtension, or similar features for other template renderer implementations) with the RouteResult after routing but before executing middleware. This would require likely a subject/observer or event system to accomplish, or the ability to define routed middleware wrappers.

For the record, the UrlHelper provided in zend-expressive-zendviewrenderer does have a setRouteResult() method, which, if you can get access to the route result, will be used in cases where no route name is provided. However, it is not currently used to provide matched parameters, which should be improved. The bigger issue, however, is getting the RouteResult in the first place, as the routing middleware does not push that instance anywhere; it only uses it to determine the middleware to execute, and to inject matched parameters into request attributes.

I think we should likely take the following actions:

  • First, I think we should push the route result into the attributes as well, under a key named for the RouteResult class. This allows access to it from other middleware.
  • Second, we need to update the UrlHelper in the zend-expressive-zendviewrenderer package to merge matched parameters with those passed to the helper.
  • Third, we need a way to wrap routed middleware inside of other middleware easily. This will allow us to do something like wrap a routed middleware with a generic middleware that would grab the route result from the request attributes and inject it into the URL helper. Other use cases would be for doing things such as authorization, validation, and similar workflows that may depend on the results of routing, but can be done generically. (The Apigility workflow is a good example of a use case for this.)

I think the first two can be done during the RC status, and I'll slate the third to a 1.1 release, as it would be a new feature.

Once the first two items are accomplished, you can have your middleware injected with the UrlHelper instance, and then during invocation, inject the UrlHelper with the RouteResult as pulled from the request attributes. This will solve the immediate problem, albeit in a way that couples your middleware to the renderer implementation.

When we accomplish the third item, we can then ship zend-expressive-zendviewrenderer with middleware to accomplish this automatically.

@RalfEggert
Copy link
Contributor Author

You're accustomed to the ZF2 behavior. 😄

Seems that I am getting to be old-school these days. ;-)

Sounds great and I guess I will not be the only one demanding this feature. Until all this is implemented I have just split my route into two different routes. One without the params and one with them. That solved the problem for now. But I am really looking forward for your suggested solution.

@weierophinney
Copy link
Member

@RalfEggert #200 is helping address this, and I'll be following that up with a PR against zendframework/zend-expressive-zendviewrenderer to provide a delegator factory to use with that renderer; the result will be injection of the URL view helper with the calculated RouteResult, which will accomplish the behavior you're looking for.

@weierophinney
Copy link
Member

@RalfEggert
Copy link
Contributor Author

@weierophinney
Looks great. I would like to test it in my application but I am not sure how to setup my composer.json to get your merged changes for expressive and the view renderer.

@weierophinney
Copy link
Member

Wait until RC3 is tagged on both the zend-expressive AND zend-expressive-skeleton repos; that'll be the easiest way to test it out. The skeleton repo will also show you how you can register the delegator factory (assuming you're using zend-servicemanager) to make the behavior automatic.

@RalfEggert
Copy link
Contributor Author

Ok, I will wait then. :-)

@weierophinney
Copy link
Member

@RalfEggert This is now addressed in RC3!

If you already have an application, you'll need to update your dependency versions for all router and template selections to use ^1.0. Additionally, you'll likely want to add zendframework/zend-expressive-helpers at ^1.1 (though zendviewrenderer should pick it up already).

Finally, you'll need to add the following to the config/autoload/dependencies.global.php file:

  • Under invokables: Zend\Expressive\Helper\ServerUrlHelper::class => Zend\Expressive\Helper\ServerUrlHelper::class
  • Under factories: Zend\Expressive\Helper\UrlHelper::class => Zend\Expressive\Helper\UrlHelperFactory::class

In config/autoload/middleware-pipeline.global.php:

  • Add the following dependencies section:
    php 'dependencies' => [ 'factories' => [ Zend\Expressive\Helper\ServerUrlMiddleware::class => Zend\Expressive\Helper\ServerUrlMiddlewareFactory::class, ], ],
  • Add the following inside the middleware_pipeline.pre_routing section: [ 'middleware' => Zend\Expressive\Helper\ServerUrlMiddleware::class ],

At that point, everything should work together. zendviewrenderer wraps the UrlHelper and ServerUrlHelper to create zend-view helpers for the serverUrl and url helpers, respectively. These are then seeded correctly based on the request and the matched route, respectively.

@RalfEggert
Copy link
Contributor Author

@weierophinney

Ok, my composer.json looks like this now.

    "require": {
        "zendframework/zend-expressive": "~1.0.0@rc || ^1.0",
        "zendframework/zend-expressive-zendrouter": "^1.0",
        "zendframework/zend-expressive-zendviewrenderer": "^1.0",
        "zendframework/zend-expressive-helpers": "^1.1",
        "zendframework/zend-servicemanager": "^2.5",
        "zendframework/zend-stdlib": "~2.7",
        "ocramius/proxy-manager": "^1.0",
        "roave/security-advisories": "dev-master"
    },

I added the configs as you suggested. But working with optional route params like described in the original post still does not work. This means, I still get an Zend\Mvc\Router\Exception\InvalidArgumentException.

What did I miss?

@RalfEggert
Copy link
Contributor Author

@weierophinney

Maybe my routing definition is wrong?

#186 (comment)

@weierophinney
Copy link
Member

I've tracked it down; it's an order of operations issue.

The ZendViewRendererFactory creates a closure factory for creating its url helper, and that closure pulls the Zend\Expressive\Helper\UrlHelper service from the container to seed the Zend\Expressive\ZendView\UrlHelper instance to return.

The problem is that this is the first that the Zend\Expressive\Helper\UrlHelper is pulled, and, because it happens when pulling the template renderer, which happens when pulling the route middleware, it happens after the Application instance triggers the route result observers. In other words, it's never notified!

Interestingly, I ran into a related, but different, situation yesterday when using my phly-expressive-mustache package: in that case, I'm pulling the UrlHelper when creating the MustacheTemplate instance. The problem that occurs there is that this happens when creating the TemplatedFinalHandler, which occurs when creating the Application… but the UrlHelper pulls the Application in order to register with it, which leads to a circular dependency issue.

I think I'm going to need to alter this, and have the UrlHelper either pulled in the ApplicationFactory, or seeded via middleware (à la the ServerUrlMiddleware). The first would look something like this:

namespace Zend\Expressive\Container;

use Interop\Container\ContainerInterface;
use Zend\Expressive\Application;
use Zend\Expressive\Helper\UrlHelper;

class ApplicationFactory
{
    function __invoke(ContainerInterface $container)
    {
        /* create the application instance, inject routes, etc. */

        if ($container->has(UrlHelper::class)) {
            $application->attachRouteResultObserver($container->get(UrlHelper::class));
        }
        return $application;
}

The second would look like this:

namespace Zend\Expressive\Helper;

use Zend\Expressive\Router\RouteResultSubjectInterface;

class UrlHelperMiddleware
{
    private $application;
    private $helper;

    public function __construct(UrlHelper $helper, RouteResultSubjectInterface $application)
    {
        $this->helper = $helper;
        $this->application = $application;
    }

    public function __invoke($request, $response, callable $next)
    {
        $this->application->attachRouteResultObserver($this->helper);
        return $next($request, $response);
    }
}

A third approach is to use delegator factories (if using the service manager), or Pimple's extend() feature. However, since I don't know of a way to accomplish this with Aura.Di, nor do I know if other containers implement such functionality, I'm leaning towards one of the above.

In both cases, the UrlHelper factory would be changed to only create and return the UrlHelper, not attach it to the Application instance.

The first approach would be more turn-key; however, it also depends on you having the zend-expressive-helpers package installed. I'm not sure how I feel about specifying a service that the package does not require. Additionally, it opens the gates for having another configuration feature (route result observers), and that's another vector to maintain.

The second approach would work regardless of middleware library, so long as you're also consuming zend-expressive-router. However, it requires a bit more configuration. That said, we could do this by default in the skeleton, just like we do with the ServerUrl. Because it's configurable, you can also remove it if desired.

I'll get something in place for an RC4 in the next 1–2 days. Thanks for challenging me on this, @RalfEggert !

@weierophinney weierophinney reopened this Dec 8, 2015
@RalfEggert
Copy link
Contributor Author

Thanks for the explanation. I don't mind if the solution works kind of out of the box or is rather an configuration issue. If it needs to be installed via the skeleton, than its OK to me. Most important is that it works for beginners with the 1.0.0 release.

@weierophinney
Copy link
Member

@RalfEggert Yes, the idea is to ensure this works out-of-the-box. If you could, please take a look at zendframework/zend-expressive-helpers#2, as it implements the middleware-based solution.

Once I've merged that I'll also update the skeleton, and then the docs for zend-expressive.

@weierophinney
Copy link
Member

@RalfEggert I've merged the changes in zend-expressive-helpers and released v1.2.0 of that package. I've also now updated zend-expressive-skeleton to use that version, and to add configuration for the new UrlHelperMiddleware (which you can see in zendframework/zend-expressive-skeleton#33). Finally, I updated the docs in #218.

To test it locally, I did the following:

Created a new application based on dev-master of the skeleton

$ composer create-project zendframework/zend-expressive-skeleton expressive "~1.0.0-dev@dev"

I selected:

  • The zend-mvc router
  • zend-servicemanager
  • The zend-view renderer
  • whoops for error reporting

Manually created the route reported

In public/index.php, above the $app->run(); statement, I added the following:

$app->get('/voting[/:pos[/:neg]]', function ($req, $res, $next) use ($container) {
    $renderer = $container->get(TemplateRendererInterface::class);
    $r = new ReflectionProperty($renderer, 'renderer');
    $r->setAccessible(true);
    $phpRenderer = $r->getValue($renderer);

    $res->write(sprintf("url(): %s<br />\n", $phpRenderer->url()));
    $res->write(sprintf("url('voting'): %s<br />\n", $phpRenderer->url('voting')));
    $res->write(sprintf("url('voting', ['pos' => 10000]): %s<br />\n", $phpRenderer->url('voting', ['pos' => 10000])));
    $res->write(sprintf("url('voting', ['neg' => 10000]): %s<br />\n", $phpRenderer->url('voting', ['neg' => 10000])));
    return $res;
}, 'voting');

Started a web server

I used the built-in web server:

$ php -S 0.0.0.0:8080 -t public/

Navigated to the new route

I navigated to http://localhost:8080/voting/1/2, where I received the following results:

url(): /voting/1/2
url('voting'): /voting/1/2
url('voting', ['pos' => 10000]): /voting/10000/2
url('voting', ['neg' => 10000]): /voting/1/10000

So, I can verify it's now working correctly!

@weierophinney
Copy link
Member

Just verified the changes solved my issues in phly-expressive-mustache as well! 😄

@RalfEggert
Copy link
Contributor Author

Checked it and it works as expected! Nice work!

@pine3ree
Copy link
Contributor

Hello @RalfEggert , @weierophinney .

I had similar doubts with the new url helpers

Let's assume a named route 'voting' as implemented in your prevous examples.

What if I wish that

$urlHelper->generate('voting', /*[]*/);

would provide just

'/voting'

I tried the new url helpers with the following route segments alternately

'hello[/:name]';   /*(1)*/
'hello[/:[name]]'; /*(2)*/

with a simple route defined as

//...
[
    'name' => 'hello',
    'path' => '/hello[/:name]'/*(1)*/,/*'hello[/:[name]]*/ /*(2)*/
    'middleware' => App\Action\HelloAction::class,
    'allowed_methods' => ['GET'],
],

In both cases calling generate() without a 'name' parameter results a zend mvc exception
Zend \ Mvc \ Router \ Exception \ InvalidArgumentException - Missing parameter "name"

If i use ['name' => null] i get the same exception (parameter check is using isset)
If i use ['name' => ''] i get the following URIs

'/hello/'; // with route 1
'/hello';  // with route 2

i get the same results setting the 'name' parameter using

//...
[
    'name' => 'hello',
    'path' => '/hello[/[:name]]',
    'middleware' => App\Action\HelloAction::class,
    'allowed_methods' => ['GET'],
    'options' => [
        'defaults' => [
            'name' => '',
        ]
    ]
],
//...

Wouldn't it be better to allow null|unset parameters for optional segments?

kind regards.

@weierophinney
Copy link
Member

@pine3ree The behavior you're seeing is not due to the zend-expressive implementation, but rather how the zend-mvc router works with regards to generating URIs. As such, raise an issue in zendframework/zend-mvc.

@pine3ree
Copy link
Contributor

@weierophinney
You are totally right! Sorry i posted it here, also with me being already aware it was related to zend-mvc router. I will find some time to look deeper into zend-mvc ad maybe hint a solution in zend-mvc repo. I haven't apply the latest component updates to my zf2 projects, so I cannot say yet if it always behaved like this.
Thanks.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants