Skip to content

[2.2][Routing] Added support for default attributes with default values of method params #5904

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 4 commits into from
Closed

[2.2][Routing] Added support for default attributes with default values of method params #5904

wants to merge 4 commits into from

Conversation

lyrixx
Copy link
Member

@lyrixx lyrixx commented Nov 3, 2012

Bug fix: no
Feature addition: yes
Backwards compatibility break: no
Symfony2 tests pass: yes

With this patch, you can configure your default values likes this:

/**
 * @Route("/hi/{name}", name="hi")
 */
public function hiAction($name = "Bob")
{
    return new Response($name);
}

@Tobion
Copy link
Contributor

Tobion commented Nov 3, 2012

I'm unsure. How does one know if that param defines a default value or a requirement? It's too vague.

@lyrixx
Copy link
Member Author

lyrixx commented Nov 3, 2012

It's only a default value, not a requirement.
It's just a shortcut to avoid defaults={"name"="bob"}

@Tobion
Copy link
Contributor

Tobion commented Nov 3, 2012

Yes, but its not clear. It could also be a shortcut to requirements={"name"="bob"}, which has totally different meaning. So it's not self-explanatory.
-1 for me.

@lyrixx
Copy link
Member Author

lyrixx commented Nov 3, 2012

it is the default php behavior. It's a default value for a variable...

@stof
Copy link
Member

stof commented Nov 4, 2012

@Tobion using the default value of the method to set a requirement does not make any sense. I don't see why someone would expect this behavior

@fabpot
Copy link
Member

fabpot commented Nov 6, 2012

@lyrixx Can you add some unit tests?

@Tobion
Copy link
Contributor

Tobion commented Nov 6, 2012

Oh I misunderstood the PR. I thought this makes the name param default to hi. @Route("/hi/{name}", name="hi"). But it's just the name of the route. Your example was easy to misinterpret as you used name everywhere.

Tobion
Tobion reviewed Nov 6, 2012
View reviewed changes
src/Symfony/Component/Routing/Loader/AnnotationClassLoader.php
@@ -149,6 +149,11 @@ protected function addRoute(RouteCollection $collection, $annot, $globals, \Refl
}

$defaults = array_merge($globals['defaults'], $annot->getDefaults());
foreach ($method->getParameters() as $param) {
if ($param->isOptional()) {
$defaults[$param->getName()] = $param->getdefaultvalue();
Copy link
Contributor

Choose a reason for hiding this comment

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

getDefaultValue()

@fabpot
Copy link
Member

fabpot commented Nov 10, 2012

@lyrixx Can you finish this PR?

@lyrixx
Copy link
Member Author

lyrixx commented Nov 10, 2012

@fabpot Yes i will as soon as possible.

@lyrixx
Copy link
Member Author

lyrixx commented Nov 10, 2012

I rebase and amend my commit. (I changed doc in commit message to be less confusing)

I will try to add tests.
But for now, AnnotationClassLoader::load is not really tested, and AnnotationClassLoader::addRoute is absolutely not tested. So I think I should add tests for these methods ? And then add tests for my patch.
I will try tomorrow.

@lyrixx
[Routing] Added one more test fixture
cb68c7d
@lyrixx
[Routing] Added more tests on AnnatationClassLoader
ba72734
@lyrixx
[Routing] Added support for default attributes with default values of…
e6c592f 
… method params

You can configure default values likes this:

``` php
/**
 * @route("/hi/{username}", name="hi")
 */
public function hiAction($username = "Bob")
{
    return new Response($username);
}
```
@lyrixx
[Routing] Added more tests for default attributes
3988354
@lyrixx
Copy link
Member Author

lyrixx commented Nov 11, 2012

@fabpot I added new tests. I tried to made very atomic commits.

@fabpot fabpot closed this in b099460 Nov 12, 2012
@lyrixx lyrixx mentioned this pull request Nov 13, 2012
Merged
@Tobion Tobion mentioned this pull request Apr 20, 2013
Closed
lyrixx pushed a commit to lyrixx/SensioFrameworkExtraBundle that referenced this pull request Jan 10, 2014
@fabpot
merged branch lyrixx/doc-command-default-value (PR sensiolabs#172)
e6a189f 
This PR was merged into the master branch.

Commits
-------

79d7839 [Doc] Added more doc about default value
06e22ab [Doc] Be more consistant on routing example

Discussion
----------

Doc command default value

see symfony/symfony#5904
henrikbjorn pushed a commit to henrikbjorn/ParamConverter that referenced this pull request Oct 14, 2014
@lyrixx
[Doc] Added more doc about default value
79d7839 
see symfony/symfony#5904
@fabpot fabpot mentioned this pull request Jan 19, 2017
Merged
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
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@lyrixx @Tobion @stof @fabpot