fuel / core

Fuel PHP Framework - The core of the Fuel v1 framework
http://fuelphp.com
813 stars 345 forks source link

Controller_Rest Promotes Non-RESTful URIs #649

Closed knudpotente closed 12 years ago

knudpotente commented 12 years ago

In a RESTful API, the URI should identify only the resource that is being acted on - for instance:

  1. http://myserver.com/monkeys/ - identifies the list of monkeys on the server
  2. http://myserver.com/monkeys/33 - identifies monkey id 33 on the server
  3. http://myserver.com/monkeys/33/bananas - identifies the list of bananas that belong to monkey 33

In order to identify the action that is to be performed with these resources, in a RESTful API we use only the HTTP verb:

  1. GET http://myserver.com/monkeys/ - I want to retrieve the list of monkeys from the server
  2. GET http://myserver.com/monkeys/33 - I want to retrieve the monkey who has id 33
  3. GET http://myserver.com/monkeys?country=Kenya&sex=M - I want to retrieve a list of male monkeys from Kenya
  4. POST http://myserver.com/monkeys/ - I am creating a new monkey record
  5. POST http://myserver.com/monkeys/33/bananas - I am creating a new banana record for monkey 33
  6. PUT http://myserver.com/monkeys/33 - I am updating the record of monkey 33 (can also be used to create a new record, if monkey 33 doesn't already exist)
  7. DELETE http://myserver.com/monkey/33 - I want to delete monkey 33 from the server.

As can be seen, in a RESTful API the action is NOT specified in the URI - this is a very important distinction between RESTful and non-RESTful APIs. The following, for instance, is not a RESTful URI:

Because this URI specifies an action, not a resource.

In a RESTful interface, we should only ever have to deal with 4 actions, and a RESTful controller, therefore, should only need to have 4 functions: get(), post(), put() and delete().

In 1.1-RC1, however, the 'standard' Controller_Rest seems to rely on the programmer having to use action-based URIs in order to route to an appropriate function. Even though it's a 'REST Controller', it is not using the HTTP method to determine what action to take on the resource. This seems to be teaching and promoting un-RESTful implementations.

It is reasonably trivial to implement 'standard' RESTful routing, simply by adding the following router() function at the beginning of a Controller_Rest sub-class:


    public function router($res, array $args){
        parent::router($res, $args);

        switch (Input::method()){
            case "GET":
                $this->get_index($res,$args);
                break;
            case "POST":
                $this->post_index($res,$args);
                break;
            case "PUT":
                $this->put_index($res,$args);
                break;
            case "DELETE":
                $this->delete_index($res,$args);
                break;
            default:
                // The request uses an unsupported http method
                // Handle the error here - i.e.:
                $data['error'] = "Unsupported HTTP method request.";
                $this->response($data);
        }
    }

The controller then has to implement the appropriate _get_index(), post_index(), put_index() and deleteindex() functions.

It seems to me, that this should be the default behaviour and setup of a Controller_Rest class. If the programmer needs to implement action-based URIs, then they are diverging from the RESTful standard, and they should have to use router() - and not the other way around.

knudpotente commented 12 years ago

Below is the complete code for the sub-class I'm currently designing to use as a super for my RESTful controllers. It implements standard RESTful practices, and the behaviour can be easily overridden by sub-classes, if needed:

<?php

class Controller_Restful extends Controller_Rest
{
    public function router($res, array $args)
    {
        parent::router($res, $args);

        switch (Input::method()){
            case "GET":
                $this->get_index($res,$args);
                break;
            case "POST":
                $this->post_index($res,$args);
                break;
            case "PUT":
                $this->put_index($res,$args);
                break;
            case "DELETE":
                $this->delete_index($res,$args);
                break;
            default:
                // The request uses an unsupported http method
                // Handle the error here - i.e.:
                $data['error'] = "Unsupported HTTP method request.";
                $this->response->set_header('Allow', 'GET, POST, PUT, DELETE');
                $this->response($data, 405);
        }
    }

    // HTTP Status Codes that MUST be used when producing Responses for
    // our GET, POST, PUT and DELETE operations:
    // 200 - Success - operation successfull, return resource info as $data
    // 204 - No Content - operation successfull, but no $data will be returned.
    // 400 - Bad Request - operation could not be done, because resource info
    //                     was malformed (or missing). No return $data needed.
    // 403 - Forbidden - client does not have authorisation to perform the
    //                   operation at this URI. May return $data explaining.
    // 404 - Not Found - resource at this URI does not exist. No $data returned.
    // 500 - Internal Server Error - operation produces an error - such as a
    //                      database query error. May return $data explaining.
    //
    // Responses WITH $data can be set with:
    // $this->response($data,$http_status);
    //
    // Responses WITHOUT $data can be set simply with:
    // $this->response->status = $http_status;

    public function get_index($res, array $args)
    {

    }

    public function post_index($res, array $args)
    {

    }

    public function put_index($res, array $args)
    {
        // If PUTing without specifying a resource, then
        // client is attempting to update an entire list of resources at once.
        // This is usually a security risk and in most cases not to be allowed:
        if (empty($res)){
            $data['error'] = "Unsupported HTTP method request.";
            $this->response->set_header('Allow', 'GET, POST');
            $this->response($data, 405);
        }
    }

    public function delete_index($res, array $args)
    {
        // If DELETEing without specifying a resource, then
        // client is attempting to delete an entire list of resources at once.
        // This is usually a security risk and in most cases not to be allowed:
        if (empty($res)){
            $data['error'] = "Unsupported HTTP method request.";
            $this->response->set_header('Allow', 'GET, POST');
            $this->response($data, 405);
        }
    }

}
dhrrgn commented 12 years ago

What are you talking about? Controller_Rest determined which action to invoke by looking at the the Request method. So if you POST to /monkeys it will invoke the post_index() method on the 'Monkeys' controller.

philsturgeon commented 12 years ago

Agreed with Dan here, you are talking nonsense. Call your controller Monkeys and use function index_post() {}. If thats not enough use Routes. REST controller is fine.

knudpotente commented 12 years ago

@phil: from what you are saying, then there are "magic" index_post, index_get, index_put and index_delete, right? I have not tested this, but I can say for certain that 'post_index' does -not- get called automatically, as suggested by Dan. This is why I had to add the routes() function.

If, indeed, by using 'index_[method]' we get the functionality automatically, then perhaps this could be changed for a request for it to be mentioned explicitly in the manual? - it's pretty important.

Many thanks in advance.

philsturgeon commented 12 years ago

Normally a URL is action_foo() and it can be reached under any method, then in the Controller_Rest controller you can use post_foo(). post_index() will be the default method hit when you post to the controller, so it's not "magic" it works with the same logic as the whole rest of FuelPHP.

knudpotente commented 12 years ago

Indeed, that is how i would expect it work. In my end, however, it doesn't: 'post_index' does not get called by default, nor put/get/delete_index. That is why I've had to use the routes() function, and that is the reason for this issue.

I'm happy to hear that this is the desired and intended behaviour. The manual seems to imply that an explicit action is required in the URL.

philsturgeon commented 12 years ago

No your issue suggested the logic and implementation was wrong. If its not being called then its a bug but that is very different to saying "The REST controller is not RESTful".

Please do a little debug as things are working over here.


Knud Potente mailto:reply@reply.github.com 2 December 2011 12:42

Indeed, that is how i would expect it work. In my end, however, it doesn't: 'post_index' does not get called by default, nor put/get/delete_index. That is why I've had to use the routes() function, and that is the reason for this issue.

I'm happy to hear that this is the desired and intended behaviour. The manual seems to imply that an explicit action is required in the URL.


Reply to this email directly or view it on GitHub: https://github.com/fuel/core/issues/649#issuecomment-2989548