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

[Book][Routing] Add example about how to match multiple methods #5201

Merged
merged 1 commit into from
Jul 16, 2015

Conversation

xelaris
Copy link
Contributor

@xelaris xelaris commented Apr 21, 2015

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

@weaverryan
Copy link
Member

I like this, but I'd actually like to solve it in an easier way: add a commented-out example in the previous section:

contact_process:
    path:     /contact
    defaults: { _controller: AppBundle:Main:processContact }
    methods:  [POST]
    # or to match GET *or* POST methods
    # methods: [GET, POST]

What do you think @xelaris?

@javiereguiluz
Copy link
Member

I usually prefer concise explanations, as proposed by @weaverryan. But in this case, I think it could be better to show a verbose explanation as proposed by @xelaris. Adding a comment is not clear enough for me. Let's ask @xabbuh and @wouterj for their opinion about this. Thanks.

@xabbuh
Copy link
Member

xabbuh commented Jun 24, 2015

I would have said that the example proposed by @weaverryan would be clear enough.

@javiereguiluz Do you think from your experience about talking with beginners that this is something not so obvious (even with commented out example)?

@javiereguiluz
Copy link
Member

@xabbuh I don't know which would be the opinion of newcomers. It was just a personal comment. Let me show you a pair of screenshots explainig why I said that.

We have this:

before1

And Ryan proposes to add a comment:

after1

It's not a bad idea at all. But my concern is ... what would happen with the other 3 config formats? For example, consider the case of "annotations":

Before:

before2

After:

after2

But I think we could find a solution: what if we use GET for contactAction () and GET + POST for processContactAction()?

@weaverryan
Copy link
Member

I hadn't thought about the annotations format, so I agree with Javier. But making the process handle get or post might be confusing since it has the same URL as the above, and so will never match in GET. I'm in favor of actually merging as is, after making all this fuss (the additions actually small - mostly code block).

@wouterj
Copy link
Member

wouterj commented Jun 25, 2015

What about using [POST, PUT] (for instance) for processContactForm?

@weaverryan
Copy link
Member

Oh, that's a cool idea - I can't believe I didn't think of that! +1 for making the previous example use Post and Put instead of this new section. @xelaris can you make that change?

Thanks!

@xabbuh
Copy link
Member

xabbuh commented Jun 25, 2015

Oh yeah, that's indeed a great idea @wouterj.

@xelaris xelaris force-pushed the routing-multi-method branch from 6f854f9 to 29dbb8d Compare July 5, 2015 19:08
@xelaris
Copy link
Contributor Author

xelaris commented Jul 5, 2015

I like the idea of changing the existing example to demonstrate how to match multiple methods and I changed the PR as suggested by @wouterj. But I have two points about the current state of this PR:

  • I noticed that the example doesn't work well with the best practices, as it uses two actions to display and process the form. This was actually a reason for the first version of this PR.
  • I'm in doubt if PUT is a suitable method to send a contact form.

I'm +1 with @javiereguiluz suggestion to change the processContactAction() method to match GET and POST. This would kill two birds with one stone: Demonstrate how to match multiple routes and fulfill the best practise.
To make the example more consistent then, I suggest to rename processContactAction to contactFormAction and use a comment like // ... display and process a contact form and to change contactAction to newsAction with route /news or something and use a comment like // ... display your news.

What are your opinion about this?

@xabbuh
Copy link
Member

xabbuh commented Jul 7, 2015

@xelaris Good point with the best practices. But wouldn't we then don't have an example with only one allowed method when choosing your solution?

@xelaris
Copy link
Contributor Author

xelaris commented Jul 7, 2015

@xabbuh The newsAction with route /newswould match only GET.

@xabbuh
Copy link
Member

xabbuh commented Jul 7, 2015

Good point, looks like a good idea then.

@xelaris xelaris force-pushed the routing-multi-method branch from 29dbb8d to c1e8453 Compare July 15, 2015 20:46
@xelaris
Copy link
Contributor Author

xelaris commented Jul 15, 2015

I have changed the PR to apply the latest proposal. What do you think about it?

@xabbuh
Copy link
Member

xabbuh commented Jul 15, 2015

👍 from me

@weaverryan
Copy link
Member

LOVE it. Thanks @xelaris - you rock for your patience and ideas!

@weaverryan weaverryan merged commit c1e8453 into symfony:2.3 Jul 16, 2015
weaverryan added a commit that referenced this pull request Jul 16, 2015
… methods (xelaris)

This PR was merged into the 2.3 branch.

Discussion
----------

[Book][Routing] Add example about how to match multiple methods

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

Commits
-------

c1e8453 [Book][Routing] Change example to match multiple methods
@xelaris xelaris deleted the routing-multi-method branch October 31, 2022 12:17
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.

5 participants