[DependencyInjection][FrameworkBundle] Introducing container non-empty parameters

[yceruto]

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 (null, '' or []). 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:

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

[nicolas-grekas]

I was wondering if we couldn't go with another API, namely a new argument to the constructor of the Parameter class?

new Parameter(string $id, string $errorOnEmpty = null)

Independently:
The name $container->requireParameter() made me think this would be checked ahead of time, not just before it's needed. In this regard, the naming is confusing to me because any parameter is required.

We could keep both approaches at once, but is it worth it?

[yceruto]

I was wondering if we couldn't go with another API, namely a new argument to the constructor of the Parameter class?

new Parameter(string $id, string $errorOnEmpty = null)

Looks good to me. I guess this error message would override the general one since it's more specific, right?

Independently:
The name $container->requireParameter() made me think this would be checked ahead of time, not just before it's needed. In this regard, the naming is confusing to me because any parameter is required.

Yeah, I wasn’t 100% convinced about this name/method. Following your previous idea, I think that adding $errorOnEmpty to the $container->setParameter() instead would be better?

$container->setParamater('kernel.secret', $value, 'A non-empty secret is required.');

We could keep both approaches at once, but is it worth it?

I think so. I’ll give it a try and see

[stof]

I would not add this in setParameter() as this would mean that each place setting the value of a parameter would have to define the proper validation (while in many cases, it could instead run the validation).

Adding this in the Parameter constructor would not help IMO. Most usages of this constructor are implicit through the %kernel.secret% syntax to reference parameters in argument values.
And this would also loose the benefit mentioned by @yceruto about automatically validating empty secrets for all third-party bundles using the parameter without any change on their side, because it would require them to opt-in for validation when referencing the parameter.

[yceruto]

I would not add this in setParameter() as this would mean that each place setting the value of a parameter would have to define the proper validation (while in many cases, it could instead run the validation).

I was thinking of setting it to null by default (programmatically, for BC) which maintains the current behavior. In other words, the non-empty validation will only run with a custom message if it's set. This means the third (optional) argument in setParameter() will serve two purposes: enabling non-empty validation and providing a custom error message. I'm not sure if this is intuitive or clear enough, though.

Adding this in the Parameter constructor would not help IMO. Most usages of this constructor are implicit through the %kernel.secret% syntax to reference parameters in argument values.

I agree, although some PHP definitions could benefit from it as well. Anyway, I'll explore that further once the initial proposal is solid.

[stof]

@yceruto but this setParameter method is called by file loaders when they find parameter definitions (and it is not called by FrameworkBundle). this puts the validation responsibility in the wrong place (if you ask end-user to enable validation, you could also ask them to pass a valid value instead, with the same effectiveness at catching the issue)

[yceruto]

@stof I see your point. That brings me back to the current proposal of a separate method that enables this validation, no matter how/where the parameter is defined.

I'm open to hearing other alternative names for requireParameter()... nonEmptyParameter() ?

[yceruto]

I updated the proposal terminology from "require" to "non-empty". Hopefully it's clearer now.

[yceruto]

By addressing the todo/comment below, many fixtures will be updated. Before continuing, I’d like to get confirmation that this approach is acceptable. Thanks!

[yceruto]

@symfony/mergers any objection to move forward in this direction? more changes in PhpDumper are coming...

[nicolas-grekas]

LGTM. Minor comments only :)

[yceruto]

Comments addressed (Windows failures look unrelated).

[yceruto]

Just rebased

[fabpot]

Are we sure we want to use empty()? The "0" string is probably a valid non-empty value, right? At least, this is how we're dealing with empty values everywhere else in the framework (routing comes to mind).

[mtarld]

Agreed, empty is returning true for false and 0 value as well, and that can sometimes be considered as a valid parameter values.

IMHO, the valid "empty" parameters values could be:

• ''
• []
• null

WDYT?

[yceruto]

Agreed! I've updated it to consider only null, '', and [] as empty parameter values.

[nicolas-grekas]

I'd add false personally. 0 is ok but false?

[yceruto]

After reconsidering, I don’t see false or 0 as empty values within their respective data types. For instance, a container parameter used as a toggle to enable or disable a feature would have false as a valid value in that context.

However, null (absence of value), an empty string '' (absence of chars), and an empty array [] (absence of elements). I can't reasoning the same way for bool, int or float data types.

[fabpot]

Thank you @yceruto.