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
[DependencyInjection] Reset env vars when resetting the container #54666
Conversation
faizanakram99
commented
Apr 18, 2024
•
edited by OskarStark
edited by OskarStark
Q | A |
---|---|
Branch? | 7.1 |
Bug fix? | no |
New feature? | yes |
Deprecations? | no |
Issues | Fix #54627 |
License | MIT |
d50e460
to
4452c58
Compare
So all env vars would be reset ? Without option / configuration ? Or am i reading this wrong ? |
Yes (for now), I wasn't sure how to best make it opt-in (config based). Do you think it should be opt in? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could even consider this as a bugfix IMHO.
In non-worker mode (aka for most pp), env vars are read for every requests.
I'm all for the feature.
This will a have a negative impact on performances for messenger applications that uses a slow custom env-var loader or processor. |
env loaders/processors are used on web requests, so they have to be fast anyway. |
No, but that would probably need some doc on messenger page afterward :) |
If they process messages at high throughput, they are going to see a rapid impact on their infrastructure, with congested queues. I've been in that situation, I know it's the kind of change I would want to be notified about. |
I'm not sure I understand your concern. As I wrote, env loaders/processors are used on web requests, so they have to be fast anyway. Fast enough for high-throughput. |
Yes, I'm talking about an application that fetches certain variables from Hashicorp Vault, without using caches. As these variables were not used for the web, there were no problem. I wrote an article about how I solved this issue and having resetable env var loader is a very great feature for this use-case, but I just wanted to warn that it's a change in behavior that I think is too important to be released as a bugfix. |
fabbot errors are unrelated. |
reverted as per review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes sense to me.
Maybe we could also add a StaticEnvVarLoader that'd be a decorator of EnvVarLoaderInterface and that'd we use to decorate SodiumVault?
What would |
Index: src/Symfony/Bundle/FrameworkBundle/Secrets/StaticEnvVarLoader.php
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/src/Symfony/Bundle/FrameworkBundle/Secrets/StaticEnvVarLoader.php b/src/Symfony/Bundle/FrameworkBundle/Secrets/StaticEnvVarLoader.php
new file mode 100644
--- /dev/null (date 1714166958969)
+++ b/src/Symfony/Bundle/FrameworkBundle/Secrets/StaticEnvVarLoader.php (date 1714166958969)
@@ -0,0 +1,40 @@
+<?php
+
+/*
+ * This file is part of the Symfony package.
+ *
+ * (c) Fabien Potencier <fabien@symfony.com>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Symfony\Bundle\FrameworkBundle\Secrets;
+
+use Symfony\Component\DependencyInjection\Attribute\AsDecorator;
+use Symfony\Component\DependencyInjection\Attribute\Autowire;
+use Symfony\Component\DependencyInjection\Attribute\AutowireDecorated;
+use Symfony\Component\DependencyInjection\EnvVarLoaderInterface;
+
+/**
+ * @internal
+ */
+#[AsDecorator('secrets.vault')]
+class StaticEnvVarLoader implements EnvVarLoaderInterface
+{
+ private array $envCache = [];
+
+ public function __construct(
+ #[AutowireDecorated]
+ private EnvVarLoaderInterface $envVarLoader,
+ ) {
+ }
+
+ public function loadEnvVars(): array
+ {
+ if ([] !== $this->envCache) {
+ return $this->envCache;
+ }
+
+ return $this->envCache = $this->envVarLoader->loadEnvVars();
+ }
+} Like this? |
Yes! |
Done a888820 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
I just moved StaticEnvVarLoader to the DI component, completed the wiring in FrameworkBundle (we don't use autowiring) and tweaked a bit the messages in changelog.
Thank you @faizanakram99. |