mrjgreen / phroute

A super fast PHP router, with route parameters, restful controllers, filters and reverse routing.
Other
714 stars 96 forks source link

Controllers Routing #28

Open TCB13 opened 9 years ago

TCB13 commented 9 years ago

Hello,

I was trying to setup a controller like explained here: https://github.com/mrjgreen/phroute#controllers

But it doesn't work properly. It always ends up on the anyIndex() method and ignores the for example postTest() if I set my method to POST. If I remove the anyIndex() method it doesn't work.

Is this poorly implemented or I'm using it the wrong way?

mrjgreen commented 9 years ago

Not sure. I'll have a quick look and see if I can replicate. In the meantime if you want to post your code as a comment here I can take a look and see if I can work out where its going wrong.

TCB13 commented 9 years ago

@mrjgreen I found out that if the method is named post() instead of postTest() it works fine. I'm running on dev-master was this changed recently?

TCB13 commented 9 years ago

I also found another small issue regarding the controllers... If I try to do something like this:

$router->group(["prefix" => "/test/{id:i}"], function($router) use($ns) {
    $router->controller("/", "$ns\\Test");
    $router->controller("/page", "$ns\\Page");
});

And then declare on the Page and Test controllers the id:

class Page {

    public function get($id)
    {
        return 'PAGE - GET method ' . $id;
    }

}

I'll get Phroute\\Phroute\\Exception\\BadRouteException: Cannot use the same placeholder 'id' twice.

However, if I do it manually, it will work...

$router->group(["prefix" => "/test/{id:i}"], function($router) use($ns) {
    $router->get("/", ["$ns\\Test", "get"]);
    $router->get("/page", ["$ns\\Page", "get"]);
});

Why? Thanks.

mrjgreen commented 9 years ago

I think you may have the implementation wrong... Please post your code and I'll point you in the right direction. Is there are reason for you running on dev-master? Always best to go for a stable release if possible.

mrjgreen commented 9 years ago

The first part of the controller method is the http method verb. So getTest will resolve to a route /test sent with a method GET.

The behaviour of a public controller method named just get is currently undefined. I will add this to the tests and throw an appropriate exception.

Please post your entire route set up so I can try to figure out what you are doing. You will also need to show me both your Test and Page controllers - its no good just putting one of them up there :)

TCB13 commented 9 years ago

@mrjgreen Regarding the first issue:

I had it implemented like this:

$router->controller('/page/', "$ns\\Page");

And then my Page Controller was:

class Page {

    public function anyIndex() {
        return "PAGE - any index";
    }

    public function getPage()
    {
        return 'PAGE - GET method';
    }

}

And it just gave me PAGE - any index. However if I rename getPage() into get() it works without any issues.

Looks like the docs aren't updated. It's seems better just to write get() instead of getPage() it's smaller and less redundant.

mrjgreen commented 9 years ago

No you cannot write just get()! I'm afraid you have misunderstood how the controller works. The docs are complete and comprehensive. Could you also post your dispatch command. I think thats the crux of this.

NB. The any in anyIndex refers to the method I.E. this route will respond to any http method: get,post,put etc...

A correct working implementation below:


$router->controller('/page', "$ns\\Page");

class Page {

    public function anyIndex() {
        return "PAGE - ANY index";
    }

    public function getPage(){
        return 'PAGE - GET method';
    }
}

#Dispatch

$dispatcher = (new Dispatcher($router->getData()));

$dispatcher->dispatch('GET', '/page'); //PAGE - ANY index
$dispatcher->dispatch('PUT', '/page'); //PAGE - ANY index
$dispatcher->dispatch('POST', '/page'); //PAGE - ANY index

$dispatcher->dispatch('GET', '/page/page'); //PAGE - GET method
mrjgreen commented 9 years ago

I may be wrong, but it seems that what you may be trying to do is really:


#Note the controller is assigned to '/'
#this is the base which will prepend the routes within the controller
$router->controller('/', "$ns\\Page");

class Page {

    public function anyIndex() {
        return "PAGE - ANY index";
    }

    public function getPage(){
        return 'PAGE - GET method';
    }
}

#Dispatch

$dispatcher = (new Dispatcher($router->getData()));

$dispatcher->dispatch('GET', '/'); //PAGE - ANY index
$dispatcher->dispatch('PUT', '/'); //PAGE - ANY index
$dispatcher->dispatch('POST', '/'); //PAGE - ANY index

