Skip to content

[DI] Fix ServiceLocatorArgument::setValues() for non-reference values #21794

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

Merged
merged 1 commit into from
Feb 28, 2017

Conversation

chalasr
Copy link
Member

@chalasr chalasr commented Feb 28, 2017

Q A
Branch? master
Fixed tickets #21625 (comment)
Tests pass? yes
License MIT

ResolveInvalidReferencesPass calls setValues() with resolved invalid reference (null), the Reference type check should occurs at construction only.

@stof
Copy link
Member

stof commented Feb 28, 2017

hmm, we should still allow only Reference|null in the setter

@chalasr chalasr force-pushed the fix-slocator-setval branch 3 times, most recently from d23c60c to 2535108 Compare February 28, 2017 08:59
@chalasr
Copy link
Member Author

chalasr commented Feb 28, 2017

@stof fixed

@@ -39,7 +39,7 @@ public function getValues()
public function setValues(array $values)
{
foreach ($values as $v) {
if (!$v instanceof Reference) {
if (!$v instanceof Reference && null !== $v) {
Copy link
Member

Choose a reason for hiding this comment

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

nulls will create services that have "has()" return true, and "get()" return null. Not sure it's useful at all. What about skipping them instead?

Copy link
Member

Choose a reason for hiding this comment

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

Self answer: bad idea: maybe the consumer expects a value from get for the provided key. :)
Test case missing?

Copy link
Member Author

Choose a reason for hiding this comment

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

Test case added

@chalasr chalasr force-pushed the fix-slocator-setval branch 2 times, most recently from b71b4db to ae18b79 Compare February 28, 2017 09:30
@chalasr chalasr force-pushed the fix-slocator-setval branch from ae18b79 to 256b836 Compare February 28, 2017 09:31
@chalasr
Copy link
Member Author

chalasr commented Feb 28, 2017

Build failure unrelated.

@nicolas-grekas
Copy link
Member

Thank you @chalasr.

@nicolas-grekas nicolas-grekas merged commit 256b836 into symfony:master Feb 28, 2017
nicolas-grekas added a commit that referenced this pull request Feb 28, 2017
…ence values (chalasr)

This PR was merged into the 3.3-dev branch.

Discussion
----------

[DI] Fix ServiceLocatorArgument::setValues() for non-reference values

| Q             | A
| ------------- | ---
| Branch?       | master
| Fixed tickets | #21625 (comment)
| Tests pass?   | yes
| License       | MIT

`ResolveInvalidReferencesPass` [calls `setValues()`](https://github.com/symfony/symfony/blob/master/src/Symfony/Component/DependencyInjection/Compiler/ResolveInvalidReferencesPass.php#L91) with resolved invalid reference (null), the `Reference` type check should occurs at construction only.

Commits
-------

256b836 [DI] Fix ServiceLocatorArgument::setValues() for non-reference values
@chalasr chalasr deleted the fix-slocator-setval branch February 28, 2017 12:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants