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

Controller.Module.Action Event is case-sensitive #8422

Closed
Zeichen32 opened this Issue Jul 24, 2015 · 10 comments

Comments

Projects
None yet
3 participants
@Zeichen32
Copy link
Contributor

Zeichen32 commented Jul 24, 2015

I've noticed that Events triggered by the FrontController Dispatcher are case-sensitive, but the called actions are is case-insensitive.

For example:
?module=CoreAdminHome&action=optout => triggers action CoreAdminHome:_optOut_
?module=CoreAdminHome&action=optOut => triggers action CoreAdminHome:_optOut_
?module=CoreAdminHome&action=OptOut => triggers action CoreAdminHome:_optOut_

Events:
?module=CoreAdminHome&action=optout => triggers event Controller.CoreAdminHome._optout_
?module=CoreAdminHome&action=optOut => triggers event Controller.CoreAdminHome._optOut_
?module=CoreAdminHome&action=OptOut => triggers event Controller.CoreAdminHome._OptOut_

So the docs or the behavior is wrong:

* Triggered directly before controller actions are dispatched.
*
* This event exists for convenience and is triggered directly after the {@hook Request.dispatch}
* event is triggered.
*
* It can be used to do the same things as the {@hook Request.dispatch} event, but for one controller
* action only. Using this event will result in a little less code than {@hook Request.dispatch}

File: https://github.com/piwik/piwik/blob/master/core/FrontController.php#L492

@tsteur

This comment has been minimized.

Copy link
Member

tsteur commented Jul 24, 2015

I'm a bit surprised to are all rendered perfectly. We should always use the correct action name as defined in the code or alternatively always lowercase I don't really mind :) We should have a look why it renders all of those actions when only one should be valid

@Zeichen32

This comment has been minimized.

Copy link
Contributor Author

Zeichen32 commented Jul 24, 2015

Its because php functions are case-insensitive.

Note: Function names are case-insensitive, though it is usually good form to call functions as they appear in their declaration.

http://php.net/manual/en/functions.user-defined.php

The easiest way were to convert the controller action name to lowercase. But it's maybe a BC.

@mattab

This comment has been minimized.

Copy link
Member

mattab commented Jul 24, 2015

I would suggest that &action= should match the controller action as found in code (case sensitive) VS using lowercase only

Edit: this may also be BC breaking for some users, but I suppose only the few who would have used the wrong case for &action in their Widgetized URL or wrong case in events listeners

@Zeichen32

This comment has been minimized.

Copy link
Contributor Author

Zeichen32 commented Jul 24, 2015

If we change the ControllerResolver it can be done case-sensitive:

https://github.com/piwik/piwik/blob/master/core/Http/ControllerResolver.php#L74

/** @var $controller Controller */
$controller = $this->abstractFactory->make($controllerClass);

$action = $action ?: $controller->getDefaultAction();

if (!is_callable(array($controller, $action)) || !in_array($action, get_class_methods($controller))) {
    return null;
}
@tsteur

This comment has been minimized.

Copy link
Member

tsteur commented Jul 24, 2015

👍 sounds good, should also not cause any performance issues

@Zeichen32

This comment has been minimized.

Copy link
Contributor Author

Zeichen32 commented Jul 24, 2015

Ive create a PR to fix this issue #8426

@tsteur

This comment has been minimized.

Copy link
Member

tsteur commented Jul 24, 2015

We could afterwards write a test that gets a list of all events (starting with Controller.) of all plugins and check whether the controller actions actually exist. This way we make sure we don't break anything and will get notified easily in case one has a typo in the future

@Zeichen32

This comment has been minimized.

Copy link
Contributor Author

Zeichen32 commented Jul 24, 2015

I will add some tests

@mattab mattab added this to the 3.0.0 milestone Sep 18, 2015

@mattab mattab added the c: Platform label Sep 18, 2015

sgiehl added a commit that referenced this issue Oct 4, 2015

Fix issue #8422
Author:    Jens Averkamp <j.averkamp@two-developers.com>

sgiehl added a commit that referenced this issue Oct 6, 2015

Fix issue #8422
Author:    Jens Averkamp <j.averkamp@two-developers.com>

@mattab mattab modified the milestones: 3.0.0-b1, 3.0.0 Feb 8, 2016

@tsteur

This comment has been minimized.

Copy link
Member

tsteur commented Apr 4, 2016

This issue was closed via #8426

@tsteur

This comment has been minimized.

Copy link
Member

tsteur commented Aug 8, 2016

From my understanding this issue was fixed

@tsteur tsteur closed this Aug 8, 2016

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.