$dispatcher->dispatch('GET', '/page'); //PAGE - GET method
TCB13 commented 9 years ago

My dispatcher is:

$dispatcher = new \Phroute\Phroute\Dispatcher($this->router->getData());
$response = $dispatcher->dispatch($_SERVER['REQUEST_METHOD'], parse_url($_SERVER['REQUEST_URI'], PHP_URL_PATH));
TCB13 commented 9 years ago

Why do you say Note the controller is assigned to '/'?

Can't I just have a controller for each route I might want? In a way that:

mrjgreen commented 9 years ago

Okay - try one of the set ups I've suggested and let me know what you find... does that give the expected results? I'm not 100% sure I understand what you are trying to achieve with your routes. Perhaps if you could give me an idea of which routes you want to define I will show you how to do it. So post something showing:

GET     /
GET     /page
POST    /page/create
PUT     /page/update
etc...
TCB13 commented 9 years ago

I don't know if you read my comment because we commented almost at the same time. The ideia, as described, was to:

mrjgreen commented 9 years ago

You are right I didn't :) I've moved my reply below

class Test {
    public function getIndex(){
        return 'GET - test'
    }

    public function postIndex(){
        return 'POST - test'
    }
}

class Page {
    public function getIndex(){
        return 'GET - page'
    }

    public function postIndex(){
        return 'POST - page'
    }
}

$router->controller('/test', 'Controllers\Test');
$router->controller('/page', 'Controllers\Page');

#To have routes responding on:
GET      /page    //GET - page
POST     /page    //POST - page
GET      /test    //GET - test
POST     /test    //POST - test

You are correct that get() and post() will achieve a similar result but this is not actually by design. I would advise against using that... its undefined behaviour and may change in the future.

mrjgreen commented 9 years ago

Note this would then allow you to extend functionality by doing:

class Page {
    public function getIndex(){
        return 'GET - page'
    }

    public function postIndex(){
        return 'POST - page'
    }

    public function putPublish(){
        return 'POST - publish'
    }

    public function putSuspend
        return 'POST - suspend'
    }

    etc...
}

$router->controller('/page', 'Controllers\Page');

#To have routes responding on:

GET      /page    //GET - page
POST     /page    //POST - page
PUT      /page/publish    //PUT - publish
PUT      /page/suspend    //PUT - suspend
etc...
mrjgreen commented 9 years ago

I actually quite like your idea of defining the default route using just get(), post() etc...

Maybe I will add this into the tests and make it defined behaviour :) Then we can keep it!

TCB13 commented 9 years ago

Great! I initially misunderstood the default routing using Index. I'm glad that even with that we could make anything useful with it!

As I said before just get() and post() make things easier, I'm not really against adding Index on those methods, but well, you know, simplicity always pays off.

Anyway, what about this:

$router->controller('/page/{id:i}', "$ns\\Page");

I always get a Phroute\\Phroute\\Exception\\BadRouteException: Cannot use the same placeholder 'id' twice is this normal? Why can't it be done?

Thanks for all!

Timmybrain commented 6 years ago

Great! I initially misunderstood the default routing using Index. I'm glad that even with that we could make anything useful with it!

As I said before just get() and post() make things easier, I'm not really against adding Index on those methods, but well, you know, simplicity always pays off.

Anyway, what about this:

$router->controller('/page/{id:i}', "$ns\\Page");

I always get a Phroute\\Phroute\\Exception\\BadRouteException: Cannot use the same placeholder 'id' twice is this normal? Why can't it be done?

Thanks for all!

I am really interested in this last question.

How can I get ID or other unique identifiers from a controller based routes!

DoctorMcKay commented 5 years ago

@Timmybrain You can use func_get_arg(0) to get the first placeholder value in your controller's route. In fact, if you have placeholders in your controller's route, you must use func_get_arg for everything, even if you specify further arguments in the controller method.

For example:

$router->controller('/users/{id}', UserController::class);

And in your controller, you might have:

class UserController {
    public function getIndex() {
        echo "User ID: " . func_get_arg(0);
    }

    public function getFoo($arg) {
        echo "User ID: " . func_get_arg(0) . "; arg: " . func_get_arg(1);
    }
}

Thus requesting /users/5 would output User ID: 5 and requesting /users/5/foo/9 would output User ID: 5; arg: 9.