Skip to content
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.

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

[DependencyInjection] Fixed resolving of service configurators containing Definition objects #14835

Merged
merged 1 commit into from Jun 11, 2015

Conversation

Projects
None yet
4 participants
@webmozart
Copy link
Contributor

commented Jun 2, 2015

Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #14834
License MIT
Doc PR -
if (is_array($callable)) {
$callable[0] = $callable[0] instanceof Reference ? $this->get((string) $callable[0]) : $parameterBag->resolveValue($callable[0]);
}
$callable = $this->resolveServices($parameterBag->resolveValue($callable));

This comment has been minimized.

Copy link
@stof

stof Jun 2, 2015

Member

this now allows expressions too, which looks wrong to me

This comment has been minimized.

Copy link
@webmozart

webmozart Jun 3, 2015

Author Contributor

why not be consistent with the rest?

This comment has been minimized.

Copy link
@stof

stof Jun 3, 2015

Member

Expressions are not supported in configurators in other places. They are only supported in arguments. So you don't introduce consistency but inconsistency (and using an expression for the service instance of the callable does not make sense either btw)

This comment has been minimized.

Copy link
@xabbuh

xabbuh Jun 9, 2015

Member

I agree with @stof.

This comment has been minimized.

Copy link
@webmozart

webmozart Jun 10, 2015

Author Contributor

Alright. What solution do you propose then? Duplicate resolveServices() and remove expression support from the duplicate? Any naming suggestions?

This comment has been minimized.

Copy link
@stof

stof Jun 10, 2015

Member

well, we don't need to support the case of arrays either for configurators first element. So this is just a matter of adding the support of Definition object in the current code (which already supports references)

This comment has been minimized.

Copy link
@webmozart

webmozart Jun 11, 2015

Author Contributor

Yes, but that means we still have duplicated and potentially buggy code. If somebody adds functionality to resolveServices(), they need to think about changing this code too (like happened before). IMO this should be fixed.

@webmozart webmozart force-pushed the webmozart:issue14834 branch from 796f5c6 to 6ebcddd Jun 11, 2015

@webmozart

This comment has been minimized.

Copy link
Contributor Author

commented Jun 11, 2015

I updated the PR to match @stof's comments. I don't like this solution however, because the duplicated code in createService() and resolvedServices() bears potential for new bugs.

@fabpot

This comment has been minimized.

Copy link
Member

commented Jun 11, 2015

👍

@fabpot

This comment has been minimized.

Copy link
Member

commented Jun 11, 2015

Thank you @webmozart.

@fabpot fabpot merged commit 6ebcddd into symfony:2.7 Jun 11, 2015

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details

fabpot added a commit that referenced this pull request Jun 11, 2015

bug #14835 [DependencyInjection] Fixed resolving of service configura…
…tors containing Definition objects (webmozart)

This PR was merged into the 2.7 branch.

Discussion
----------

[DependencyInjection] Fixed resolving of service configurators containing Definition objects

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

Commits
-------

6ebcddd [DependencyInjection] Fixed resolving of service configurators containing Definition objects
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.