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

Route action and controller parameter inconsistencies #13555

Closed
scrnjakovic opened this Issue Oct 28, 2018 · 9 comments

Comments

Projects
6 participants
@scrnjakovic
Copy link
Contributor

scrnjakovic commented Oct 28, 2018

$router->add('...', [
    'controller' => 'my_controller' // MyController
    'action' => 'my_action' // my_action
]);
@stamster

This comment has been minimized.

Copy link
Contributor

stamster commented Oct 30, 2018

That's why I prefer to use MicroCollection - define everything manually.

$adminRoutes->get('/deconnexion', 'logout', 'admin_logout');
@scrnjakovic

This comment has been minimized.

Copy link
Contributor Author

scrnjakovic commented Oct 30, 2018

@stamster nonetheless, the fact that framework treats those two differently is an issue :)

@Ark4ne

This comment has been minimized.

Copy link

Ark4ne commented Oct 30, 2018

Personally, I do not think it's a problem.

The controller is transformed because you have to use the PSR-4.
The action is not transformed because there is no standard for the use of camelCase or snakeCase.

It's up to the developer to do it right in my opinion.

@scrnjakovic

This comment has been minimized.

Copy link
Contributor Author

scrnjakovic commented Oct 30, 2018

@Ark4ne the two are closely related, so if we use one case for controller, we should stick with it when it comes to the action as well, regardless of the lack of the standard for the action.

Standards exist so that code we write is compatible and predictable. The lack of it doesn't mean we should stop caring. Having controller and action use two completely different cases introduces unexpected behavior, especially since it's not clearly mentioned in the docs.

But I could delete my whole answer and just say consistency.

@niden niden self-assigned this Oct 31, 2018

@niden

This comment has been minimized.

Copy link
Member

niden commented Oct 31, 2018

Not going to be fixed/addressed in 3.x series. Adding this to 4.x

Thank you @scrnjakovic

@niden niden added the Bug - Medium label Oct 31, 2018

@niden niden added this to the 4.0.0 milestone Oct 31, 2018

@scrnjakovic

This comment has been minimized.

Copy link
Contributor Author

scrnjakovic commented Oct 31, 2018

Nope, thank you @niden.

@JABirchall

This comment has been minimized.

Copy link

JABirchall commented Nov 11, 2018

@scrnjakovic If i get this right you want the framework to call your controller My_Controller
Unfortunatly I this will be possible, even in 4.x as 4.0 is aiming to be full PSR compliant, and class named My_Controller will violate:

you can't use underscores in Namespaces or classnames. They must be CamelCase with no seperations between words.

Also @Ark4ne PSR-1 4.3 enforces method naming conventions to lowerCamelCase.

The only bug is that the action does not get transformed to myAction @niden.

@gnumoksha

This comment has been minimized.

Copy link

gnumoksha commented Nov 27, 2018

@JABirchall as @scrnjakovic said, the problem is the different behaviour. my_controller will be mapped to MyController and my_action will be mapped to a controller's method named my_action. Since @Ark4ne already pointed out the PSR guidelines for the class name, the most reasonable is to change the way Phalcon maps the action name.

@niden niden added this to In progress in 4.0 Release Nov 28, 2018

@niden niden moved this from In progress to To do in 4.0 Release Nov 28, 2018

@niden niden moved this from To do to In progress in 4.0 Release Feb 2, 2019

@niden

This comment has been minimized.

Copy link
Member

niden commented Feb 6, 2019

Resolved

@niden niden closed this Feb 6, 2019

4.0 Release automation moved this from In progress to Done Feb 6, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.