Skip to content
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

Closure factories support #3

Closed
unkind opened this issue Feb 2, 2017 · 10 comments
Closed

Closure factories support #3

unkind opened this issue Feb 2, 2017 · 10 comments

Comments

@unkind
Copy link

unkind commented Feb 2, 2017

I use something similar to your component in my internal project, i.e. we have configuration loader which mimics YAML syntax of the DI component. I made such a decision for the same reasons as you described in the README (::class, constants, etc.).

However, it is pain in ass that there is no easy way to utilize closures in such configs for factories:

return [
    'services' => [
        'logger' => [
            'class' => Logger::class,
            'factory' => function (string $logFilename) {
                $logger = new LegacyFileLoggerWithSetterInjection();
                $logger->setFile($logFilename);

                return $logger;
            },
            'arguments' => ['%log_filename%']
        ]
    ]
];

From technical point of view, it is possible to implement it for context-free closures: without $this and without use statement (otherwise you won't be able to compile it). Unfortunately, it's hard to do it without changing the core PhpDumper (my attempt to bring this feature into the core failed in the past: symfony/symfony#12115, symfony/symfony#11953).

It would be great if you could find some way to implement the feature.

@mnapoli
Copy link
Owner

mnapoli commented Feb 3, 2017

Oh that's a really interesting idea. I'm keeping this issue open!

@rejinka
Copy link
Contributor

rejinka commented Feb 4, 2017

I don't get the point, i think. Perhaps, this isn't a good example, because you could use a method call instead?

Another more explicit change would be to move the closure into a real factory and use symfony/di's factory mechanism handle this.

I think with create()->method() this example is already really easy to solve.

@unkind
Copy link
Author

unkind commented Feb 4, 2017

because you could use a method call instead?

IDE won't help to autocomplete method name, all refactorings against the method won't be applied.

Another more explicit change would be to move the closure into a real factory and use symfony/di's factory mechanism handle this.

I don't find it practical to create a new class every time I need simple factory just for DI purposes.

You can find very similar example here: http://php-di.org/doc/best-practices.html#using-libraries

return [
    // ...

    Psr\Log\LoggerInterface::class => DI\factory(function () {
        $logger = new Logger('mylog');

        $fileHandler = new StreamHandler('path/to/your.log', Logger::DEBUG);
        $fileHandler->setFormatter(new LineFormatter());
        $logger->pushHandler($fileHandler);

        return $logger;
    }),
];

You always can configure it with existing keywords, but sometimes you don't want to waste time, especially with legacy code.

@pitchart
Copy link
Contributor

pitchart commented Feb 5, 2017

👍 , if it can also be used with a callable class

@mnapoli
Copy link
Owner

mnapoli commented Feb 5, 2017

@pitchart it can already be done I think:

return [
    'foo' => factory(FooFactory::class)
];

@unkind
Copy link
Author

unkind commented Feb 6, 2017

It's possible to generate static class with methods for each closure. But I don't see way to make it transparent for this library because someone should dump this class somewhere to cache (in the same file with container, for example) and/or configure autoloader.

As a dirty hack, it's possible to override Kernel::dumpContainer and append the static class to the file with compiled container.

But it still requires to do some things for users who use the DependencyInjection component as a standalone library.

@mnapoli
Copy link
Owner

mnapoli commented Feb 6, 2017

@unkind just trying to understand: you suggest generating a static class because it's impossible to add support for closure without changing PhpDumper is that right?

@unkind
Copy link
Author

unkind commented Feb 6, 2017

@mnapoli exactly.

@rejinka
Copy link
Contributor

rejinka commented Feb 7, 2017

We can not override Kernel::dumpContainer() as you already mentioned. Can we even rely on cache? Iirc you can use symfony/dependency-injection without dumping the container to a file. This really is not advisable, but if i'm right, we shouldn't break this.

Is there any point in our scope, where we could put this generated classes in?

As far as i know, we need superclosure as a dependency to create this classes. When the final goal is, to get this library merged into symfony at some point, this would be a blocker.

@unkind unkind closed this as completed May 8, 2018
@unkind unkind reopened this Nov 19, 2018
@unkind
Copy link
Author

unkind commented Nov 19, 2018

symfony/symfony#28992 (comment) — technically, it is possible to resolve issue using ExpressionLanguage.

@unkind unkind closed this as completed Aug 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants