WIP [Routing] add scheme and method route definition option #6049

Merged
merged 4 commits into from Jan 15, 2013

Conversation

Projects
None yet
6 participants
Member

Tobion commented Nov 18, 2012

fixes #5990
BC break: no
Deprecations: specifying _scheme and _method in the requirements section has been deprecated. you should use the dedicated schemes and methods config options instead. for BC both possibilities are synchronized.
Before

article_edit:
    pattern: /article/{id}}
    requirements: { '_method': 'POST|PUT', '_scheme': 'https', 'id': '\d+' }

After

article_edit:
    pattern: /article/{id}}
    methods: [POST, PUT]
    schemes: https
    requirements: { 'id': '\d+' }

Before

<route id="article_edit" pattern="/article/{id}}">
    <requirement key="_method">POST|PUT</requirement>
    <requirement key="_scheme">https</requirement>
    <requirement key="id">\d+</requirement>
</route>

After

<route id="article_edit" pattern="/article/{id}}" methods="POST PUT" schemes="https">
    <requirement key="id">\d+</requirement>
</route>

WIP

src/Symfony/Component/Routing/Route.php
@@ -98,6 +122,8 @@ public function unserialize($data)
$this->defaults = $data['defaults'];
$this->requirements = $data['requirements'];
$this->options = $data['options'];
+ $this->schemes = $data['schemes'];
+ $this->methods = $data['options'];
@pborreli

pborreli Nov 18, 2012

Contributor

$data['options'] => $data['methods'] ?

@Tobion

Tobion Nov 19, 2012

Member

yeah, thx

src/Symfony/Component/Routing/RouteCollection.php
+ */
+ public function setSchemes($schemes)
+ {
+ foreach ($this->routes as $name => $route) {
@stloyd

stloyd Nov 19, 2012

Contributor

$name is never used. Same in loop below.

src/Symfony/Component/Routing/Route.php
+ {
+ $this->schemes = array_map('strtolower', (array) $schemes);
+
+ // this is too keep BC and will be removed in a future version
@lmcd

lmcd Nov 21, 2012

Contributor

too = to

Member

Tobion commented Dec 12, 2012

@fabpot: Instead of the current xml attributes methods and schemes (see above), I implemented this xml syntax first:

<route id="article_edit" pattern="/article/{id}}">
    <method>POST</method>
    <method>PUT</method>
    <scheme>https</scheme>
</route>

But I think it's not that good as you need to specify multiple elements without key.
And when importing and overwriting routes so that there is no method restriction, you would need to add a single empty method element:

<import resource="file.xml">
    <method></method>
</route>

So I think a whitespace separated attribute list is the better choice. There is also an XSD type for such things (see implementation xsd:list).

Contributor

mvrhov commented Dec 12, 2012

@Tobion I'd go with comma separated list of methods and schemes in xml and strip the spaces. BTW I'm pretty sure that schemes should be schemas

Member

Tobion commented Dec 12, 2012

Well, I used a whitespace separated list because it seems that this is the standard ways of using lists in XML. Because the xsd:list explicitly expects a whitespace separated list.

Contributor

mvrhov commented Dec 12, 2012

I suggested the comma separated because, I can see myself automatically writing them like that as this is how I usually write lists.
Lets wait for what others think.

Member

Tobion commented Dec 12, 2012

I could implement it so that both GET POST and GET,POST and GET|POST is accepted.

@fabpot fabpot referenced this pull request Jan 14, 2013

Merged

Routing options #6738

fabpot added a commit that referenced this pull request Jan 15, 2013

merged branch fabpot/routing-options (PR #6738)
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.

@fabpot fabpot merged commit e803f46 into symfony:master Jan 15, 2013

1 check passed

default The Travis build passed
Details

@Tobion Tobion deleted the Tobion:scheme-method-option branch Jan 15, 2013

ondrejmirtes pushed a commit to ondrejmirtes/symfony that referenced this pull request Nov 25, 2013

merged branch Tobion/scheme-method-def-tolerance (PR #7268)
This PR was merged into the 2.2 branch.

Commits
-------

54c333d [Routing] unify and fix the loader tests
41ad9d8 [Routing] make xml loader more tolerant

Discussion
----------

[Routing] make xml loader more tolerant

schemes and methods may also be delimited by whitespace, comma or pipe.
Fixes symfony#6049 (comment)
this eases migration as now `methods="GET|POST"` also works
the second commit unifies the tests and fixes some strange assertions that were useless

| Q             | A
| ------------- | ---
| Bug fix?      | [yes]
| New feature?  | [yes but not really]
| BC breaks?    | [no]
| Deprecations? | [no]
| Tests pass?   | [yes]
| License       | MIT
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment