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

Examples don't match text, in routing chapter, section "Adding HTTP Method Requirements" #5583

Closed
toddkaufmann opened this issue Jul 30, 2015 · 13 comments
Labels
actionable Clear and specific issues ready for anyone to take them. bug good first issue Ideal for your first contribution! (some Symfony experience may be required) hasPR A Pull Request has already been submitted for this issue. Routing

Comments

@toddkaufmann
Copy link

or maybe I don't understand something.

The text says
"Despite the fact that these two routes have identical paths (/contact), the first route will match only GET requests and the second route will match only POST requests."

but the examples (annotations,yaml,xml,php) have "/news" and "/contact" and does not match the description.

Okay here in 2.2:
http://symfony.com/doc/2.2/book/routing.html#adding-http-method-requirements

It looks like something happened when annotations were added. I'm not familiar enough with the docs and where the example code block gets pulled in from.

@xabbuh xabbuh added Routing actionable Clear and specific issues ready for anyone to take them. labels Aug 1, 2015
@xabbuh
Copy link
Member

xabbuh commented Aug 1, 2015

That's a good catch @toddkaufmann. We changed the code examples in #5201, but apparently forgot to update the text too.Do you like to create a pull request for this?

@xabbuh xabbuh added good first issue Ideal for your first contribution! (some Symfony experience may be required) bug labels Aug 1, 2015
@toddkaufmann
Copy link
Author

I can't update the text because I don't understand the intent.
To me the previous example was clear so I'd just revert to that.

I scanned through the discussion on this issue, but not being versed enough in Symfony best practices I couldn't explain the use case & implementation properly.

@bicpi
Copy link
Contributor

bicpi commented Aug 4, 2015

Hi everyone,
I've read trough the affected text, examples and discussions.

I think it's essential for understanding this feature to have the exact same URL pattern for two different actions. The routing should only differ in the HTTP method, so you can clearly get the idea behind this. It was clear in the previous example using /contact only, but I see the objection that it's not a best practice. My proposal would be to show a piece of an API, let's say for the URL pattern /api/events. GET lists the events, POST creates an event. The /contact form with [GET, POST] could be used as an additional example how to restrict a route to multiple methods.

What do you think?

@xabbuh
Copy link
Member

xabbuh commented Aug 4, 2015

Wouldn't this then become to lengthy and complicated? I bet beginners would sadly just stop reading when it comes to APIs here. Imo rewording the paragraph would be enough to show how you can use methods requirements.

@bicpi
Copy link
Contributor

bicpi commented Aug 5, 2015

I understand your concerns, but is there a good use case for Adding HTTP Method Requirements for beginners then? I mean, does restricting contactFormAction() to GET and POST makes sense as the default HTTP method is ANY and it will just work without setting a method restriction. From a beginner's perspective without API knowledge there's no PUT or PATCH etc.

@xabbuh
Copy link
Member

xabbuh commented Aug 5, 2015

I am actually not sure. Maybe this should simply be moved to the cookbook explained with the API example.

@Saphyel
Copy link
Contributor

Saphyel commented Aug 9, 2015

I agree with bicpi an example of PUT would be nice

@xabbuh xabbuh added the hasPR A Pull Request has already been submitted for this issue. label Aug 26, 2015
@ycklsr
Copy link

ycklsr commented Aug 26, 2015

Hi there,
I just added a PR (before xabbuh kindly informs me that this issue is still being discussed)
At this stage of the course, no reference of any kind has been made regarding APIs, so I don't think it would be a great idea to unnecessarily complexify this part. We should keep in mind that the original intent in this routing section was to show how easily we can segregate HTTP methods and their subsequent routing even if the routes share the same path.

@bicpi
Copy link
Contributor

bicpi commented Aug 27, 2015

@Gwad Yes, you're right. The problem is, what do we consider as a simple, but valid use-case for this – which conforms to the Symfony Best Practices? API may is, but seems to complex to introduce in this context. Form processing is simple and clear, but does not follow the best practices. Any suggestions for another example?

@ycklsr
Copy link

ycklsr commented Aug 27, 2015

@bicpi Well, maybe that we should simply add a mention in the description text below the example code indicating that such non-exclusive routing is not suitable for REST APIs. If my memory serves me correctly, references to more complex Symfony2 notions were always done this way. On the other side, the examples codes were almost exclusively limited to the illustration of the current notion studied. Which is quite understandable, one's wouldn't illustrate some explanation with a digression... These examples codes are almost made to be copied and pasted by the reader :)

@carlos-granados
Copy link

I think that the API idea is very good. Nowadays almost everyone understands what an API is and how it works, so I don´t think it is going to put off users. In any case, some decission needs to be made. The current situation is showing some text which is plain wrong and has been like this for a very long time. We either fix the example or the text around it

@wouterj
Copy link
Member

wouterj commented Dec 21, 2015

Fyi, as APIs are already introduced earlier in the book, I've created #6062 to rewrite this section to use a very basic API example.

@OskarStark
Copy link
Contributor

👍

xabbuh added a commit that referenced this issue 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 xabbuh closed this as completed Jan 11, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
actionable Clear and specific issues ready for anyone to take them. bug good first issue Ideal for your first contribution! (some Symfony experience may be required) hasPR A Pull Request has already been submitted for this issue. Routing
Projects
None yet
Development

No branches or pull requests

8 participants