-
Notifications
You must be signed in to change notification settings - Fork 238
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fixing double use of library key in Router::match(). #388
Conversation
$response = Dispatcher::run(new Request(array( | ||
'url' => '/users/login' | ||
))); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this looks like an integration test now. are you sure the correct library/controller will be dispatched?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I was in doubt whether it was an integration test or not. Never the less, it should proof the bug...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It may show "a" bug, but not 'the" bug. Also, fixing this bug may introduce other bugs because the integration test is incomplete. We need to make sure that the proper library is loaded when the key is unset.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, where do we go from here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bump -_-
@@ -215,6 +215,10 @@ public static function match($url = array(), $context = null, array $options = a | |||
$suffix = isset($url['#']) ? "#{$url['#']}" : null; | |||
unset($url['#']); | |||
|
|||
if (is_array($url) && isset($url['library'])) { | |||
unset($url['library']); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't look like the correct fix at all. How would this not break all attempts to route to a library?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nateabele Have a look at the library logic in Dispatcher::applyRules
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'Dispatcher::applyRules' makes sure both the library key and the controller library prefix (lib.controller) are set, since some users use the library key, others the prefix. I also think this 'magic' is needed to make the route self-aware of it's context (it's part of the library x). 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/*). I've been using this fix for a month now, unsetting $url['library'] in the router hasn't shown any issues for me so far. I hope this makes things a bit more clear from my perspective...
Conflicts: tests/cases/net/http/RouterTest.php
I recently came over this issue and had a lot of struggle with views within libraries. Probably, this is not the correct solution, but it would then help to have some guidance on how to do things properly. If possible, we can go into a chat on irc to further clarify what this is all about. Thanks for more information about that topic. My current solution looks like that:
It does not work as expected, but it gets me closest to what i want to achieve. That is: rendering the view from the plugin, but the layout from the app. Elements are the big question here, still working on that. |
Failing test included + fix for issue #374 and #321.