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

Update HTTP method requirement example #6062

Merged
merged 1 commit into from
Jan 11, 2016

Conversation

wouterj
Copy link
Member

@wouterj wouterj commented Dec 21, 2015

Q A
Doc fix? yes
New docs? no
Applies to 2.3+
Fixed tickets #5583

After many reports (#5779, #5659) of the text no longer matching the example, I think it's time to update the example.

The example now uses an API, which is a very common thing now and often needs this feature. API's are already used in the Page Creation article, which is placed before this article, so it shouldn't be too hard to understand.

I know that from a REST prespective, HEAD and GET using the same action doesn't seem that great, but it's the only action I could think of that may support 2 methods.

@xabbuh
Copy link
Member

xabbuh commented Dec 31, 2015

👍 I really like this.

@OskarStark
Copy link
Contributor

like it too 👍

Status: Reviewed

@xabbuh
Copy link
Member

xabbuh commented Jan 10, 2016

@javiereguiluz @weaverryan What do you think about it?

path: /news
defaults: { _controller: AppBundle:Main:news }
methods: [GET]
api_show_post:
Copy link
Member

Choose a reason for hiding this comment

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

Just asking: should api_show_post be api_post_show? I usually do "global preffix" (api_) + "item" (post) + "action" (show).

Copy link
Member

Choose a reason for hiding this comment

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

yea, that makes sense - so there would be a loose standard of api_CONTROLLERNAME_ACTIONNAME

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@weaverryan
Copy link
Member

👍 This is very well done

@xabbuh xabbuh merged commit 30c2750 into symfony:2.3 Jan 11, 2016
xabbuh added a commit that referenced this pull request Jan 11, 2016
This PR was merged into the 2.3 branch.

Discussion
----------

Update HTTP method requirement example

| Q | A
| --- | ---
| Doc fix? | yes
| New docs? | no
| Applies to | 2.3+
| Fixed tickets | #5583

After many reports (#5779, #5659) of the text no longer matching the example, I think it's time to update the example.

The example now uses an API, which is a very common thing now and often needs this feature. API's are already used in the Page Creation article, which is placed before this article, so it shouldn't be too hard to understand.

I know that from a REST prespective, HEAD and GET using the same action doesn't seem that great, but it's the only action I could think of that may support 2 methods.

Commits
-------

30c2750 Update method requirement example
xabbuh added a commit that referenced this pull request Jan 11, 2016
@xabbuh
Copy link
Member

xabbuh commented Jan 11, 2016

Thanks Wouter! Changes are merged and route names updated in 87d3167.

}

/**
* @Route("/contact")
* @Method({"GET", "POST"})
* @Route("/api/posts/{id}")
Copy link
Contributor

Choose a reason for hiding this comment

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

i dislike posts here, this is unnecessary ambiguous. (post vs posts)

Copy link
Member Author

Choose a reason for hiding this comment

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

a common practice is to have consistent URLs for your API (to avoid the inconsistency stuff like github's pulls and issues urls). Also, without it the example wouldn't make any sense anymore.

@wouterj wouterj deleted the routing_method_requirement branch January 25, 2016 15:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants