-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[FrameworkBundle] Lazy kernel.secret
parameter resolving
#57462
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
Conversation
src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Configuration.php
Outdated
Show resolved
Hide resolved
src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Configuration.php
Outdated
Show resolved
Hide resolved
kernel.secret
empty by default
What about not defining the parameter instead when the value is not set? |
kernel.secret
empty by defaultkernel.secret
resolving on uri_signer
service
I didn't include any changelog as it's part of #56985. |
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'm wondering about what this change will achieve in terms of DX?
Also, what about remember-me and signed URLs that also use kernel.secret?
From what I tried, enabling esi+fragments currently triggers a compile time exception if kernel.secret is not defined:
You have requested a non-existent parameter "kernel.secret". Did you mean this: "kernel.charset"?
The message is not the best, but wouldn't the proposed way throw the same message, just at runtime vs compile time? Why is it better?
Then, if we really want a runtime exception, we could try adding a small internal factory that would throw a better error message if the secret is not configured?
src/Symfony/Bundle/FrameworkBundle/Resources/config/services.php
Outdated
Show resolved
Hide resolved
Still thinking about it... cause that we can improve the DX for those services but what about the rest? 3rd-party services requiring the param directly won't get any DX.
Updated
Yes, I'll go that way in the next step |
kernel.secret
resolving on uri_signer
servicekernel.secret
parameter resolving
At least for this PR I want to avoid the compiler-errors when none of these services are used. |
we could deprecate the parameter and replace it by a kernel.secret service (in the next step) |
Thank you @yceruto. |
…tainer non-empty parameters (yceruto) This PR was merged into the 7.2 branch. Discussion ---------- [DependencyInjection][FrameworkBundle] Introducing container non-empty parameters | Q | A | ------------- | --- | Branch? | 7.2 | Bug fix? | no | New feature? | yes | Deprecations? | no | Issues | - | License | MIT This new iteration (following up #57462, #56985 and symfony/recipes#1317) is about to improve the DX when we're dealing with optional parameters (this is the case for `kernel.secret` now and likely others out there) . Nicolas regarding your comment on #57462 (comment), I tried, but after some tests I realized that the impact of deprecating the `kernel.secret` is huge and, in some cases, counterproductive, as we used to reference that parameter in many configurations, see https://github.com/search?q=language%3APHP+%25kernel.secret%25+&type=code&p=3, which is currently a convenient way to share a config value. So I gave this new concept for container parameters a try. Basically, a non-empty parameter is one that must exist and cannot be [empty](https://www.php.net/manual/en/function.empty.php). It's evaluated when the `ParameterBag::get()` method is invoked. Additionally, we can now connect the parameter with its source by passing a custom error message with details on how to proceed if it fails, thus improving the DX. This is what we can achieve with this feature: ```php $container = new ContainerBuilder(); if (isset($config['secret'])) { $container->setParameter('app.secret', $config['secret']); } // NEW $container->nonEmptyParameter('app.secret', 'Did you forget to configure the "app.secret" option?'); $container->register('security_service', 'SecurityService') ->setArguments([new Parameter('app.secret')]) ->setPublic(true) ; ``` when the `security_service` is initiated/used, the `app.secret` parameter will be evaluated based on the non-empty conditions. If it's missing or empty, a helpful exception message will be thrown. Before (case when it's missing): ``` You have requested a non-existent parameter "app.secret". ``` After: ``` You have requested a non-existent parameter "app.secret". Did you forget to configure the "app.secret" option? ``` This would also address our concern about third-party services depending on the `kernel.secret` parameter when `APP_SECRET` is empty (and the `secrets` option is disabled). In that case, even if they are not checking for empty secret value in their own, it'll fail. Commits ------- 98156f7 Introducing container non-empty parameters
…tainer non-empty parameters (yceruto) This PR was merged into the 7.2 branch. Discussion ---------- [DependencyInjection][FrameworkBundle] Introducing container non-empty parameters | Q | A | ------------- | --- | Branch? | 7.2 | Bug fix? | no | New feature? | yes | Deprecations? | no | Issues | - | License | MIT This new iteration (following up symfony/symfony#57462, symfony/symfony#56985 and symfony/recipes#1317) is about to improve the DX when we're dealing with optional parameters (this is the case for `kernel.secret` now and likely others out there) . Nicolas regarding your comment on symfony/symfony#57462 (comment), I tried, but after some tests I realized that the impact of deprecating the `kernel.secret` is huge and, in some cases, counterproductive, as we used to reference that parameter in many configurations, see https://github.com/search?q=language%3APHP+%25kernel.secret%25+&type=code&p=3, which is currently a convenient way to share a config value. So I gave this new concept for container parameters a try. Basically, a non-empty parameter is one that must exist and cannot be [empty](https://www.php.net/manual/en/function.empty.php). It's evaluated when the `ParameterBag::get()` method is invoked. Additionally, we can now connect the parameter with its source by passing a custom error message with details on how to proceed if it fails, thus improving the DX. This is what we can achieve with this feature: ```php $container = new ContainerBuilder(); if (isset($config['secret'])) { $container->setParameter('app.secret', $config['secret']); } // NEW $container->nonEmptyParameter('app.secret', 'Did you forget to configure the "app.secret" option?'); $container->register('security_service', 'SecurityService') ->setArguments([new Parameter('app.secret')]) ->setPublic(true) ; ``` when the `security_service` is initiated/used, the `app.secret` parameter will be evaluated based on the non-empty conditions. If it's missing or empty, a helpful exception message will be thrown. Before (case when it's missing): ``` You have requested a non-existent parameter "app.secret". ``` After: ``` You have requested a non-existent parameter "app.secret". Did you forget to configure the "app.secret" option? ``` This would also address our concern about third-party services depending on the `kernel.secret` parameter when `APP_SECRET` is empty (and the `secrets` option is disabled). In that case, even if they are not checking for empty secret value in their own, it'll fail. Commits ------- 98156f7d64 Introducing container non-empty parameters
#56985 and symfony/recipes#1317 following up
The goal of this PR is to fix the current compiler-errors about a missing
kernel.secret
parameter when it's not set at all. Thus, improving the first-time experience with minimalistic apps.