zendframework / zend-expressive-fastroute

FastRoute integration for Expressive
Other
31 stars 25 forks source link

Returning empty path with missing mandatory route param #30

Closed Lansoweb closed 7 years ago

Lansoweb commented 7 years ago

Hi! Don't know if I misunderstood but i'm facing a problem with the url view helper.

I have the following route definition:

[
    'name' => 'user',
    'path' => '/user/{id}',
    'middleware' => User\Action\UserAction::class,
    'allowed_methods' => ['GET','POST'],
],

If, in a view, I create a link to it like:

<a href="<?= $this->url('user') ?>">Back</a>

it returns an empty string, and the button just refresh the page. In this line the $path is empty.

Shouldn't the helper maybe throw an exception if a mandatory parameter is missing? It not, how the best approach to prevent this?

I'm glad to provide a PR if an exception is necessary here.

Best Regards, Leandro

michaelmoussa commented 7 years ago

I'd be in favor of throwing an exception in such an occurrence, and it'd probably be worth looking into how zend-expressive-aurarouter and zend-expressive-zendrouter handle the same situation in order to try to achieve consistency among all three.

I can't think of a use case where I would want to ask for a URL to be generated and be OK with an empty string coming back if I didn't provide enough information for it to be assembled, so I might even consider this behavior a bug. What do you think, @weierophinney ?

weierophinney commented 7 years ago

@michaelmoussa Looks like @xtreamwayz may have a fix; @Lansoweb, could you try his patch and see if it corrects the issue for you?

weierophinney commented 7 years ago

Forgot to link to the patch: #32.

geerteltink commented 7 years ago

@weierophinney A test is added for this issue.

Lansoweb commented 7 years ago

@weierophinney Sure. Will try it in a few. Thanks @xtreamwayz

Lansoweb commented 7 years ago

@weierophinney @xtreamwayz Worked perfectly! Even discovered other links with missed params in the project already :-)