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

Fixed deprecated _method errors #1054

Merged
merged 2 commits into from Jun 4, 2015

Conversation

Padam87
Copy link
Contributor

@Padam87 Padam87 commented Jun 2, 2015

Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #1051
License MIT


if (isset($annoRequirements['_method'])) {
Copy link
Member

Choose a reason for hiding this comment

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

there is another place using _method in this file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@lsmith77 lsmith77 added this to the 1.7.0 milestone Jun 2, 2015

// add route to collection
$route = new Route(
$path, $defaults, $requirements, $options, $host, array(), $methods, $condition
$path, $defaults, array(), $options, $host, array(), $methods, $condition
Copy link
Member

Choose a reason for hiding this comment

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

this is wrong. Requirements should still be set in the route

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

not always. There is code setting the _format requirement a few lines above

@Padam87
Copy link
Contributor Author

Padam87 commented Jun 2, 2015

Also fixed the method -> methods problem.

@Padam87
Copy link
Contributor Author

Padam87 commented Jun 3, 2015

bump

lsmith77 added a commit that referenced this pull request Jun 4, 2015
Fixed deprecated _method errors
@lsmith77 lsmith77 merged commit 8b87549 into FriendsOfSymfony:master Jun 4, 2015
@lsmith77 lsmith77 removed the in review label Jun 4, 2015
@Padam87
Copy link
Contributor Author

Padam87 commented Jun 4, 2015

Awesome, thank you!

@Padam87 Padam87 deleted the deprecated-fix branch June 4, 2015 09:16
$requirements['_method'] = $this->getMethod();
$this->setRequirements($requirements);

$this->setMethods((array) $this->getMethod());
Copy link
Member

Choose a reason for hiding this comment

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

I guess this change is problematic, since we override the method, without checking if something was set before .. ?

Copy link
Member

Choose a reason for hiding this comment

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

14:53 <Zerrvox> lsmith: FosRestBundle 1.7 has a BC break https://github.com/FriendsOfSymfony/FOSRestBundle/blob/bd233aca63b0965171d60fe4c539afa10d45633e/Controller/Annotations/Route.php
14:53 <lsmith> Zerrvox: what breaks?
14:55 <Zerrvox> If you have a route annotation in a FOSRest Controller the method of the generated route ends up being null
14:55 <Zerrvox> which cannot be loaded

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It shouldn't be a BC break.
(array) null ends up being an empty [], which is is the default value for the methods property, and this code is in the constructor.

Although it might be a problem, tested it with both 1.6 and 1.7, and the methods part is definitely overwritten somewhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants