Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Fix a bug where all the routes from the second controller onwards were incorrectly connected. #2987

Merged
merged 1 commit into from Mar 10, 2014

Conversation

Projects
None yet
6 participants
Contributor

bar commented Mar 10, 2014

No description provided.

Member

ADmad commented Mar 10, 2014

Tests?

Owner

markstory commented Mar 10, 2014

It looks pretty clear to me that $options gets overwritten. Odd that no tests failed though.

@markstory markstory added this to the 3.0.0 milestone Mar 10, 2014

Contributor

bar commented Mar 10, 2014

@ADmad I'm not in a box right now, will see what I can do later.

@markstory Yup.

Is there a reason for not being able to set _ext when using mapResources()?

Owner

markstory commented Mar 10, 2014

@bar Probably because the intent is that people will use parseExtensions() and content type negotiation.

Contributor

bar commented Mar 10, 2014

Mhh, but parseExtensions() will not parse a RESTful route like /controller/12 (notice the lack of .json there) and sometimes you just want your API to always deliver the same content type. What should be the correct way to do it then?

Member

dereuromark commented Mar 10, 2014

@bar For me a RESTful route that is supposed to accept/deliver JSON should always have the .json extension.

Contributor

bar commented Mar 10, 2014

@dereuromark Because REST is an architectural style and not a standard IMO (and in regard of RESTful routes) that is what content type negotiation is used for. Consider 2 resources, one delivering jpeg images and the other JSON, the syntax for both should be the same, and the client should know what to do based on what the content type from the response is.

What I was saying was that we should be able to build RESTful routes without the .json using what we use for that in CakePHP (or maybe setting it as the default content type when no extension is set).

The question being, can we do that in a simple way?

Owner

markstory commented Mar 10, 2014

@bar That is not true, you can use Accept: application/json in the headers to do content negotiation.

REST purists would probably argue that extensions are wrong and that you should always use content negotiation. More strictly you should be using application specific content types like application/vnd-myapp+json for example.

Contributor

bar commented Mar 10, 2014

@markstory Sorry, what is not true? (I may have missed it in the translation).

Contributor

bar commented Mar 10, 2014

@markstory I don't mean extensions are wrong, they are very useful when you want to change the content type of the response from the client. Not a purist either, so the media type in the content type goes beyond the scope of the question :p

Owner

markstory commented Mar 10, 2014

Mhh, but parseExtensions() will not parse a RESTful route like /controller/12 (notice the lack of .json there)

@bar From what I remember, parseExtensions() wasn't choosy about rest/non-rest routes.

@markstory markstory added a commit that referenced this pull request Mar 10, 2014

@markstory markstory Merge pull request #2987 from bar/3.0-mapResources
Fix a bug where all the routes from the second controller onwards were incorrectly connected.
17f3725

@markstory markstory merged commit 17f3725 into cakephp:3.0 Mar 10, 2014

1 check passed

default The Travis CI build passed
Details

@bar bar deleted the bar:3.0-mapResources branch Mar 10, 2014

Contributor

bar commented Mar 11, 2014

@markstory It is not, but it is about extensions, so that route won't qualify as a mapResource'd one :(

Owner

markstory commented Mar 11, 2014

Ok, I don't think I understand the problem. I will have to try some requests against a mapped resource.

Contributor

bar commented Mar 11, 2014

What I want to achieve is to have a RESTful API without the need to specify the extensions in the resources

GET http://example.org/post/1 instead of GET http://example.org/post/1.json

Maybe 'fixing' the resource representation to JSON, XML, etc server side.

Owner

lorenzo commented Mar 11, 2014

@bar is is acceptable to require the api clients to send the "Accept" header? That is how it is currently implemented and works fine

Member

AD7six commented Mar 11, 2014

If you need it you can of course add a detector.

i.e. this sort of thing:

function beforeFilter() {
    $this->request->addDetector('json', function() { return true; // or reference $this->params[xxx] });
    parent::beforeFilter();
}

Maybe I'm also missing the point - I get kind of lost between "it is about extension" and "I want ... without extension".

Contributor

bar commented Mar 11, 2014

My apologies for not being clear enough.

What I try to accomplish:

  1. build a RESTful API
  2. use mapResources()
  3. Don't (completely) rely on extensions to represent the responses.
  4. Have a default representation when no extensions and no Accept headers are used in the requests.

For what I know, I can do the first 3 (considering JSON and XML) very easily using parseExtensions() to achieve content type negotiation (Accept header) as @markstory and @lorenzo points out.

I may be the one missing something :p but... what should be the best approach to accomplish (4)?

Owner

markstory commented Mar 11, 2014

For 4. you can just set the default viewClass to be 'Json' in your controller class definitions.

Owner

lorenzo commented Mar 11, 2014

I agree with @bar, mapResources should be able to accept ['ext' => 'json'] to be emrged in the second argument of Router::connect. I've donde that in some project for providing a default like this for the urls that are meant to return json, it was easier than requesting the consumers to send the correct headers.

Owner

markstory commented Mar 11, 2014

Fair enough, having a default extension feels quite reasonable to me, and is something we could even add to 2.5, or another 2.x release.

Contributor

bar commented Mar 11, 2014

@markstory Thanks.

I remember setting viewClass, but I also recall having some issue when using Crud, it changed the expected view file and ended in a Missing View error.

Owner

markstory commented Mar 11, 2014

@bar Do you want to cook up a pull request for being able to set a default extension on the routes mapResources() makes?

Contributor

bar commented Mar 11, 2014

@markstory Sure I'll see what I can do.

Contributor

bar commented Mar 12, 2014

I did not have much time, but maybe something like #3007?

@mattpotts mattpotts pushed a commit to mattpotts/cakephp that referenced this pull request Apr 4, 2014

@ADmad ADmad Fix datetime comparison in relative datetime functions.
Closes #2987,#3514
ae8386c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment