Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

[2.2] [FrameworkBundle] avoid trying to resolve container placeholders on arrays on router _defaults #4995

Merged
merged 1 commit into from Oct 5, 2012

Conversation

Projects
None yet
4 participants
Contributor

docteurklein commented Jul 20, 2012

Bug fix: yes
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: ~
Todo: ~
License of the code: MIT
Documentation PR: ~

Permits to pass arrays in route _defaults.

Member

stof commented Jul 20, 2012

This seems weird. An array could contain parameters in it.

Contributor

docteurklein commented Jul 20, 2012

@stof An object too then, no ? Why accepting objects but not arrays ? Would you propose to recursively resolve array values ?

Member

stof commented Jul 20, 2012

@docteurklein Resolving array values recursively would be consistent with the way the DIC parameters are resolved. I don't really see how you would resolve objects (and btw, it is pretty much an edge case as you cannot really put an object in your routes if you define them in your YAML or XML config files or with annotations)

Contributor

docteurklein commented Jul 20, 2012

@stof I agree. I can manage recursive array resolving if needed.

@stof stof and 1 other commented on an outdated diff Jul 20, 2012

src/Symfony/Bundle/FrameworkBundle/Routing/Router.php
@@ -114,7 +114,13 @@ private function resolveString($value)
{
$container = $this->container;
- if (null === $value || false === $value || true === $value || is_object($value)) {
+ if (is_array($value)) {
+ foreach ($value as $key => $val) {
+ $value[$key] = $this->resolveString($val);
+ }
@stof

stof Jul 20, 2012

Member

I would add a return $value after the foreach loop instead of changing the next condition to make it check arrays too. It would be more readable IMO as you would see directly what the array handling would be

@docteurklein

docteurklein Jul 20, 2012

Contributor

Thanks, fixed.

@docteurklein docteurklein commented on the diff Jul 20, 2012

src/Symfony/Bundle/FrameworkBundle/Routing/Router.php
@@ -114,6 +114,13 @@ private function resolveString($value)
{
$container = $this->container;
+ if (is_array($value)) {
+ foreach ($value as $key => $val) {
+ $value[$key] = $this->resolveString($val);
+ }
@docteurklein

docteurklein Jul 20, 2012

Contributor

I miss a space here, do I ?

@stloyd

stloyd Jul 20, 2012

Contributor

Yep, an new line is missing =)

@docteurklein

docteurklein Jul 20, 2012

Contributor

fixed.

Owner

fabpot commented Jul 23, 2012

Can you squash your commits before I merge? Thanks.

Contributor

docteurklein commented Jul 23, 2012

@fabpot done.

fabpot added a commit that referenced this pull request Oct 5, 2012

merged branch docteurklein/fix-router-resolve-string (PR #4995)
This PR was merged into the master branch.

Commits
-------

4b86765 [FrameworkBundle] recursively resolve container parameter placeholders for arrays in router _defaults

Discussion
----------

[2.2] [FrameworkBundle] avoid trying to resolve container placeholders on arrays on router _defaults

Bug fix: yes
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: ~
Todo: ~
License of the code: MIT
Documentation PR: ~

Permits to pass arrays in route `_defaults`.

---------------------------------------------------------------------------

by stof at 2012-07-20T13:07:36Z

This seems weird. An array could contain parameters in it.

---------------------------------------------------------------------------

by docteurklein at 2012-07-20T13:17:00Z

@stof An object too then, no ? Why accepting objects but not arrays ? Would you propose to recursively resolve array values ?

---------------------------------------------------------------------------

by stof at 2012-07-20T13:31:06Z

@docteurklein Resolving array values recursively would be consistent with the way the DIC parameters are resolved. I don't really see how you would resolve objects (and btw, it is pretty much an edge case as you cannot really put an object in your routes if you define them in your YAML or XML config files or with annotations)

---------------------------------------------------------------------------

by docteurklein at 2012-07-20T13:36:43Z

@stof I agree. I can manage recursive array resolving if needed.

---------------------------------------------------------------------------

by fabpot at 2012-07-23T13:58:07Z

Can you squash your commits before I merge? Thanks.

---------------------------------------------------------------------------

by docteurklein at 2012-07-23T14:39:17Z

@fabpot  done.

@fabpot fabpot merged commit 4b86765 into symfony:master Oct 5, 2012

@docteurklein docteurklein deleted the docteurklein:fix-router-resolve-string branch Sep 8, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment