[11.x] Add typed getters for configuration

[AlexandreGerault]

Hi

I've been struggling sometimes with Laravel and Larastan on high levels (8 & 9 mostly). Indeed, the configuration helper config give values as mixed which requires annotation or assertion each time it is used to satisfy static analysis. It gives this kind of errors:

Parameter #1 $config of method Class::method() expects array, mixed given.

I think it can be great to have, like the Request object, typed getters, like ->string($key);, ->integer($key); etc.

This would make it much more easier to satisfy static analysis. Only array might be complaining sometimes.

[henzeb]

interesting idea. But the Contract changes are breaking ones. If this wants to be added, this should be done in a major version like 11.

[AlexandreGerault]

Thanks for the answer. I wonder how the contracts are breaking changes since I just added new methods 🤔 Is the contract used somewhere else?

[driesvints]

Is the contract used somewhere else?

That's the whole point of a contract 🙂 so it can be used somewhere else. Can you revert the changes to the contract and mark this as ready?

[AlexandreGerault]

I can but how do we make sure the contract would be updated on the next major release and won't be forgotten?

Is that a good idea to merge without the contract being changed?

[driesvints]

To be fair. I don’t think this pr will get accepted as it’s easy enough to typecast. I feel like this is a bit of feature creep. You can either remove the changes to the contract here, mark the pr as ready for review or sent it in to master.

[AlexandreGerault]

I think it's worth to have these methods, as we do for request.

Nowadays we often tend to use static analysis tools to improve our codebase quality and spot bugs before it can ever happen. Using the config helper will always bring error on strict Larastan configuration as it returns mixed.

Today the only way to make sure we comply with strict static analysis is to either use annotation to type the return of config calls, either to use assertions everytime we use the config helper. In my opinion this can become cumbersome very quickly.

Instead of doing this:

$normalizers = config('serializer.normalizers');

Assert::isArray($normalizers); // works for static analysis when having the webmozart/assert extension installed

Serializer::make($normalizers);

or

$normalizers = config('serializer.normalizers');

if (!is_array($normalizers)) {
    throw new InvalidArgumentException("The normalizers config must be an array");
 }
 
Serializer::make($normalizers);

We can do

Serializer::make(config()->array('serializer.normalizers'));

This make it so much easier on a larger codebase and also clearer to read.

Maybe I'm wrong but I don't see any real drawbacks with this 🤔

EDIT: I know it's also possible to just type hint with annotations like so:

/** @var array $normalizers */
$normalizers = config('serializer.normalizers');
 
Serializer::make($normalizers);

But I believe it's still cumbersome to this every time. Yet I understand your point of it being feature creep 😄 I just thought Laravel was going more typed so I thought this could be a good idea

[AlexandreGerault]

I just rebased the branch correctly so it doesn't include commits from 10.x 😅

[taylorotwell]

I think you can just drop the methods from the contract entirely and only put them on the repository. 👍 Also you need docblocks like the rest of the framework for consistency. Please mark as ready for review again when you want me to take another look.

[AlexandreGerault]

I think you can just drop the methods from the contract entirely and only put them on the repository. 👍 Also you need docblocks like the rest of the framework for consistency. Please mark as ready for review again when you want me to take another look.

Should I also support the default parameter in the methods? Just realized now, but I think it makes sens. However I don't think it makes sens to support many keys here 🤔

/**
 * Get the specified configuration value typed as a string.
 * If the value isn't a string it should throw an exception.
 *
 * @param  string  $key
 * @param  ?string  $default
 * @return string
 */
public function string(string $key, ?string $default = null): string
{
    $value = $this->get($key, $default);

    if (! is_string($value)) {
        throw new \InvalidArgumentException(
            sprintf('Configuration value for key [%s] must be a string, %s given.', $key, gettype($value))
        );
    }

    return $value;
}

[AlexandreGerault]

I think default values can be added on a next PR if that's not the moment 😄