[Routing] Fix BC break in AnnotationClassLoader defaults attributes handling #21379

chalasr · Jan 23, 2017

Q	A
Branch?	2.7
Bug fix?	no
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	`77289b9`#commitcomment-20572462
License	MIT
Doc PR	n/a

This fixes a BC break introduced in #21333. Instead of removing the automatic request attributes creation, we keep it but only for attributes that are mandatory (i.e. present in the route path).

Thanks to @iltar for the idea.

chalasr · Jan 23, 2017

We could still deprecate this feature in 3.3 as it's inconsistent with other loaders, properly defining attributes is always better IMHO.

linaori · Jan 23, 2017

Can you add a test-case with the (Request $request = null) and a test with a controller like this (via WebTestCase) to prevent regressions?

What to do about the feature regarding defaults?

it should be kept as is
it should be removed
it should be added to the other loaders as well

chalasr · Jan 23, 2017

What to do about the feature regarding defaults?

IMHO it should be removed, at least not kept as is, switching between two config formats for routing should not break an app.

Can you add a test-case with the (Request $request = null) and a test with a controller like this (via WebTestCase) to prevent regressions?

Sure!
Status: needs work

stof · Jan 23, 2017

IMHO it should be removed, at least not kept as is, switching between two config formats for routing should not break an app.

Removing it makes sense, but it must be done using a deprecation first (so deprecated in 3.3 and removed in 4.0)

chalasr · Jan 23, 2017

Functional test added. Hope the fix is good, it was not that easy 😄

Status: needs review

fabpot · Jan 23, 2017

Not sure we should deprecate the feature, I think it's a nice one.

chalasr · Jan 23, 2017

I can understand. I'll look at adding it to other formats on 3.3 if you agree

chalasr · Jan 23, 2017

...Bundle/FrameworkBundle/Tests/Functional/Bundle/TestBundle/Controller/AnnotatedController.php

+use Symfony\Component\HttpFoundation\Response;
+use Symfony\Component\Routing\Annotation\Route;
+
+class AnnotatedController extends ContainerAware


It doesn't need the container.

…andling

chalasr · Jan 24, 2017

Build failure is normal (high deps only).

fabpot · Jan 24, 2017

Thank you @chalasr.

… attributes handling (chalasr) This PR was merged into the 2.7 branch. Discussion ---------- [Routing] Fix BC break in AnnotationClassLoader defaults attributes handling | Q | A | ------------- | --- | Branch? | 2.7 | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | 77289b9#commitcomment-20572462 | License | MIT | Doc PR | n/a This fixes a BC break introduced in #21333. Instead of removing the automatic request attributes creation, we keep it but only for attributes that are mandatory (i.e. present in the route path). Thanks to @iltar for the idea. Commits ------- 1d298f0 [Routing] Fix BC break in AnnotationClassLoader defaults attributes handling

fabpot · Feb 1, 2017

This is still a BC break as demonstrated by the tests of FrameworkExtraBundle: https://travis-ci.org/sensiolabs/SensioFrameworkExtraBundle/jobs/197153674

@chalasr Can you have a look at this?

linaori · Feb 1, 2017

    /**
     * @Route("/simple/multiple/{a}/{b}/")
     * @Template("@Foo/Simple/some.html.twig")
     */
    public function someMoreAction($a, $b, $c = 'c')
    {
    }

expectation is henk, bar, c as {c} is not provided. Result is henk, bar, . $c appears to be empty while it should get c as value.

chalasr · Feb 1, 2017

I'll look into it.

chalasr · Feb 1, 2017

Reproduced on our test case in 2.7 and 3.3 (master...chalasr:reproduce-sensioextra-test) all is fine for argument resolving.

The bottleneck seems to be in the Template annotation handling which relies on request attributes to resolve template vars: https://github.com/sensiolabs/SensioFrameworkExtraBundle/blob/3.0/EventListener/TemplateListener.php#L153.
Since we no longer set request attributes for arguments that are not expected in the route path (because it was buggy), the test is failing.
I think it should be solved on the SensioFrameworkExtraBundle by making it inject vars from both request attributes and method argument defaults.
Let me know what do you think, anyway I'll open a PR on SensioFrameworkExtraBundle later today so you can judge.

chalasr · Feb 1, 2017

@fabpot See sensiolabs/SensioFrameworkExtraBundle#456

This PR was merged into the 3.0 branch. Discussion ---------- Fix default vars resolution for Template annotation Fixes broken `@Template` default vars resolution which was relying on a buggy behavior fixed symfony/symfony#21379 (the annotation class loader no longer sets request attributes for any argument that has a default value, but only for ones which may be expected in the route path). Commits ------- 9ddf58a Fix default vars resolution for Template annotation

This was referenced Jan 23, 2017

[Routing] Mention AnnotationClassLoader BC break in changelog #21377

Closed

[WCM][Routing] Update default attributes example for annotations symfony/symfony-docs#7408

Closed

carsonbot added the Status: Needs Work label Jan 23, 2017

chalasr force-pushed the routing/fix-bc-break branch 7 times, most recently from 33e78ce to de10ea6 Compare January 23, 2017 20:10

carsonbot added Status: Needs Review and removed Status: Needs Work labels Jan 23, 2017

chalasr commented Jan 23, 2017

View reviewed changes

chalasr force-pushed the routing/fix-bc-break branch from de10ea6 to e3eb006 Compare January 23, 2017 20:35

[Routing] Fix BC break in AnnotationClassLoader defaults attributes h…

1d298f0

…andling

chalasr force-pushed the routing/fix-bc-break branch from e3eb006 to 1d298f0 Compare January 23, 2017 20:38

nicolas-grekas added this to the 2.7 milestone Jan 24, 2017

fabpot merged commit 1d298f0 into symfony:2.7 Jan 24, 2017

chalasr deleted the routing/fix-bc-break branch January 24, 2017 18:50

chalasr mentioned this pull request Feb 1, 2017

Fix default vars resolution for Template annotation sensiolabs/SensioFrameworkExtraBundle#456

Merged

chalasr mentioned this pull request Feb 9, 2017

[ParamConverter] Unable to guess how to get a Doctrine instance from the request information #21563

Closed

Search code, repositories, users, issues, pull requests...

Uh oh!

[Routing] Fix BC break in AnnotationClassLoader defaults attributes handling #21379

[Routing] Fix BC break in AnnotationClassLoader defaults attributes handling #21379

Uh oh!

Conversation

chalasr commented Jan 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chalasr commented Jan 23, 2017

Uh oh!

linaori commented Jan 23, 2017

Uh oh!

chalasr commented Jan 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stof commented Jan 23, 2017

Uh oh!

chalasr commented Jan 23, 2017

Uh oh!

fabpot commented Jan 23, 2017

Uh oh!

chalasr commented Jan 23, 2017

Uh oh!

chalasr Jan 23, 2017

Choose a reason for hiding this comment

Uh oh!

chalasr commented Jan 24, 2017

Uh oh!

fabpot commented Jan 24, 2017

Uh oh!

fabpot commented Feb 1, 2017

Uh oh!

linaori commented Feb 1, 2017

Uh oh!

chalasr commented Feb 1, 2017

Uh oh!

chalasr commented Feb 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chalasr commented Feb 1, 2017

Uh oh!

Uh oh!

chalasr commented Jan 23, 2017 •

edited

Loading

chalasr commented Jan 23, 2017 •

edited

Loading

chalasr commented Feb 1, 2017 •

edited

Loading