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] Add iterable to possible binding type #44979

Merged
merged 1 commit into from Jan 12, 2022

Conversation

sveneld
Copy link
Contributor

@sveneld sveneld commented Jan 11, 2022

Q A
Branch? 4.4
Bug fix? yes
New feature? no
Deprecations? no
License MIT

When iterable type is set in binding like in example https://symfony.com/doc/current/service_container.html#binding-arguments-by-name-or-type, system tries to autoload class iterable here src/Symfony/Component/DependencyInjection/Compiler/ResolveBindingsPass.php:137

if (is_subclass_of($m[1], \UnitEnum::class)) {

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Jan 11, 2022

Please add a test case that covers the situation.

@sveneld
Copy link
Contributor Author

sveneld commented Jan 11, 2022

Add test for this case

@sveneld sveneld force-pushed the fix_ResolveBindingsPass branch from 2c65299 to a5d1ba2 Compare Jan 11, 2022
@sveneld
Copy link
Contributor Author

sveneld commented Jan 12, 2022

@nicolas-grekas done

@@ -212,6 +213,31 @@ public function testEmptyBindingTypehint()
$pass->process($container);
}

public function testIterableBindingTypehint()
{
spl_autoload_register(
Copy link
Member

@nicolas-grekas nicolas-grekas Jan 12, 2022

Choose a reason for hiding this comment

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

is this required? if not let's remove it

Copy link
Contributor Author

@sveneld sveneld Jan 12, 2022

Choose a reason for hiding this comment

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

it's required to show the problem, if this function will run it will mean that code of ResolveBindingsPass.php working incorrect

$pass = new ResolveBindingsPass();
$pass->process($container);

$this->assertInstanceOf(
Copy link
Member

@nicolas-grekas nicolas-grekas Jan 12, 2022

Choose a reason for hiding this comment

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

on one line please

);

$container = new ContainerBuilder();
$bindings = [
Copy link
Member

@nicolas-grekas nicolas-grekas Jan 12, 2022

Choose a reason for hiding this comment

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

the variable could be removed

@nicolas-grekas nicolas-grekas modified the milestones: 5.4, 4.4 Jan 12, 2022
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

(for 4.4)

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Jan 12, 2022

Thank you @sveneld.

@nicolas-grekas nicolas-grekas merged commit 9f89250 into symfony:4.4 Jan 12, 2022
8 of 10 checks passed
@sveneld sveneld deleted the fix_ResolveBindingsPass branch Jan 12, 2022
nicolas-grekas added a commit that referenced this pull request Feb 16, 2022
This PR was submitted for the 5.4 branch but it was merged into the 4.4 branch instead.

Discussion
----------

[DependencyInjection] Fix type binding

| Q             | A
| ------------- | ---
| Branch?       | 5.4
| Bug fix?      | yes
| License       | MIT

If $type is a scalar compiler pass should not check it in function is_subclass_of($type, \UnitEnum::class), because is_subclass_of trying to autoload class with name array, string, etc.

Related to #44979

Commits
-------

3754018 Fix type binding
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.

None yet

3 participants