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
Closed

Dispatcher tries to camel case twice #12829

jesseforrest opened this issue Apr 30, 2017 · 12 comments
Assignees
Labels
bug A bug report status: medium Medium

Comments

@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 Stale issue - automatically closed label Apr 16, 2018
@jesseforrest
Copy link
Author

@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
Copy link

@jerejones Could you solve it?

@sergeyklay sergeyklay reopened this Jul 1, 2018
@stale stale bot removed the stale Stale issue - automatically closed label Jul 1, 2018
@stale stale bot added the stale Stale issue - automatically closed label Sep 29, 2018
@stale stale bot closed this as completed Sep 30, 2018
@sergeyklay sergeyklay reopened this Oct 18, 2018
@stale stale bot removed the stale Stale issue - automatically closed label Oct 18, 2018
@sergeyklay
Copy link
Contributor

@niden Could you please take a look

@niden niden self-assigned this Oct 19, 2018
@niden
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.

@alexbusu
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
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
Copy link
Member

niden commented Feb 6, 2019

If you can handle this Alex that would be great.

@alexbusu
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
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
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
Copy link
Member

niden commented Feb 6, 2019

That should be fine :)

@phalcon phalcon deleted a comment from stale bot Feb 13, 2019
@phalcon phalcon deleted a comment from stale bot Feb 13, 2019
@niden
Copy link
Member

niden commented Feb 13, 2019

This has been addressed

@niden niden closed this as completed Feb 13, 2019
@niden niden added the 4.0 label Jun 21, 2019
@niden niden added bug A bug report status: medium Medium and removed Bug - Medium labels Dec 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug A bug report status: medium Medium
Projects
None yet
Development

No branches or pull requests

5 participants