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

Make resource factories meaningful #29133

Closed
Bilge opened this Issue Nov 8, 2018 · 3 comments

Comments

Projects
None yet
4 participants
@Bilge
Contributor

Bilge commented Nov 8, 2018

Description
It is not currently meaningful to define a factory for a resource (collection of services).

Example

    App\Foo:
      resource: '../src/Foo/*'
      factory: App\Foo\MyFactory:create

The factory will not know which service it is supposed to create because it does not know which class it was invoked for. That is: there is no way to pass the class name to the factory.

I expect someone to point out that we can just create a factory for every class. Notwithstanding the resultant class explosion, in my particular case, writing a "generic" factory makes sense because all instances of this type can be constructed using reflection to map the name of constructor parameters to request attributes and/or query parameter names. This would save having to create many factories, but the missing link is informing the factory which class it's supposed to be constructing.

@stof

This comment has been minimized.

Member

stof commented Nov 8, 2018

factories can receive arguments (if you defined arguments in a service definition using a factory, they are argument for the factory method, not for the constructor, as the container won't use the constructor by itself).
But the batch loading will not define arguments for each service with the class name in them, and there is no easy way to achieve this (this is not a use case for which the class discovery was meant to provide a solution).

all instances of this type can be constructed using reflection to map the name of constructor parameters to request attributes and/or query parameter names.

be careful about that. The container has a longer lifecycle than the request. There might be no request at all available when the service gets instantiated, and there might be multiple requests handled by the kernel (and your service won't be recreated for each of them). This sentence makes me think that these classes should probably not be defined as services (or should be re-architectured first, which would remove this use case).

@Bilge

This comment has been minimized.

Contributor

Bilge commented Nov 8, 2018

Hi @stof. I was able to achieve it quite easily with a simple compiler pass to copy the class name from the definition into the argument.

I appreciate your warning about the container lifecycle. However, there is no use case in our application for handling multiple requests. Nevertheless, I completely take your point, and have recently reflected on the idea that the DIC does not seem suitable for building things that rely on runtime data. The factory pattern facilitates creating services that rely on runtime data, but since these services are shared (created once), then as you point out, they could represent obsolete data at a later time. Unfortunately, I don't know a better way to architect this, but mixing runtime (dynamic) and compile time (static) data in the DIC is definitely proving to be a source of difficulty for me, not least of all because it does not seem designed to do this.

I even have "services" called CurrentX, like CurrentAuthenticationToken, that are constructed using runtime data from the RequestStack. But having a current anything seems like a code smell to me. Appreciate any advice to improve this!

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Nov 8, 2018

I was able to achieve it quite easily with a simple compiler pass

great! On my side, I think that's good enough: I don't feel like we should do more in core.

@Bilge Bilge closed this Nov 26, 2018

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