Skip to content

[Hackday][2.7] Added deprecated notes to '_method' and '_scheme' route requirements #12719

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
wants to merge 1 commit into from
Closed

[Hackday][2.7] Added deprecated notes to '_method' and '_scheme' route requirements #12719

wants to merge 1 commit into from

Conversation

dcsg
Copy link
Member

@dcsg dcsg commented Nov 29, 2014

Q A
Bug fix? no
New feature? no
BC breaks? no
Deprecations? yes
Tests pass? no
Fixed tickets [#12641, #12640]
License MIT
Doc PR

@dcsg
Copy link
Member Author

dcsg commented Nov 29, 2014

This one is tricky I decided to put the deprecations in YamlLoader and XmlLoader due to have it on stack trace to the user know what file is "broken". Also I added it in Route class because Plain PHP will instantiate it directly.

This brokes a lot of tests, so now I need to know how do you want me to fix them. I can remove the _scheme and _method from those tests that are failing since I am adding specific tests for this.

@xabbuh
Copy link
Member

xabbuh commented Nov 29, 2014

@dcsg The PHPUnit configuration will be modified to don't let tests fail for the E_USER_DEPRECATED error level (see #12705).

@dcsg
Copy link
Member Author

dcsg commented Nov 29, 2014

Thanks for the info @xabbuh ;)

timglabisch
timglabisch reviewed Nov 29, 2014
View reviewed changes
src/Symfony/Component/Routing/Loader/XmlFileLoader.php
@@ -131,6 +131,13 @@ protected function parseRoute(RouteCollection $collection, \DOMElement $node, $p

list($defaults, $requirements, $options, $condition) = $this->parseConfigs($node, $path);

if (isset($requirements['_method']) || isset($requirements['_scheme'])) {
trigger_error(
'The router \'_method\' and \'_scheme\' requirements was removed. You should use the new \'methods\' and \'schemes\' settings instead.',
Copy link

Choose a reason for hiding this comment

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

please format it this way:

symfony/src/Symfony/Component/HttpKernel/Debug/ErrorHandler.php

Line 14 in c6cc3eb

trigger_error('Symfony\Component\HttpKernel\Debug\ErrorHandler is deprecated since version 2.3 and will be removed in 3.0. Use the same class from the Debug component instead.', E_USER_DEPRECATED);

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok

Copy link
Member

Choose a reason for hiding this comment

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

the requirements are not removed, they are deprecated

@fabpot fabpot added the Routing label Dec 7, 2014
stof
stof reviewed Jan 3, 2015
View reviewed changes
src/Symfony/Component/Routing/Route.php
@@ -86,6 +86,13 @@ class Route implements \Serializable
*/
public function __construct($path, array $defaults = array(), array $requirements = array(), array $options = array(), $host = '', $schemes = array(), $methods = array(), $condition = '')
{
if (isset($requirements['_method']) || isset($requirements['_scheme'])) {
Copy link
Member

Choose a reason for hiding this comment

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

this should be done in setRequirements actually (given that the constructor calls it)

@stof
Copy link
Member

stof commented Jan 3, 2015

A few things to change here IMO:

  • the XML and Yaml file loaders should convert these requirements to the new format before setting them in the Route, to avoid triggering a second deprecation warning in the Route object.
  • tests which are ensuring that the BC layer works should be moved to Legacy* test cases ignoring the deprecation warning by using $this->iniSet('error_reporting', -1 & ~E_USER_DEPRECATED) in the setUp method
  • tests which are ensuring the behavior of the scheme and method requirements themselves should use the new API for them to avoid triggering deprecation warnings.

@dcsg can you update your PR ?

@dcsg
Copy link
Member Author

dcsg commented Jan 6, 2015

@stof I am a little busy this week, but I can try update it on weekend. Is that ok?

@stof
Copy link
Member

stof commented Jan 6, 2015

yes it is

@Tobion
Copy link
Contributor

Tobion commented Jan 11, 2015

@dcsg I'm finishing this PR.

@Tobion Tobion mentioned this pull request Jan 11, 2015
Merged
@Tobion
Copy link
Contributor

Tobion commented Jan 11, 2015

Closing in favor of #13361 which adds the triggers at different places.

@Tobion Tobion closed this Jan 11, 2015
@dcsg dcsg deleted the 2.7 branch January 11, 2015 15:54
@dcsg
Copy link
Member Author

dcsg commented Jan 11, 2015

Ok thanks and sorry for the delay

@Tobion
Copy link
Contributor

Tobion commented Jan 11, 2015

Thanks for your initiative to start this.

Tobion added a commit that referenced this pull request Jan 13, 2015
@Tobion
feature #13361 [Routing] apply deprecation triggers and fix tests (To…
9f85517 
…bion)

This PR was merged into the 2.7 branch.

Discussion
----------

[Routing] apply deprecation triggers and fix tests

| Q             | A
| ------------- | ---
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | [#12641, #12640, #12719]
| License       | MIT
| Doc PR        |

Correctly add deprecation for _scheme and _method requirements and fix code to not trigger deprecations itself. And fix test so that they either explicitly test deprecated functionality or alternatively refactor tests to use new style.

It also fixes the deprecation triggers for "pattern" which was not correctly done for YAML/XML loading.

Commits
-------

9af0ff2 [FrameworkBundle] fix routing container param resolving to not access deprecated requirements while maintaining BC
bd91867 [FrameworkBundle] remove superfluous test that is already covered in routing
bc1c5c8 [Routing] apply deprecation triggers and fix tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Routing
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@dcsg @xabbuh @stof @Tobion @timglabisch @fabpot