Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

[FrameworkBundle] Lazykernel.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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
nicolas-grekas merged 1 commit intosymfony:7.2fromyceruto:secret_param
Jun 29, 2024

Conversation

yceruto
Copy link
Member

@ycerutoyceruto commentedJun 20, 2024
edited
Loading

QA
Branch?7.2
Bug fix?no
New feature?yes
Deprecations?no
Issues-
LicenseMIT

#56985 andsymfony/recipes#1317 following up

The goal of this PR is to fix the current compiler-errors about a missingkernel.secret parameter when it's not set at all. Thus, improving the first-time experience with minimalistic apps.

valtzu reacted with thumbs up emoji
@carsonbotcarsonbot added this to the7.2 milestoneJun 20, 2024
@ycerutoyceruto changed the titleMake kernel.secret empty by default[FrameworkBundle] Make kernel.secret empty by defaultJun 20, 2024
@carsonbotcarsonbot changed the title[FrameworkBundle] Make kernel.secret empty by defaultMake kernel.secret empty by defaultJun 24, 2024
@ycerutoyceruto changed the titleMake kernel.secret empty by default[FrameworkBundle] Make kernel.secret empty by defaultJun 24, 2024
@OskarStarkOskarStark changed the title[FrameworkBundle] Make kernel.secret empty by default[FrameworkBundle] Makekernel.secret empty by defaultJun 25, 2024
@nicolas-grekas
Copy link
Member

But what if a third-party service is using the empty secret and does not throw an exception in such a case?

What about not defining the parameter instead when the value is not set?

@ycerutoyceruto changed the title[FrameworkBundle] Makekernel.secret empty by default[FrameworkBundle] Lazykernel.secret resolving onuri_signer serviceJun 26, 2024
@yceruto
Copy link
MemberAuthor

I didn't include any changelog as it's part of#56985.

Copy link
Member

@nicolas-grekasnicolas-grekas left a 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?

@yceruto
Copy link
MemberAuthor

I'm wondering about what this change will achieve in terms of DX?

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.

Also, what about remember-me and signed URLs that also use kernel.secret?

Updated

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?

Yes, I'll go that way in the next step

@ycerutoyceruto changed the title[FrameworkBundle] Lazykernel.secret resolving onuri_signer service[FrameworkBundle] Lazykernel.secret parameter resolvingJun 26, 2024
@yceruto
Copy link
MemberAuthor

yceruto commentedJun 26, 2024
edited
Loading

At least for this PR I want to avoid the compiler-errors when none of these services are used.

@nicolas-grekas
Copy link
Member

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.

we could deprecate the parameter and replace it by a kernel.secret service (in the next step)

@nicolas-grekas
Copy link
Member

Thank you@yceruto.

@nicolas-grekasnicolas-grekas merged commit4cdead1 intosymfony:7.2Jun 29, 2024
5 of 10 checks passed
@ycerutoyceruto deleted the secret_param branchJune 29, 2024 16:41
fabpot added a commit that referenced this pull requestSep 19, 2024
…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       | MITThis new iteration (following up#57462,#56985 andsymfony/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, seehttps://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
symfony-splitter pushed a commit to symfony/framework-bundle that referenced this pull requestSep 19, 2024
…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       | MITThis new iteration (following upsymfony/symfony#57462,symfony/symfony#56985 andsymfony/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 onsymfony/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, seehttps://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
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@nicolas-grekasnicolas-grekasnicolas-grekas approved these changes

@fabpotfabpotfabpot requested changes

@chalasrchalasrAwaiting requested review from chalasrchalasr is a code owner

Assignees
No one assigned
Projects
None yet
Milestone
7.2
Development

Successfully merging this pull request may close these issues.

4 participants
@yceruto@nicolas-grekas@fabpot@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp