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

Dispatcher tries to camel case twice #12829

Closed
jesseforrest opened this Issue Apr 30, 2017 · 12 comments

Comments

Projects
5 participants
@jesseforrest
Copy link

jesseforrest commented Apr 30, 2017

Expected and Actual Behavior

The following line in the Dispatcher can cause the controller name to be incorrectly camel cased two times, which can cause routes to not work:
https://github.com/phalcon/cphalcon/blob/master/phalcon/dispatcher.zep#L781

For single word controllers (e.g. User) this is not a problem, but for multiple word controllers (e.g. GiftCard) it is.

The problem arises when routes are defined in either of the following formats:

$router->add('/gift-card', ['controller' => 'GiftCard', 'action' => 'showGiftCard']);
$router->add('/gift-card', 'GiftCard::showGiftCard');

IMO, the real problem is that Phalcon Router class is trying to be too accommodating and allowing the controller to be passed in as GiftCard, giftCard or gift_card.

To solve the problem, without a change being made to Phalcon, I could have done 1 of 2 things.

The first thing was to change the controller names to be snake_case. Example:

$router->add('/gift-card', ['controller' => 'gift_card', 'action' => 'showGiftCard']);
$router->add('/gift-card', 'gift_card::showGiftCard');

I decided against this approach because engineers wouldn't be able to simply search across the code base for all uses of GiftCard anymore. I also didn't like this approach because it enforced that controllers were passed into the router in snake_case, but action names were camelCase.

The second option was to override the default Dispatcher behavior. This was the approach I took for now until this issue can be discussed and resolved:

<?php
use Phalcon\Mvc\Dispatcher;

class FixedDispatcher extends Dispatcher
{
    public function getHandlerClass(): string
    {
        parent::getHandlerClass();
        return $this->_handlerName . $this->_handlerSuffix;
    }
}

I will create another story for documentation too, since I feel like the following page should showcase more examples with multiple word controller/action names:
https://docs.phalconphp.com/en/3.0.2/reference/routing.html

Describe what you are trying to achieve and what goes wrong.

I think the documentation and code should be explicit and enforce a certain way. Ideally the way we pass in controller and action names would be similar.

Another side note, which is important for reproducing. This bug only appears if your source code is on a case-sensitive operating system (e.g. linux). It will not appear on mac, or if your code is mounted to a mac OS, because mac treats the following the same: GiftCardController.php, giftcardController.php, giFtCaRdCoNtroLLeR.php`, etc.

Details

  • Phalcon version: (php --ri phalcon)
Web framework delivered as a C-extension for PHP
phalcon => enabled
Author => Phalcon Team and contributors
Version => 3.0.3
Build Date => Dec 24 2016 19:16:23
Powered by Zephir => Version 0.9.5a-dev

Directive => Local Value => Master Value
phalcon.db.escape_identifiers => On => On
phalcon.db.force_casting => Off => Off
phalcon.orm.events => On => On
phalcon.orm.virtual_foreign_keys => On => On
phalcon.orm.column_renaming => On => On
phalcon.orm.not_null_validations => On => On
phalcon.orm.exception_on_failed_save => Off => Off
phalcon.orm.enable_literals => On => On
phalcon.orm.late_state_binding => Off => Off
phalcon.orm.enable_implicit_joins => On => On
phalcon.orm.cast_on_hydrate => Off => Off
phalcon.orm.ignore_unknown_columns => Off => Off
  • PHP Version: (php -v)
PHP 7.0.17-2+deb.sury.org~trusty+1 (cli) (built: Mar 15 2017 09:38:47) ( NTS )
Copyright (c) 1997-2017 The PHP Group
Zend Engine v3.0.0, Copyright (c) 1998-2017 Zend Technologies
    with Zend OPcache v7.0.17-2+deb.sury.org~trusty+1, Copyright (c) 1999-2017, by Zend Technologies
  • Operating System:
Ubuntu
  • Installation type: Compiling from source || installing via package manager
  • Zephir version (if any):
  • Server: Apache
  • Other related info (Database, table schema):

@stale stale bot added the stale label Apr 16, 2018

@sergeyklay sergeyklay closed this Apr 16, 2018

@jesseforrest

This comment has been minimized.

Copy link
Author

jesseforrest commented Apr 16, 2018

@sergeyklay : I spent a decent amount of time documenting the issue. Preferably this would not be automatically closed out without at least a response from the Phalcon team.

@takahashiyuya

This comment has been minimized.

Copy link

takahashiyuya commented Jun 27, 2018

@jerejones Could you solve it?

@sergeyklay sergeyklay reopened this Jul 1, 2018

@stale stale bot removed the stale label Jul 1, 2018

@stale stale bot added the stale label Sep 29, 2018

@stale stale bot closed this Sep 30, 2018

@sergeyklay sergeyklay reopened this Oct 18, 2018

@stale stale bot removed the stale label Oct 18, 2018

@sergeyklay

This comment has been minimized.

Copy link
Member

sergeyklay commented Oct 18, 2018

@niden Could you please take a look

@niden niden self-assigned this Oct 19, 2018

@niden

This comment has been minimized.

Copy link
Member

niden commented Oct 19, 2018

@jesseforrest Thank you for the verbose message. I will try and have a look at this this shortly.

@niden niden added the Bug - Medium label Dec 23, 2018

@niden niden added this to To do in 4.0 Release via automation Feb 6, 2019

@alexbusu

This comment has been minimized.

Copy link
Contributor

alexbusu commented Feb 6, 2019

This is the same issue I faced trying to camelize the action name, in my last PR.
@niden it seems the problems would be solved if the camelize function would not "strtolower" the input before apply the formatting. Currently MyHandler string for controller input in router is returned as MyhandlerController in dispatcher.

@niden

This comment has been minimized.

Copy link
Member

niden commented Feb 6, 2019

So the question is where do we concentrate. camelize function offered by Zephir or fix this here? I fear that messing up with the camelize will break more things :(

@niden

This comment has been minimized.

Copy link
Member

niden commented Feb 6, 2019

If you can handle this Alex that would be great.

@alexbusu

This comment has been minimized.

Copy link
Contributor

alexbusu commented Feb 6, 2019

I noticed where Zephir's camelize() applies this behaviour.
That part is not to be touched though, because it's used in several places across the framework, in sensitive places (adapters name camelize + prepend to NS, Models getters/setters and other stuff like this).
What about Phalcon's own version of camelize (Text::toCamelCase() or something)?
@niden Yes, I could do it.

@niden

This comment has been minimized.

Copy link
Member

niden commented Feb 6, 2019

Regarding the toCamelize this needs much more discussion. I don;t like adding one method just for one area. We have to check all the camelization (if that is even a word) in the framework and come up with a robust solution really.

@alexbusu

This comment has been minimized.

Copy link
Contributor

alexbusu commented Feb 6, 2019

To not complicate things, maybe a private method in dispatcher is enough for these specific cases, of handler name and active method name.

@niden

This comment has been minimized.

Copy link
Member

niden commented Feb 6, 2019

That should be fine :)

@alexbusu alexbusu referenced this issue Feb 12, 2019

Merged

Add `Dispatcher::toCamelCase()` method #13822

3 of 3 tasks complete

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

@phalcon phalcon deleted a comment from stale bot Feb 13, 2019

@phalcon phalcon deleted a comment from stale bot Feb 13, 2019

@niden

This comment has been minimized.

Copy link
Member

niden commented Feb 13, 2019

This has been addressed

@niden niden closed this Feb 13, 2019

4.0 Release automation moved this from In progress to Done Feb 13, 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.