Closed hkdobrev closed 6 years ago
One good way to address that in my opinion, which would make using lazy closures for configuration options on many providers, would be to allow extending something which is still a scalar or non-invokable object as it could be overridden to a wrapping closure later before it is accessed.
Old, but related: https://github.com/silexphp/Pimple/issues/70
Then TwigServiceProvider
would be able to do the array_replace
operation in an extend
wrapping closure after the declaration of $app['twig.options'] = array();
.
This way everything is lazy and it would improve how the provider works even more as it would do the parameter replacement for any Twig factory retrieval and not just for the default $app['twig']
service like now.
@fabpot @skalpa Do you have a suggestion for that problem? I'm keen on tackling that, but I'm not certain I'm on the best path so it's intuitive to use.
Why would you want twig.options
to be a closure to start with ? What's your use case ?
If you ask me too many things are already defined with closures, and while it makes sense to register arrays of services like this, I don't believe it does the rest of the time.
Ie: in the pimple issue you linked the solution to the author's problem is just not to use extend()
:
$c['some_value'] = array(1, 2, 3);
$c['some_value'] = array_merge($c['some_value'], array(4, 5));
@skalpa I have something like this:
$app->register(new TwigServiceProvider(), [
'twig.options' => [
'cache' => $app['caching']
? $app['cache.dir'].'/twig'
: false,
],
]);
When running cache:clear
in our Silex app we perform something very similar to the latest cache:clear
implementation in Symfony 4 with a cache clearer and a cache warmer. In Symfony, the kernel could be reloaded now and the DIC could be re-compiled. However, with Silex and Pimple it's quite easier if the cache.dir
service in Pimple is not retrieved during registering of providers so it could be overridden during the cache clearing and warming with a custom value.
We went over everything using cache.dir
so early and we successfully used closures around all except for TwigServiceProvider
.
In the end, we overcame the problem in our cache:clear
command, but it's a bit more complex and has more assumptions, because of this limitation.
I understand you shouldn't wrap everything in closures, but the service provider should allow it in cases when it just needs scalars from the container. It shouldn't try to write to the same key in the container when it exposes it for overriding to the user.
What you're doing is extremely fragile. Best practices dictate that:
Changing the content at a very late stage will force you to ensure the value you are changing has not been used yet and implement workarounds to make sure that is the case (which is the problem you're facing).
If I understand what you're doing, then I would recommend that you do something similar to what the framework command does: let your console command instantiate a second instance of your application that would have a different value set for cache.dir
, and use this application to warm up your caches.
I want the
twig.options
configuration to be a closure and Pimple allows that easily via:However,
TwigServiceProvider
does not allow that, because it is usingarray_replace
getting and then setting thetwig.options
key in one go:https://github.com/silexphp/Silex/blob/c62cfba1b9a3533274ad01b9f55d87000c26071c/src/Silex/Provider/TwigServiceProvider.php#L55-L61
This results in Pimple throwing an exception - which is normal because the provider has just retrieved a key and tried to override it. This overriding works only if it's a fixed value and Pimple has not executed a closure and frozen the result.