{"action":"created","issue":{"url":"https://api.github.com/repos/mosbth/Anax-MVC/issues/25","repository_url":"https://api.github.com/repos/mosbth/Anax-MVC","labels_url":"https://api.github.com/repos/mosbth/Anax-MVC/issues/25/labels{/name}","comments_url":"https://api.github.com/repos/mosbth/Anax-MVC/issues/25/comments","events_url":"https://api.github.com/repos/mosbth/Anax-MVC/issues/25/events","html_url":"https://github.com/mosbth/Anax-MVC/pull/25","id":154222350,"number":25,"title":"Fixed some bugs, errors and Updated CRouterBasic","user":{"login":"DennisSkoko","id":6786633,"avatar_url":"https://avatars.githubusercontent.com/u/6786633?v=3","gravatar_id":"","url":"https://api.github.com/users/DennisSkoko","html_url":"https://github.com/DennisSkoko","followers_url":"https://api.github.com/users/DennisSkoko/followers","following_url":"https://api.github.com/users/DennisSkoko/following{/other_user}","gists_url":"https://api.github.com/users/DennisSkoko/gists{/gist_id}","starred_url":"https://api.github.com/users/DennisSkoko/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/DennisSkoko/subscriptions","organizations_url":"https://api.github.com/users/DennisSkoko/orgs","repos_url":"https://api.github.com/users/DennisSkoko/repos","events_url":"https://api.github.com/users/DennisSkoko/events{/privacy}","received_events_url":"https://api.github.com/users/DennisSkoko/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":0,"created_at":"2016-05-11T11:38:23Z","updated_at":"2016-05-11T12:20:01Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/mosbth/Anax-MVC/pulls/25","html_url":"https://github.com/mosbth/Anax-MVC/pull/25","diff_url":"https://github.com/mosbth/Anax-MVC/pull/25.diff","patch_url":"https://github.com/mosbth/Anax-MVC/pull/25.patch"},"body":"## Minor Updates\r\nI've updated some more errors, syntax and one Srutinizer issue that occured from last pull request.\r\n\r\nThe following problems are:\r\n* Case-insenitive function calls\r\n* Unused variables\r\n* Incorrect parameter list for parent class\r\n* Undefined Exception\r\n* Scrutinizer issue\r\n\r\nThese problems should not have any problems with previous versions of Anax-MVC\r\n\r\n## Major: CRouterBasic Updated\r\nA problem with the router was that you could not use the default controller (`IndexController`).\r\n\r\nFor example if you wanted a main controller defining the simple routes like this:\r\n```php\r\nclass IndexController {\r\n    // Will work for localhost\r\n    public function indexAction() {...}\r\n    \r\n    /* Will not work for localhost/about.\r\n     * Router will instead look for a 'about' controller with an index action.\r\n     */\r\n    public function aboutAction() {...}\r\n}\r\n```\r\n\r\n## Where is the problem?\r\nThe problem is that if you look here at your current code:\r\n```php\r\n// CRouterBasic : line 132\r\n// If the first part of the url is only for defining the controllers name.\r\n$dispatcher->setControllerName(isset($parts[0]) ? $parts[0] : 'index');\r\n```\r\n\r\nAnother problem was that if you use index action with parameters, it will not work for the first parameter.\r\nImagine this controller.\r\n```php\r\nclass UserController {\r\n    // Will not work for localhost/user/13\r\n    // Because it looks for a action named 13 which doesn't exist.\r\n    public function indexAction($id) {...}\r\n}\r\n```\r\n\r\n## How did you fix it?\r\nInstead of using the variable $parts and just using first value as a controller, second as method and so on.\r\n\r\nThe updated version instead send the first index from $parts to the dispatcher as a controller name, checks if valid.\r\nIf the controller is valid then it will remove the first index from $parts.\r\nIf it' not valid or there is no first value from $parts then the default controller will be used and still continue.\r\n\r\nThe next stage is to check action name and it uses the same first index as before.\r\nThen the process is the same as for controller.\r\n\r\nThe the rest will be parameters for the action.\r\nYou can check the code for more information.\r\n\r\n## Cool, does the fix work?\r\nYes! Works like a charm!\r\n...but it's not perfect.\r\n\r\nA problem occurs where the parameter list can cause some problems.\r\n\r\nAn example of problem:\r\n```php\r\nclass IndexController {\r\n    // Will work for localhost\r\n    public function indexAction() {...}\r\n    \r\n    // Will work for localhost/about.\r\n    public function aboutAction() {...}\r\n}\r\nclass UserController {\r\n    // Will work for localhost/user\r\n    public function indexAction() {...}\r\n}\r\n```\r\n\r\nWhere is the problem? Well if you are going to following route: `localhost/not/valid`\r\nIt will then go to IndexController::indexAction() with the params: `[\"not\", \"valid\"]`.\r\n\r\nThis will result as there will be no 404 pages when you have defined IndexController::indexAction().\r\n\r\n## How did you fix the params problem?\r\nEven though PHP doesn't complain when sending to many parameters, I think it should not be done anyway.\r\nI wanted to somehow validate the parameters list and throw 404 error when the parameter list is too large or small.\r\nSo I updated `CDispatcherBasic` and added a method so it can validate the parameter list like this:\r\n\r\n```php\r\n$reflection = new \\ReflectionMethod($this->controller, $this->action);\r\n\r\nif (count($this->params) === $reflection->getNumberOfParameters()) {\r\n    return true;\r\n}\r\n\r\nreturn false;\r\n```\r\n\r\n\r\n## Conclusion\r\nYou may ask the question: \"Will this pull request be compatible with the older versions?\" and the answer is \"I don't know\".\r\nI can't say for sure if this is compatible but I have tested it with my me-sida from phpmvc and it works fine.\r\nEither you can release this as a new major release or a minor patch. It's up to you.\r\n\r\nI don't know if you want this feature or not but I could recommend it and I don't know much about security but allowing users to send unexpected variables into the system could be one of them.\r\nIf you don't like this change, don't be afraid to tell me. I can do any changes if you want to and I can also send a pull request containing only the minor updates.\r\n\r\nSorry for the long text.\r\n\r\nRegards,\r\nDennis Skoko\r\n"},"comment":{"url":"https://api.github.com/repos/mosbth/Anax-MVC/issues/comments/218442761","html_url":"https://github.com/mosbth/Anax-MVC/pull/25#issuecomment-218442761","issue_url":"https://api.github.com/repos/mosbth/Anax-MVC/issues/25","id":218442761,"user":{"login":"DennisSkoko","id":6786633,"avatar_url":"https://avatars.githubusercontent.com/u/6786633?v=3","gravatar_id":"","url":"https://api.github.com/users/DennisSkoko","html_url":"https://github.com/DennisSkoko","followers_url":"https://api.github.com/users/DennisSkoko/followers","following_url":"https://api.github.com/users/DennisSkoko/following{/other_user}","gists_url":"https://api.github.com/users/DennisSkoko/gists{/gist_id}","starred_url":"https://api.github.com/users/DennisSkoko/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/DennisSkoko/subscriptions","organizations_url":"https://api.github.com/users/DennisSkoko/orgs","repos_url":"https://api.github.com/users/DennisSkoko/repos","events_url":"https://api.github.com/users/DennisSkoko/events{/privacy}","received_events_url":"https://api.github.com/users/DennisSkoko/received_events","type":"User","site_admin":false},"created_at":"2016-05-11T12:20:01Z","updated_at":"2016-05-11T12:20:01Z","body":"After sending this I realised that when using default parameters on your actions will not work.\r\nThis is the updated version of CDispatcherBasic that will fix this problem:\r\n```php\r\npublic function isParamsValid()\r\n{\r\n    $reflection = new \\ReflectionMethod($this->controller, $this->action);\r\n    $numberOfParams = count($this->params);\r\n\r\n    if ($numberOfParams >= $reflection->getNumberOfRequiredParameters()\r\n        && $numberOfParams <= $reflection->getNumberOfParameters()) {\r\n        return true;\r\n    }\r\n\r\n    return false;\r\n}\r\n```"}}