UnionOfRAD / lithium

li₃ is the fast, flexible and most RAD development framework for PHP
http://li3.me
BSD 3-Clause "New" or "Revised" License
1.22k stars 238 forks source link

$this->form->create(...) generates a RoutingException: 'No parameter match found for URL' insides sub modules (libraries) #374

Closed Ciaro closed 11 years ago

Ciaro commented 12 years ago

$this->form->create(...) seems to generate a RoutingException in libraries (sub modules), which is very weird...

Uncaught exception 'lithium\net\http\RoutingException' with message 'No parameter match found for URL ('controller' => 'li3_docs.ApiBrowser', 'action' => 'index', 'library' => 'li3_docs')

How to reproduce: Try adding $this->form->create(null); to any of the views of the library 'li3_docs'.

nateabele commented 12 years ago

The library is being double-listed. Try removing the library key or changing controller to 'ApiBrowser'.

Ciaro commented 12 years ago

Without that key lithium will think the route is part of the main app, and try to render the templates and layout found in app/views/* (instead of for example app/libraries/li3_docs/views/*).

Ciaro commented 12 years ago

unset($url['library']); in form::create() seems to fix this. I will commit a pull request tomorrow with some unit tests to cover it.

Ciaro commented 12 years ago

This issue has been reported before, I somehow missed that. (@see https://github.com/UnionOfRAD/lithium/issues/321)

JoelLarson commented 12 years ago

I managed to get around this issue by doing this in

/config/bootstrap/routes.php
Router::connect(
    '{:controller}/{:action}/{:args}',
    array('locale' => true),
    array('persist' => array('locale', 'controller'))
);

I'm not positive if it's a proper fix, but it removed the error for the moment.

EDIT:

I'd also like to note that this error was triggered when I enabled the g11n.php bootstrap and had a $this->form->create() in my layout.

Another Edit:

It turns out that the Route above fixed my " / " route, but failed with my " /admin/* " route. From what I'm digging through, I think the cause is in the Form library not being able to handle the request params properly.

JoelLarson commented 12 years ago

Okay, my half asleep brain has come to the conclusion that this problem would be fixed by allowing the Router to specify optional params rather than just required params. I think this would fix it because then the 'locale' param (in my case) would trigger the Route with the locale option and route it back to itself with the param removed and the global param set.

marcghorayeb commented 12 years ago

@JoelLarson You can already have optional parameters with setting their default value to null in the second argument of router::connect iirc?

nateabele commented 12 years ago

I think the actual solution here would be to make sure that enabling g11n.php also enables an if-block'd continuation route for locales, similar to this one:

Router::connect(
    '/{:locale:' . join('|', array_keys(Environment::get('locales'))) . '}/{:args}',
    array(),
    array('continue' => true)
);

Or maybe it's just always there in the default routing.

@davidpersson Thoughts?

mariuswilms commented 12 years ago

I like the idea of having a continuation-route wrapped in an if-block. I'll work on this :) We already have several routes that get connected only under certain conditions.

nateabele commented 12 years ago

Really only the test ones, I think. The thing with locale routes, though, is that if you never use a locale key, it never gets triggered anyway, but yeah, I guess it's fine. One less route to hit if you're not using G11n.

Ciaro commented 12 years ago

Ok, I'm clearly missing something here?

What does localized routing have to do with this issue :s?

JoelLarson commented 12 years ago

I agree. This is a patch to the underline issue. I was hoping my comments would help point someone into the right direction.

The routing issue is from a route param being set and the Router not knowing what to do with it.

Unfortunately I'm brand new to Lithium and am not sure how it all works in the core yet. :s

mariuswilms commented 12 years ago

Reviewing this discussion thread I get the feeling that @Ciaro and @JoelLarson are seeing two distinct issues. The latter being addressed by the default localized route. Reopening until submitted PR can be merged.

Ciaro commented 12 years ago

Here's the underlying issue: https://github.com/UnionOfRAD/lithium/pull/388

So I'm closing this issue in favor of that pr.

mariuswilms commented 12 years ago

As long as no PR is merged/patch applied so that the issue reported here is solved. I think it's best to leave this ticket open. There's information in here that isn't attached to the PR.

Both issue and PR are linked together so we're reminded of this open ticket when working on the PR.

Ciaro commented 12 years ago

Same for me, I just tought you'd like to reduce the number of open issue (since you basically can mark this one as duplicate of) ;)

mariuswilms commented 12 years ago

I definitely like to reduce the number of tickets :) - It's true another possibility would be to see this as a dupe. However it's best to leave it like it is now and clarify our view upon the conceptual relationship between PR and "normal" tickets later.

Ciaro commented 12 years ago

Sure. Keep up the good work!

nateabele commented 12 years ago

Yup, sorry, my brain got sidetracked with the introduction of G11n into the conversation. :-) There are still other (but possibly closely related) issues here.

nateabele commented 11 years ago

Okay, closing this because $this->form->create(null) works fine from any li3_docs view, with or without g11n.php. Also, #388 has been resolved.