10000 [Routing] Annotated routes with a variadic parameter by wdalmut · Pull Request #13690 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[Routing] Annotated routes with a variadic parameter #13690

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

[Routing] Annotated routes with a variadic parameter #13690

wants to merge 1 commit into from

Conversation

wdalmut
Copy link
Contributor
@wdalmut wdalmut commented Feb 14, 2015

I add a test the should show a problem that occurs if you try to use variadics with annotated routes.

I don't know if anybody could be interested in this issue. But if you try to use annotated routes with variadics functions, php raise a fatal error like:

1) Symfony\Component\Routing\Tests\Loader\AnnotationFileLoaderTest::testLoadVariadic
ReflectionException: Internal error: Failed to retrieve the default value

/data/src/Symfony/Component/Routing/Loader/AnnotationClassLoader.php:143
/data/src/Symfony/Component/Routing/Loader/AnnotationClassLoader.php:125
/data/src/Symfony/Component/Routing/Loader/AnnotationFileLoader.php:65
/data/src/Symfony/Component/Routing/Tests/Loader/AnnotationFileLoaderTest.php:48
Q A
Bug fix? yes
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? no
Fixed tickets
License MIT
Doc PR

I get this error playing around with symfony2 components not in a sf2 project.
In particular using:

$routeLoader = new RouteAnnotationClassLoader($reader);
$loader = new AnnotationDirectoryLoader(new FileLocator([__DIR__.'/../app']), $routeLoader);
$routes = $loader->load(__DIR__.'/../app');

and annotated routes like

class My
{
  /**
   * @Route("/path/{id}/other/{other}")
   */
  public function aRoute(...$params)
  {
    //...
  }
}

@wdalmut wdalmut force-pushed the hotfix/annotation-with-variadic branch 3 times, most recently from 5646ab1 to 439215f Compare February 14, 2015 13:57
@wdalmut
Annotated routes with a variadic parameter
5799947 
There are no variadic default values and that generate a fatal error.
@wdalmut wdalmut force-pushed the hotfix/annotation-with-variadic branch from 439215f to 5799947 Compare February 14, 2015 13:59
@wdalmut wdalmut changed the title Annotated routes with a variadic parameter [Routing] Annotated routes with a variadic parameter Feb 15, 2015
@fabpot fabpot added the Routing label Feb 25, 2015
@stof stof mentioned this pull request Aug 1, 2015
Merged
@stof
Copy link
Member
stof commented Aug 1, 2015

I cherry-picked your commit implementing this test in #15425 when implementing the fix

@stof stof mentioned this pull request Aug 1, 2015
Merged
Tobion added a commit tha 8000 t referenced this pull request Aug 2, 2015
@Tobion
bug #15425 [Routing] Fix the retrieval of the default value for varia…
e5909be 
…dic arguments in the annotation loader (wdalmut, stof)

This PR was merged into the 2.3 branch.

Discussion
----------

[Routing] Fix the retrieval of the default value for variadic arguments in the annotation loader

| Q             | A
| ------------- | ---
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #13690
| License       | MIT
| Doc PR        | n/a

This takes the test submitted in #13690 and implements the fix for this bug

Commits
-------

73c5eff Fix the retrieval of the default value for variadic arguments
9b7d4c7 Annotated routes with a variadic parameter
@xabbuh xabbuh closed this Aug 2, 2015
@wdalmut
Copy link
Contributor Author
wdalmut commented Aug 2, 2015

👍 ;)

@wdalmut wdalmut mentioned this pull request Aug 2, 2015
Merged
fabpot added a commit that referenced this pull request Aug 9, 2015
@fabpot
bug #15426 [Serializer] Add support for variadic arguments in the Get…
620a3d4 
…SetNormalizer (stof)

This PR was merged into the 2.3 branch.

Discussion
----------

[Serializer] Add support for variadic arguments in the GetSetNormalizer

| Q             | A
| ------------- | ---
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | n/a
| License       | MIT
| Doc PR        | n/a

There were 2 broken cases:

- when the value was passed, the array was passed as argument, becoming the first value of the variadic array. The array needs to be spread into multiple arguments when calling the method
- when the value was missing, the code would throw a ReflectionException, similar to the issue reported in #13690, because a variadic argument is optional but does not have a default value

Commits
-------

704760b Add support for variadic arguments in the GetSetNormalizer
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.
4 participants
@wdalmut @stof @fabpot @xabbuh
0