Skip to content

[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

Merged
merged 1 commit into from
Jun 29, 2024

Conversation

yceruto
Copy link
Member

@yceruto yceruto commented Jun 20, 2024

Q A
Branch? 7.2
Bug fix? no
New feature? yes
Deprecations? no
Issues -
License MIT

#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.

@yceruto yceruto requested a review from nicolas-grekas June 20, 2024 12:07
@carsonbot carsonbot added this to the 7.2 milestone Jun 20, 2024
@yceruto yceruto changed the title Make kernel.secret empty by default [FrameworkBundle] Make kernel.secret empty by default Jun 20, 2024
@carsonbot carsonbot changed the title [FrameworkBundle] Make kernel.secret empty by default Make kernel.secret empty by default Jun 24, 2024
@yceruto yceruto changed the title Make kernel.secret empty by default [FrameworkBundle] Make kernel.secret empty by default Jun 24, 2024
@OskarStark OskarStark changed the title [FrameworkBundle] Make kernel.secret empty by default [FrameworkBundle] Make kernel.secret empty by default Jun 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?

@yceruto yceruto changed the title [FrameworkBundle] Make kernel.secret empty by default [FrameworkBundle] Lazy kernel.secret resolving on uri_signer service Jun 26, 2024
@yceruto
Copy link
Member Author

yceruto commented Jun 26, 2024

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

Copy link
Member

@nicolas-grekas nicolas-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
Member Author

yceruto commented Jun 26, 2024

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

@yceruto yceruto changed the title [FrameworkBundle] Lazy kernel.secret resolving on uri_signer service [FrameworkBundle] Lazy kernel.secret parameter resolving Jun 26, 2024
@yceruto
Copy link
Member Author

yceruto commented Jun 26, 2024

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-grekas nicolas-grekas merged commit 4cdead1 into symfony:7.2 Jun 29, 2024
5 of 10 checks passed
@yceruto yceruto deleted the secret_param branch June 29, 2024 16:41
fabpot added a commit that referenced this pull request Sep 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       | 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
symfony-splitter pushed a commit to symfony/framework-bundle that referenced this pull request Sep 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       | 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants