8000 ServiceClosureArgument is not recognized by lint:container · Issue #39259 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

ServiceClosureArgument is not recognized by lint:container #39259

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
vudaltsov opened this issue Dec 1, 2020 · 8 comments
Closed

ServiceClosureArgument is not recognized by lint:container #39259

vudaltsov opened this issue Dec 1, 2020 · 8 comments
Labels
Bug DependencyInjection FrameworkBundle Status: Reviewed

Comments

@vudaltsov
Copy link
Contributor
vudaltsov commented Dec 1, 2020 •
edited
Loading

Symfony version(s) affected: 5.2, 5.1, probably earlier versions too

Description
When service is injected via ServiceClosureArgument like in DoctrineMigrationsExtension, lint:container throws an error:

[ERROR] Invalid definition for service "doctrine.migrations.dependency_factory": argument 2 of "Doctrine\Migrations\DependencyFactory::setDefinition" accepts "callable", "App\CustomSchemaProvider" passed.

How to reproduce
https://github.com/vudaltsov/symfony-lint-container-reproducer

UPDATE
Since doctrine/doctrine-migrations-bundle v3.1.0 container lint fails without any custom service configuration.
@vudaltsov vudaltsov added the Bug label Dec 1, 2020
@vudaltsov
Copy link
Contributor Author

@nicolas-grekas , need your help with this. I investigated the issue a bit and it looks strange.

First, there is already an if to support ServiceClosureArgument, see line. And it's covered with a non-failing test. Good, but it does not work in the real project!

The problem is that at the time the container builder is used in ContainerLintCommand (this line), the method call is not a ServiceClosureArgument anymore, it's a reference, so the reference's class is checked against callable, not ServiceClosureArgument:

image

At the same time in the dumped container it's a closure, which is correct:

image

@stof
Copy link
Member
stof commented Dec 3, 2020

The problem is that at the time the container builder is used in ContainerLintCommand (this line), the method call is not a ServiceClosureArgument anymore, it's a reference

that's what need to be investigated. I don't see any valid reason for it to be a Reference object here.

@vudaltsov
Copy link
Contributor Author
vudaltsov commented Dec 5, 2020 •
edited
Loading

I found the issue. ContainerLintCommand uses xml dump to recreate ContainerBuilder, but XmlDumper dumps ServiceClosureArgument as a value, not as a value wrapped in a closure:

symfony/src/Symfony/Component/DependencyInjection/Dumper/XmlDumper.php

Line 279 in 31ee43a

$value = $value->getValues()[0];

Here's my idea. To add a static method ServiceClosureArgument::wrapServiceInAClosure() to be able to use it as a factory for an inline service:

final class ServiceClosureArgument
{
    // ...

    public static function wrapServiceInAClosure($service): \Closure
    {
        return function () use ($service) {
            return $service;
        };
    }
}
<call method="setDefinition">
    <argument>Doctrine\Migrations\Provider\SchemaProvider</argument>
    <argument type="service">
        <service class="Closure">
            <factory class="Symfony\Component\DependencyInjection\Argument\ServiceClosureArgument" 
                     method="wrapServiceInAClosure"/>
            <argument type="service" id="HappyInc\Doctrine\SchemaConfigurator\CompositeSchemaProvider"/>
        </service>
    </argument>
</call>

@stof
Copy link
Member
stof commented Dec 5, 2020

yeah, I think the issue is that we don't have a XML syntax to create a ServiceClosureArgument right now. they can only be created from PHP config. That's what we should fix instead IMO, rather than adding a factory method

@crmpicco
Copy link

@vudaltsov Nice find. I'm curious to know if there has been any progress with an upstream fix for this?

@derrabus derrabus mentioned this issue Apr 21, 2021
Closed
@derrabus
Copy link
Member

Thanks for the ping @crmpicco. I wasn't aware of this issue and opened another one in the meantime: #40773

@mbrodala
Copy link
Contributor
mbrodala commented Apr 28, 2021 •
edited
Loading

Notice: the same issue also occurs with doctrine/doctrine-migrations-bundle 3.0.x already.

Workaround for now:

$ bin/console lint:container --env prod
                                                                                                                        
 [OK] The container was lint successfully: all services are injected with values that are compatible with their type    
      declarations.

@rosier rosier mentioned this issue Apr 29, 2021
Merged
javiereguiluz added a commit to symfony/demo that referenced this issue Apr 30, 2021
@javiereguiluz
minor #1206 [CI] Lint the container loaded from source instead of xml…
beeff9a 
… (rosier)

This PR was merged into the main branch.

Discussion
----------

[CI] Lint the container loaded from source instead of xml

Workaround for doctrine/DoctrineMigrationsBundle#413 and symfony/symfony#39259

Commits
-------

c5172ff Lint the container loaded from source instead of xml
@nicolas-grekas nicolas-grekas mentioned this issue May 3, 2021
Closed
@nicolas-grekas nicolas-grekas mentioned this issue May 11, 2021
Merged
@nicolas-grekas
Copy link
Member

Will be fixed by #41176

nicolas-grekas added a commit that referenced this issue May 11, 2021
@nicolas-grekas
bug #41176 [DependencyInjection] fix dumping service-closure-argument…
9a53e6a 
…s (nicolas-grekas)

This PR was merged into the 4.4 branch.

Discussion
----------

[DependencyInjection] fix dumping service-closure-arguments

| Q             | A
| ------------- | ---
| Branch?       | 4.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | Fix #39259
| License       | MIT
| Doc PR        | -

5.3 uses service closures a bit more, so that this is required to make the `lint:container` command work.

Commits
-------

1aa9a24 [DependencyInjection] fix dumping service-closure-arguments
sayjun0505 added a commit to sayjun0505/sym_proj that referenced this issue Apr 16, 2023
@sayjun0505
minor #1206 [CI] Lint the container loaded from source instead of xml…
087da96 
… (rosier)

This PR was merged into the main branch.

Discussion
----------

[CI] Lint the container loaded from source instead of xml

Workaround for doctrine/DoctrineMigrationsBundle#413 and symfony/symfony#39259

Commits
-------

c5172ff Lint the container loaded from source instead of xml
spider-yamet added a commit to spider-yamet/sym_proj that referenced this issue Apr 16, 2023
@spider-yamet
minor #1206 [CI] Lint the container loaded from source instead of xml…
74b297a 
… (rosier)

This PR was merged into the main branch.

Discussion
----------

[CI] Lint the container loaded from source instead of xml

Workaround for doctrine/DoctrineMigrationsBundle#413 and symfony/symfony#39259

Commits
-------

c5172ff Lint the container loaded from source instead of xml
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Bug DependencyInjection FrameworkBundle Status: Reviewed
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
8 participants
@nicolas-grekas @stof @derrabus @xabbuh @crmpicco @vudaltsov @mbrodala @carsonbot
0