Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

[DependencyInjection] Added closure service factories#12115

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation

@unkind
Copy link
Contributor

QA
Bug fix?no
New feature?yes
BC breaks?no
Deprecations?no
Tests pass?yes
Fixed ticketsn/a
LicenseMIT
Doc PRn/a

This PR adds closure service factories for theDependencyInjection component. There are 2 kinds of closures:

useSymfony\Component\DependencyInjection\ContainerInterface;// Closure with services/parameters as arguments$container    ->register('bar1','Bar')    ->setFactory(function (Foo$foo,$bar) {returnnewBar($foo->getQux(),$bar);    })    ->addArgument(newReference('foo'))    ->addArgument('%bar%');// Pimple-like closure with container as an argument$container    ->register('bar2','Bar')    ->setFactory(function (ContainerInterface$container) {returnnewBar($container->get('foo')->getQux(),$container->getParameter('bar'));    });

It would be useful for something likeArrayFileLoader described in#11953.

@unkindunkindforce-pushed thefeature-closures-as-service-factories branch 2 times, most recently fromf2cba65 to1725733CompareOctober 3, 2014 15:52
@stof
Copy link
Member

stof commentedOct 4, 2014

I don't like the idea of the Pimple-like closures. It makes the behavior of the factory callable inconsistent between the case of a closure and the case of other type of callables.
Thus, it makes the code fragile as any bundle adding an argument in the service definition will break it (as the first argument will not be the container anymore).

Thus, using such closures will require marking the dependencies as public (as they are retrieved dynamically), and prevents the ContainerBuilder to perform any optimization on the service instantiation (as it is inside the closure). So using such Pimple-like closures has lots of drawbacks.

@unkindunkindforce-pushed thefeature-closures-as-service-factories branch 2 times, most recently from8af5df4 to73ed6fcCompareOctober 4, 2014 17:00
@unkind
Copy link
ContributorAuthor

Pimple-like syntax has been removed.

@unkindunkindforce-pushed thefeature-closures-as-service-factories branch from73ed6fc to3a9e4f8CompareOctober 4, 2014 17:03
Copy link
Member

Choose a reason for hiding this comment

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

we don't make classes final in Symfony generally

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

I think it's bad practice to be honest. No reason to inherit this class, you can make decorator to extend behaviour.

@unkindunkindforce-pushed thefeature-closures-as-service-factories branch 2 times, most recently frombff5a80 to92b59f2CompareOctober 4, 2014 18:03
Copy link
Member

Choose a reason for hiding this comment

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

add a blank line beforereturn

@unkindunkindforce-pushed thefeature-closures-as-service-factories branch from92b59f2 tob6067ffCompareOctober 4, 2014 22:32
@unkind
Copy link
ContributorAuthor

Any chance to merge it in 2.6? ping@fabpot@stof

@fabpot
Copy link
Member

No, I still have too many questions/doubts to be able to merge this in a hurry, that's never a good idea.

@unkind
Copy link
ContributorAuthor

The biggest problem of this PR is a coupling ofClosureDumperInterface withDependencyInjection component (it's bad as we need the same service inRouting configs, for instance). Probably we can moveClosureDumperInterface to theExpressionLanguage. However, it wouldn't be perfect place as well.

@fabpot
Copy link
Member

I think I'm 👎 on this one. It adds a lot of complexity for very rare use cases that you can do another way.

@unkind
Copy link
ContributorAuthor

I have to remind that initially it was done for theArrayFileLoader, where closures look very nature comparing to the expression language. In our company we use Symfony DIC with such custom file loader and sometimes it would be really helpful for us.

Some use cases:

  1. We have legacy code like singleton DB connection, i.e.Database::master(),Database::slave(), etc. Just for quick solution we would write

    <?phpreturn ['services' => ['foo' => ['factory' =>function () {returnnewFoo(Database::master());            },'class' => Foo::class,        ]    ]];

    to avoid coupling with singletonDatabase class. Also we have legacy configs withConfigLocator class. I can reuse config and don't hurt new code by singletons. We have really huge code base and sometimes it takes much work for refactoring.

  2. The same motivation as described inhttp://symfony.com/doc/current/book/service_container.html#using-the-expression-language, i.e. it's much easier to make closure rather than creating separate class.

If you say that it's a "very rare case", you have to admit that you either don't want array file configs at all or expression language factory is a "very rare case" as well.

But it won't be merged in this way of course, becauseClosureDumperInterface is tightly coupled with theDependencyInjection component, but I think it makes sense to raise this question again after#6129.

@unkindunkind closed thisFeb 5, 2015
@unkindunkind deleted the feature-closures-as-service-factories branchFebruary 5, 2015 20:38
@stof
Copy link
Member

stof commentedFeb 5, 2015

@unkind in your specific case, I would rather define services for your database connection (using your singleton getter as factory) and then inject the service in yourfoo service.

The issue when introducing the dumping of closures is that we add lots of complexity for that, and that we only allow dumping a subset of closures (anonymous functions which don't depend on their context at all)

@unkind
Copy link
ContributorAuthor

The issue when introducing the dumping of closures is that we add lots of complexity for that, and that we only allow dumping a subset of closures (anonymous functions which don't depend on their context at all)

SuperClosure allows to detect context-dependent closures, so it's not real issue IMO, we can throw an exception to avoid them. Anyway, it looks weird to have context-dependent closures in such config file.

@stof
Copy link
Member

stof commentedFeb 5, 2015

sure we can throw an exception in this case. But it means we don't support them, meaning we only support a subset of closures, not all closures.

@unkind
Copy link
ContributorAuthor

Partial support is better than nothing in this case IMO.

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

Reviewers

No reviews

Assignees

No one assigned

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@unkind@stof@fabpot@cordoval@jderusse

[8]ページ先頭

©2009-2025 Movatter.jp