Skip to content
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

Should we remove 'action', 'controller', 'directory' from request params? #676

Open
djfd opened this issue Apr 16, 2016 · 4 comments
Open

Comments

@djfd
Copy link

djfd commented Apr 16, 2016

Hi,

Currently we have (./system/classes/Kohana/Request.php:974)

// These are accessible as public vars and can be overloaded
unset($params['controller'], $params['action'], $params['directory']);

Eg. the situation with a numerous routes, where some of them require action to be explicitly supplied, while others do not, and some of them even do not have such parameter in their regex, just implied one.

Next, we are going to create canonic url for every page. Not having explicitly supplied action (as an example, the same for controller too, not sure about directory but likely it is quite similar) in params received via $p = $this->request->param() we need to make a certain efforts to supply all required route params when further calling $this->request->route()->uri($p) to avoid getting an exception Required route parameter not passed: action...

Well, when there are just a few routes it could be ok, but with routes amount increase it get even more difficult to supply all that...

Adding action to params array $p['action'] = $this->request->action() before calling ...->route()->uri($p) is not very good too, because we can want some URIs not having it...

As for me, if any of these parameters was explicitly supplied in URI then we need to leave it in params array as well.

Any opinions?

Thanks

@WinterSilence
Copy link

1.$params['directory'] replaced to $params['namespace'] in new version, devs planned use psr-4,
2. Should we remove 'action', 'controller', 'directory' from request params? - no, and i don't understand how you planned do this.
3.Need add in $this->request()->route()(controller) current params as defaults (clone route and rename as current):

// In bootstrap\index
Route::set('frontend', '(<controller>)')->defaults(['controller' => 'Page']);
// In controller Kohana\Core\Controller\Article:
echo Route::name($this->request->route()); // 'current'
echo $this->request->route()->uri() // '/Article'

@djfd
Copy link
Author

djfd commented Apr 20, 2016

@WinterSilence
2. No, I did not. This is already done in current version, see a code snippet above. So possible we need to return them back to that params array somehow?
3. this could work if we have all things predefined. but let look at the next example

Route::set(Route::BACK_END_LOGIN_LOGOFF, '(<lang>(-<locale>)/)backend/<action>', ...

there is not default action (really, what you prefer to have as a default, log in or log off?

And echo $this->request->route()->uri($this->request->param()) will fail just because the code in ./system/classes/Kohana/Request.php:974 removed <action> parameter from params array :(

@WinterSilence
Copy link

WinterSilence commented Apr 20, 2016

@djfd oh, I understood you - delete string ./system/classes/Kohana/Request.php:974 it's good idea, but need update params when changed $this->_action and etc. or delete this properties and use only $this->_params values

@djfd
Copy link
Author

djfd commented Apr 20, 2016

@WinterSilence

--two tickets to Dublin
--куда, блин?
--to Dublin ))

just a kidding :D

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

No branches or pull requests

2 participants