Skip to content

[DI] Add PHP service factories #23935

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

Closed
wants to merge 1 commit into from
Closed

[DI] Add PHP service factories #23935

wants to merge 1 commit into from

Conversation

ro0NL
Copy link
Contributor

@ro0NL ro0NL commented Aug 19, 2017

Q A
Branch? 3.4
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? not yet
Fixed tickets #23819
License MIT
Doc PR symfony/symfony-docs#...

This is a POC that more or less implements a feature described in #23819.

It allows to automate service creation from a factory, which itself is a service. You tag it, and basically it scrapes methods to factorize as new services.

So far, no real annotations are involved. Im not sure a whole system is worth it for this feature solely. Yet it introduces one new annotation, in the spirit of @required, to indicate which methods to factorize; @service or @service my.id. This convention can also be used for #23898.

To me this is convenient. Further configuration can be applied static (this PR is not that far though); adding tags, sharing, factory arguments etc. It requires no real alias support; as that would be simply forwarding methods in this context.

It checks return types for the class value, so it works on PHP7 only for now. I abused tests for this to demonstrate :) I wasnt planning for parsing @return so <PHP7 requires further configuration.

Quick example;

services:
   MyFactory: { tags: [container.service_factory] }
class MyFactory {
    /**
     * @service
     */
    public function someFoo(): Foo {
       return new Foo();
    }

    /**
     * @service some.bar
     */
    public function someBar(): Bar {
       return new Bar($this->someFoo());
    }
}

This creates the services some_foo and some.bar.

Thoughts?

/cc @GuilhemN

@unkind
Copy link
Contributor

unkind commented Aug 20, 2017

Considering there will be support of fluent PHP configuration (#23834), I'd rather put effort to create closure-based factories, e.g.

<?php

di\services()
    ->instanceof(Bar::class)
    ->factory(function () {
         return new Bar(new Foo());
     })

I tried to bring this feature in the past (#12115), but it was rejected because "It adds a lot of complexity for very rare use cases that you can do another way". However, with fluent PHP configuration in the core it wouldn't be "rare case" anymore I guess.

Even if one prefers YAML/XML formats, he still can create fluent PHP config file with the factories instead of "service factory" which you propose.

@ro0NL
Copy link
Contributor Author

ro0NL commented Aug 21, 2017

@unkind nice.. didnt even thought of such an approach :)

Yet we cannot dump such a closure factory. So im not sure it works out; the current approach mostly leverages existing features.

For the config part is was planning to have some ServiceFactoryInterface::configure($id, Definition $def). Keeping everything at the same place.

I like the fact we can create powerful services with a minimal config.

@stof
Copy link
Member

stof commented Aug 21, 2017

@unkind Closure-based factories are incompatible with the dumping of an optimized container, as we cannot dump the closure (due to a closure being able to reference variables from its defining scope). And dumping the final container to a PHP class is necessary (at least if you care about performance).

Btw, I see a big drawback with this approach: any time your factory references another of its methods to get a dependency, it will create its own instance of the services, which will not be the same than the shared service used by the container. This can lead to hard-to-debug behaviors (overwriting the foo service will not overwrite the dependency used by bar, as it does not use the foo service but its own instance with a similar configuration than the original foo service).

@ro0NL
Copy link
Contributor Author

ro0NL commented Aug 21, 2017

Btw, I see a big drawback with this approach: any time your factory references another of its methods to get a dependency, it will create its own instance of the services, which will not be the same than the shared service used by the container.

Fair point.

We could set shared to false by default, but it really depends what the user does. I.e. if it mimics sharing/memoization in PHP. We dont know that...

I tend not to care about using non-shared $this->someFoo() internally, while it maybe shared externally ($container->get('some_foo')). IMHO you are responsible to set this right, as needed. But if we can do something here that be nice for sure.

@unkind
Copy link
Contributor

unkind commented Aug 21, 2017

Closure-based factories are incompatible with the dumping of an optimized container, as we cannot dump the closure (due to a closure being able to reference variables from its defining scope).

Don't reference such variables. There is no need in it.

@stof
Copy link
Member

stof commented Aug 21, 2017

@unkind even then, getting the source code of a closure to be able to replicate it in the dumped source code cannot be done in a clean way (and is impossible if you have multiple closures defined on the same line, as closures only have a start and end line in reflection, like other functions, and they don't have a name to resolve ambiguities when having multiple closures on the line).

@unkind
Copy link
Contributor

unkind commented Aug 21, 2017

even then, getting the source code of a closure to be able to replicate it in the dumped source code cannot be done in a clean way

There are third party tools which do it quite well.

and is impossible if you have multiple closures defined on the same line

I know these restrictions, but they are too artificial and can be detected at container compilation time. Likewise, I can say that Symfony doesn't have full support of YAML, but it seems like it isn't a disaster.

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Aug 21, 2017

@unkind please open a dedicated RFC if you want to push this idea. On my side, I agree with @stof. We don't have proxy-manager in core because it's already a too particular piece of code. We won't add superclosure or similar for the same reason to me.

@ro0NL
Copy link
Contributor Author

ro0NL commented Aug 21, 2017

Agree with @nicolas-grekas, this is not about proposed setFactory(function() {}) vs. current setFactory(array(Some::class, 'foo')). But this PR basically does that by using class methods as a factory, with an automated config approach as much as possible.

Parsing @return would so help this PR :) or adding more annotations. I must agree it's convenient for config... yet im also fine with a static PHP callback to further process if we go this way.

@nicolas-grekas
Copy link
Member

In fact, this PR made me realize we have a "bug" with autowiring (that we should fix as a feature in 3.4):
if a service is defined as autowired and has a factory, we should autowire the factory method instead of the constructor of the service!
@ro0NL would you like to submit a PR fixing this? It would be a nice prerequisite for this PR also (yet, I don't if this PR will make it - but it's nice to be able to think about the idea with an implementation!)

@ro0NL
Copy link
Contributor Author

ro0NL commented Aug 21, 2017

@nicolas-grekas dont we already do that.. looking at AbstractRecursivePass::getConstructor :S

@nicolas-grekas
Copy link
Member

oh indeed!

@nicolas-grekas nicolas-grekas added this to the 3.4 milestone Aug 22, 2017
return $methodsToFactorize;
}

private function getClass(\ReflectionMethod $reflectionMethod)
Copy link
Member

Choose a reason for hiding this comment

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

you should be able to use Symfony\Component\DependencyInjection\LazyProxy\ProxyHelper::getTypeHint() instead

@ro0NL
Copy link
Contributor Author

ro0NL commented Aug 31, 2017

Closing after some discussion with @nicolas-grekas

This PR can go many ways, and i want to play with #23834 first actually :)

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.

5 participants