8000 Consider making _scheme and _method a config option · Issue #5990 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

Consider making _scheme and _method a config option #5990

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.

Sign up for GitHub

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

Jump to bottom
Closed
Tobion opened this issue Nov 12, 2012 · 6 comments · Fixed by #6049
Closed

Consider making _scheme and _method a config option #5990

Tobion opened this issue Nov 12, 2012 · 6 comments · Fixed by #6049
Labels
Routing

Comments

@Tobion
Copy link
Contributor
Tobion commented Nov 12, 2012

Currently configuring the _scheme and _method requirement in routes is pretty strange and also wrong in some cases. Current example:

route_name:
    pattern: /hello/{slug}
    hostname_pattern: {locale}.example.com
    requirements:
        locale: en|fr
        slug: \w+
        _scheme: HTTPS
        _method: GET|POST
    defaults:
        _controller: Foo:bar:baz

The problems are:

  1. variables in the requirement section reference placeholders. Except for _scheme and _method!
  2. these requirements allow regular expressions. But this is not true for _scheme and _method because many places of the routing component expect a delimited list like GET|POST. So normal regex like .+ does not work for them and breaks code.

My proposal:

route_name:
    pattern: /hello/{slug}
    hostname_pattern: {locale}.example.com
    schemes: HTTPS
    methods: [GET, POST]
    requirements:
        locale: en|fr
        slug: \w+
    defaults:
        _controller: Foo:bar:baz

This makes these scheme and method an option like pattern which is much more consistent as they are on the "same level" as the path pattern because all of them belong to the HTTP request (method + URL). One can also clearly define that not any regex is valid but only a single string or an array of allowed methods.
@Tobion Tobion mentioned this issue Nov 12, 2012
Closed
@Seldaek
Copy link
Member
Seldaek commented Nov 12, 2012

Agreed, then again if we go there we might also say _controller should be top level.

@Tobion
Copy link
Contributor Author
Tobion commented Nov 12, 2012

No, because the routing is independent of MVC and thus has no knowledge about _controller.

@fabpot
Copy link
Member
fabpot commented Nov 12, 2012

One can argue that a route without associated code (the controller) does not make sense.

@pylebecq
Copy link
Contributor

@fabpot What about the routes used by the security (when using the full stack framework) ? Some don't have a controller (I'm thinking of login check and logout of course). Though I think it would be possible to move the _controller to top level like @Seldaek said without making it mandatory, resulting in the same error as when you don't add a _controller in the defaults.

@stof
Copy link
Member
stof commented Nov 15, 2012

I don't see the reason to move _controller in another place. As far as the routing is concerned, it is a variable like all others, even if most of the time it is set in the defaults without being able to be changed by the pattern (but it could eventually be done by someone not bothering at all about SEO and loving the old symfony1 fallback route).
The special meaning of _controller comes from HttpKernel, not from the Routing (and then, we could consider _locale special too as HttpKernel gives it a special meaning too)

@Tobion
Copy link
Contributor Author
Tobion commented Nov 15, 2012

I renamed scheme to schemes and method to methods because they can accept an array (for convenience also a single string). The plural name is consistent with the security, which also names it methods when restricting access.
I'm working on a PR for this.

@Tobion Tobion mentioned this issue Nov 18, 2012
Merged
@fabpot fabpot mentioned this issue Jan 14, 2013
Merged
fabpot added a commit that referenced this issue Jan 15, 2013
@fabpot
merged branch fabpot/routing-options (PR #6738)
89f9b24 
This PR was merged into the master branch.

Commits
-------

9fc7def added the UPGRADE file for Symfony 3.0
e84cad2 [Routing] updated CHANGELOG
65eca8a [Routing] added new schemes and methods options to the annotation loader
5082994 [Routing] renamed pattern to path
b357caf [Routing] renamed hostname pattern to just hostname
e803f46 made schemes and methods available in XmlFileLoader
d374e70 made schemes and methods available in YamlFileLoader
2834e7e added scheme and method setter in RouteCollection
10183de make scheme and method requirements first-class citizen in Route

Discussion
----------

Routing options

| Q             | A
| ------------- | ---
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | no
| Deprecations? | yes
| Tests pass?   | yes
| Fixed tickets | #5989, #5990, #6049
| License       | MIT

In #5989, it has unanimously been decided to renamed `hostname_pattern` to `hostname` and `pattern` to `path`. That makes a lot of sense and I would like to do the renaming now as `hostname_pattern` is new in Symfony 2.2, so I'd like to avoid breaking BC just after the release. As we are modifying the route options, I've also included changes introduced by @Tobion in #6049 which were discussed in #5990.

As everything is BC, I think it's wise to include that in 2.2. What do you think?

---------------------------------------------------------------------------

by Tobion at 2013-01-14T18:25:53Z

I agree it should be done in 2.2. Thanks for working on it.

---------------------------------------------------------------------------

by vicb at 2013-01-14T23:11:12Z

@fabpot "Everything is BC" until it breaks BC in 3.0, that's why I'd like to see [deprecations in PR summary](symfony/symfony-docs#2116) what do you think ?

---------------------------------------------------------------------------

by vicb at 2013-01-14T23:16:40Z

it would also be great to update the CHANGELOG with deprecations (it could also help people answering your question)

---------------------------------------------------------------------------

by fabpot at 2013-01-15T07:07:03Z

@vicb: I've just updated the CHANGELOG and created the UPGRADE file for 3.0.

---------------------------------------------------------------------------

by vicb at 2013-01-15T07:15:32Z

@fabpot thanks.
59FD
@fabpot fabpot closed this as completed Jan 15, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Routing
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

WIP [Routing] add scheme and method route definition option Tobion/symfony
5 participants
@fabpot @Seldaek @pylebecq @stof @Tobion
0