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

Enable HTTP Verb in Routing #2712

Merged
merged 2 commits into from
Nov 11, 2013
Merged

Enable HTTP Verb in Routing #2712

merged 2 commits into from
Nov 11, 2013

Conversation

pveyes
Copy link
Contributor

@pveyes pveyes commented Oct 31, 2013

Using array in config/routes.php for HTTP Verb. HTTP Verb is case-insensitive

e.g:
$route['(:any)']['POST'] = "controller/any_post_method";
$route['path']['get'] = "controller/path_get_method";
$route['path']['(:any)'] = "controller/path_any_method";

Using (:any) or not using http verb at all will produce same result
e.g: $route['path']['(:any)'] is the same as $route['path']

So it won't break any existing routes

Using array for HTTP Verb

e.g:
$route['(:any)']['POST'] = "controller/post_method";
$route['path']['GET'] = "controller/path_get_method";
$route['path']['(:any)'] = "controller/path_any_method";

Using (:any) or not will make same result
e.g: $route['path']['(:any)'] == $route['path']

So it won't break existing route
// Is there a literal match? If so we're done
if (isset($this->routes[$uri]) && is_string($this->routes[$uri]))
if (isset($this->routes[$uri][$http_verb]))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should first check if $this->routes[$uri] exists and then have the other conditions depending on it, i.e.:

if (isset($this->routes[$uri]))
{
    if (is_string($this->routes[$uri]))
    {
        // regular route code
    }
    elseif (is_array($this->routes[$uri]) && isset($this->routes[$uri][$http_verb]))
    {
        // verb route code
    }
    ...
}

@narfbg
Copy link
Contributor

narfbg commented Nov 4, 2013

(:any) is a pseudo-regexp token that is only intended to be used with regular expression routes, it doesn't make sense to use it for request methods. And why do you need to do that anyway?

Also, this feature requires proper documentation and a changelog entry.

@pveyes
Copy link
Contributor Author

pveyes commented Nov 4, 2013

Thanks for your feedback. Sorry, should've read that more thoroughly.

Actually (:any) is optional, it behaves the same as using nothing so you can remove it. I just thought that user may be confused with new routing with http verb, so (:any) will show that the route accept any http verb. I use (:any) because in the existing routing (:any) is like a keyword for anything.

I'll try fixing my code and give more proper documentation

@a-h-abid
Copy link

a-h-abid commented Nov 4, 2013

I was wondering if things can be done in the other way around. I mean, put them like this

$route['some/page'] = array('verb'=>'get');
$route['some/other/page'] = array('verb'=>'post');

This way, it will be easy to target in code. plus if plan to merge with this reverse route method with some modification, things will go nicely. Also any additions can be done easily I think.

I'm using that reverse route in one of my application i'm developing. With changes to the index keys to string, I can check which controller it wants to use and get its alias name.

$route['some/page'] = array('uses' => 'Controller/method', 'as' => 'somepage');

I believe this is one of the main feature of CI3. #218 . Isn't It?

@narfbg
Copy link
Contributor

narfbg commented Nov 4, 2013

No, this wouldn't be convenient in terms of usage.

And no, reverse routing isn't a "main" feature, it's a non-existent feature. The tag on that issue just says that it's a feature request. Ignore it, even if it gets implemented at some point we don't need to worry about it now.

Fix code style, removed (:any) rule in http verb to avoid confusion, and
add proposed documentation and changelog
@CMCDragonkai
Copy link

I use the Pigeon library to handle this. Perhaps that can be integrated? I find it's hierarchical routes rather useful. https://github.com/jamierumbelow/pigeon

narfbg added a commit that referenced this pull request Nov 11, 2013
Enable HTTP Verb in Routing
@narfbg narfbg merged commit af709d6 into bcit-ci:develop Nov 11, 2013
narfbg added a commit that referenced this pull request Nov 11, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants