Skip to content

[DI] Support generating interface-bound proxies #20656

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
nicolas-grekas opened this issue Nov 27, 2016 · 19 comments
Closed

[DI] Support generating interface-bound proxies #20656

nicolas-grekas opened this issue Nov 27, 2016 · 19 comments
Assignees
Labels
DependencyInjection Feature RFC RFC = Request For Comments (proposals about features that you want to be discussed)

Comments

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Nov 27, 2016

May it be useful to make the container able to generate interface-bound proxies?

I see 3 benefits to do so:

  • hiding the concrete class to the consumer, to create a safeguard and enforce talking to its dependency only through the interface-defined contract;
  • these kind of proxies could also be able to instantiate their concrete implementation lazily, in a much simpler way than the generic lazy-proxies generated by https://github.com/Ocramius/ProxyManager;
  • as a side effect, it would allow creating lazy proxies for final classes, thus fix [ProxyManager] Support final classes #20392.

There are two places where this could be enabled, depending on who should have this responsibility:

in the proxyfied-service definition

Assuming Ns\FooClass implements Ns\FooInterface, this could be eg. in XML:

<service id="foo" class="Ns\FooClass" proxy="true" />

so that:

$container->get('foo') instanceof Ns\FooInterface; //==> true
$container->get('foo') instanceof Ns\FooClass; //==> false

lazyness could be activated when the "lazy" attribute is enabled, or be always enabled.
"proxy" attribute being true, all interfaces implemented by the service class would be proxyfied.
We could also imagine a proxy tag that explicitly tells which interfaces should be proxyfied amongst all the implemented one.

in the dependency injecting definition

In XML, this could be eg.:

<service id="foo" class="Ns\FooClass" />
<service id="bar" class="Ns\BarClass">
<argument type="service" proxy="Ns\FooInterface">foo</argument>
</service>

so that inside bar:

$this->foo instanceof Ns\FooInterface; //==> true
$this->foo instanceof Ns\FooClass; //==> false

lazyness could be activated when a "lazy" attribute is enabled, or be always enabled.
In that case, I don't think we should allow proxying several interface at once.

WDYT? (ping @theofidry, maybe you'd like to give it a try?)

@nicolas-grekas nicolas-grekas added the RFC RFC = Request For Comments (proposals about features that you want to be discussed) label Nov 27, 2016
@xabbuh
Copy link
Member

xabbuh commented Nov 28, 2016

To me this is sounds like a good idea. 👍

@pamil
Copy link
Contributor

pamil commented Nov 29, 2016

I'd love to see it 👍

Btw. shouldn't <argument type="service" proxy="Ns\FooInterface">foo</argument> be <argument type="service" proxy="Ns\FooInterface" id="foo" />?

@stof
Copy link
Member

stof commented Nov 29, 2016

This should not be in the argument configuration, as this would mean that injecting the same service in 2 different places would inject different objects (depending on whether they are proxied or no), which can creates nasty behaviors (as the DIC currently enforces that getting a shared service always return the same instance).

Regarding the implementation, I'm not sure we should get rid of ProxyManager. There are a bunch of nasty cases regarding proxy generation (handling code generation, handling all features of method signatures, etc) and I don't see any benefit reimplementing many part of the library while it is well-tested.

@pamil
Copy link
Contributor

pamil commented Nov 29, 2016

@stof totally agreed, on both points made.

@theofidry
Copy link
Contributor

theofidry commented Nov 30, 2016

I think proxy generation, interface bound or not, are far from trivial so I would prefer to keep everything in ProxyManager instead. This IMO is a missing feature from ProxyManager, not Symfony itself (although Symfony will need to change a bit its integration for this new feature).

Regarding where to enable it, I think this should be done in the service definition, it's a peculiar way of registering a service, not injecting it. So I would go with:

<service id="foo" class="Ns\FooClass" proxy="true" />

to keep the current behaviour, and if the user wants to, he can proxy against a specific interface instead:

<service id="foo" class="Ns\FooClass" proxy="App\Interface" />

Maybe having a proxy for two interfaces is doable, so we could have:

<service id="foo" class="Ns\FooClass" proxy="true">
    <proxy>
        <argument type="string">App\FooInterface</argument>
        <argument type="string">App\BarInterface</argument>
    <proxy>
</service>

But I think we can re-discuss about that once this feature has landed in ProxyManager.

@theofidry
Copy link
Contributor

theofidry commented Dec 5, 2016

As discussed at the SymfonyCon, it is true that an interface bound proxy is way simpler (approximate code):

interface FooInterface
{
    public function foo(): string;
}

class FooProxyClass implements FooInterface
{
    private $container;
    private $serviceId;
    private $service;

    public function foo(): string
    {
        if (null === $this->service) {
            $this->service = $this->container->get($this->serviceId);
        }

        return $this->service->foo();
    }
}

$proxy = Closure::bind(
    function (ContainerInterface $container, string $serviceId) {
        $this->container = $container;
        $this->serviceId = $serviceId;
        
        return $this;
    },
    new FooProxyClass(),
    'FooProxyClass'
)($container, $serviceId);

It might still be easier to have a dependency on Zend/Code to generate the proxy though.

I'm still less convinced about the argument proxy and would prefer to split this RFC in two to treat them separately, but I'm +1 for the first one.

There is also the question of if this could be handled in ProxyManager and be more generic (not coupled to Symfony DIC) via PSR-11.

@nicolas-grekas
Copy link
Member Author

nicolas-grekas commented Dec 6, 2016

Because we decided to not have a hard requirement on ProxyManager (and I'm really fine with this), lazy services are second zone citizen right now.
I forgot to mention it: this RFC is also targeted at making them first class citizen, by seeking for a way to make them so with simpler logic, which I believe this RFC allows (as you spotted).
This means interface-proxy generation should be done in the component itself, by PhpDumper I'd say.
We should KISS, the need for interop (being PSR11 or separated package for code gen) does not exist IMHO. And if it were, it might be done in a later PR.

About your example proxy, it looks nice :)
I just didn't expect the $service property and thought we should always call the container. This allows resetting the service transparently in some case where it could be needed (e.g. Doctrine EM). This shouldn't impact perf much.

There is one missing case, which is fluid interfaces.
So we should check the return value like this:

$service = $this->container->get($id);
$result = $service->foo();
return $result === $service ? $this : $result;

@nicolas-grekas
Copy link
Member Author

Of course, we should also deal with references (as arg and as return values).
But I think we don't have to deal with PHP5-style variadics (that use func_get_args()).

@nicolas-grekas
Copy link
Member Author

nicolas-grekas commented Jan 14, 2017

I guess this should also allow generating proxies based on abstract classes?

@theofidry
Copy link
Contributor

Possibly, but I would add the support for it in a second iteration.

@TheCelavi
Copy link
Contributor

TheCelavi commented Jul 10, 2017

I would like to support third comment/feature by @pamil. This feature can be desirable, at this moment, this feature would help us with our current project.

Namely, we are using Go! Aop for handling cross cutting concerns (mainly as implementation of decorator). Now, Aop bundle allows us to inject symfony services in aspects, which makes it very powerful tool.

However, we have noticed that service injected in aspect can not be, nor it can depend on, a service which class has been weaved with Aop engine. Not to get into too much explaining, it goes something like -> first service container is initialized, then aspect container, then aspects as services, then other services. If aspect requires service, that service is initialized prior to aspect, where we get into a circular dependency issue.

There are two approaches now with which we can handle this situation:

  1. Service locator, we can inject container, or service locator - we dont like that approach, we dont like idea of service locator at all
  2. Mark injected service into aspect as "lazy" (injecting proxy).

We are using approach nb. 2. However, we are not found of that approach either, because, for vendor's services we have to do extra work in order to make that service lazy.

We use a lot of aspects, they are registered and initialized with the application, but majority is not used during Request-Response lifecycle.

Therefore, in terms of execution optimization, but mostly having in mind developer experience, ability to specify that injection of specific service should be lazy would increase our productivity.

For us, solving cyclic dependency issue would be as simply as modifying from:

<argument type="service">foo</argument> 

to:

<argument type="service" proxy="Ns\FooInterface">foo</argument>

and that would make our developer experience smooth. We do not have any objection to sacrifice some performances or arguable "good, by the book, practice" on account of our productivity and achieved developer experience.

Thank you.

@stof
Copy link
Member

stof commented Jul 10, 2017

Well, marking a service as proxy at the level of the injection breaks a rule of the current system: the injection of <argument type="service" id="translator" /> always injects the same instance for shared services. If we start doing injection-level proxies, this may not be true anymore (and becomes even worse if we generate multiple proxy instances)

@TheCelavi
Copy link
Contributor

@stof Proxy is just a wrapper around of same service instance for shared service right? Each proxy would use same service instance - it would not be a same proxy, but underlying service would be the same, or I have misinterpreted proposed idea?

@TheCelavi
Copy link
Contributor

On top of that, if the rule is: "if there is a shared service A that can be lazy loaded with proxy AA, there can be only one instance of service A and only one instance of its proxy AA" -> I don't think that that can be classified as hard problem to solve. As service container manages shared services, it can manage shared proxies, right?

@stof
Copy link
Member

stof commented Jul 10, 2017

Well, the issue proposes 2 implementations. What I'm saying is that the first proposal fits well in the existing system (it is actually just a different way of implementing the existing proxy, by proxying only an interface rather than the whole class). The second one requires changing one of the invariants we have in the component (and requires adding complexity in the component for that)

@backbone87
Copy link
Contributor

backbone87 commented Nov 15, 2017

Just a DX note: it must be possible to disable proxying on the argument side. imagine a service user like this:

class Builder {
  private $service;
  public function __construct(ServiceInterface $service) {
    $this->service = $service;
  }
  public function method() {
    if($this->service instanceof SomeConcreteImplementationThatAllowsUsageOptimizations) {
      // efficient solution using special methods of conrete impl
      return;
    }
    // the long way
  }
}

if the injected service is defined by a 3rd party, you must override the whole definition, which in the worst case requires a custom compiler pass (if the service is dynamically created).
Therefore <argument type="service" id="conrete_service" proxy="false" /> should be possible

edit: i know there are other ways around this, like adding a 2nd optional injection against the concrete service, but i bet they all have their drawbacks

@theofidry
Copy link
Contributor

Not really a big fan of that @backbone87 tbh. If you don't want the service to be proxied it's simpler to either override the service definition or alter it via a Compiler pass. If it's just for that specific case, then maybe we could indeed provide a Proxy interface which would have a getProxiedInstance() method or something which every proxy would implement.

@popy-dev
Copy link
Contributor

I'm probably a bit naïve there, I've just red this, and came with one idea which allows to "opt out" the proxy, avoid name collision (the proxy having the same name than the service), and would work nicely with autowiring :

Assuming Ns\FooClass implements Ns\FooInterface

<service id="foo" class="Ns\FooClass" />
<service id="Ns\FooInterface" proxy="foo" />

Also, the service Ns\FooInterface could either implement ONLY Ns\FooInterface, or every interface of the Ns\FooClass.

One other way of configuring this kind of behaviour would be that <service id="foo" class="Ns\FooClass" proxy="true" /> would create one proxy service named after each Interface implemented by Ns\FooClass (but I guess it could be confusing seing services coming out of nowhere)

@nicolas-grekas
Copy link
Member Author

See #27697 for an implementation (not everything discussed/proposed, but still the main idea.)

@fabpot fabpot closed this as completed Jul 9, 2018
fabpot added a commit that referenced this issue Jul 9, 2018
…ith "lazy: Some\ProxifiedInterface" (nicolas-grekas)

This PR was merged into the 4.2-dev branch.

Discussion
----------

[ProxyManagerBridge][DI] allow proxifying interfaces with "lazy: Some\ProxifiedInterface"

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #20656
| License       | MIT
| Doc PR        | -

By adding `<tag name="proxy" interface="Some\ProxifiedInterface" />` to your service definitions, this PR allows generating interface-based proxies.

This would allow two things:
- generating lazy proxies for final classes
- wrapping a service into such proxy to forbid using the methods that are on a specific implementation but not on some interface - the proxy acting as a safe guard.

The generated proxies are always lazy, so that lazy=true/false makes no difference.

As a shortcut, you can also use `lazy: Some\ProxifiedInterface` to do the same (yaml example this time.)

Commits
-------

1d9f1d1 [ProxyManagerBridge][DI] allow proxifying interfaces with "lazy: Some\ProxifiedInterface"
javiereguiluz added a commit to symfony/symfony-docs that referenced this issue Aug 4, 2022
… lazy services (tucksaun)

This PR was merged into the 4.4 branch.

Discussion
----------

[DependencyInjection] Document proxifying interfaces for lazy services

Symfony 4.2 introduced the possibility of lazy load services using final classes by proxyfying specific interfaces (symfony/symfony#20656), but this has not been documented yet.

Targeting 4.4 because 4.4 is the oldest maintained branch, but should I target 4.2 as this was introduced in 4.2?

Fix #10295

Commits
-------

f86b976 [DI] Document proxifying interfaces for lazy services
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DependencyInjection Feature RFC RFC = Request For Comments (proposals about features that you want to be discussed)
Projects
None yet
Development

No branches or pull requests

10 participants