Skip to content

[11.x] Change phpdoc for typed config getters #50287

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 6 commits into from
Feb 28, 2024

Conversation

Jubeki
Copy link
Contributor

@Jubeki Jubeki commented Feb 27, 2024

Follow up from #50140

You can only provide the correct typed parameter, otherwise it will always throw an InvalidArgumentException

Since this is for Laravel 11, you could also add a php type instead of phpdoc.

@Jubeki Jubeki changed the title Change phpdoc for typed config getters [11.x] Change phpdoc for typed config getters Feb 27, 2024
@taylorotwell
Copy link
Member

Could be a closure which returns these types.

@taylorotwell taylorotwell marked this pull request as draft February 28, 2024 02:02
@Jubeki
Copy link
Contributor Author

Jubeki commented Feb 28, 2024

@crynobone should the null type als be added as as a valid return type? Because currently the integer only can return integer.

Otherwise I would make these changes with Closures, which only return a string/integer/... and not allow a null value.

@crynobone
Copy link
Member

crynobone commented Feb 28, 2024

that's the correct value of $default if you want to not use mixed. Returning null would throw an exception, which can be handled by @throws phpdoc.

Jubeki and others added 5 commits February 28, 2024 10:45
Co-authored-by: Mior Muhammad Zaki <crynobone@gmail.com>
Co-authored-by: Mior Muhammad Zaki <crynobone@gmail.com>
Co-authored-by: Mior Muhammad Zaki <crynobone@gmail.com>
Co-authored-by: Mior Muhammad Zaki <crynobone@gmail.com>
Co-authored-by: Mior Muhammad Zaki <crynobone@gmail.com>
@Jubeki Jubeki marked this pull request as ready for review February 28, 2024 09:46
@AlexandreGerault
Copy link
Contributor

?int, ?string, ?bool should be the return types or change default to 0, '', and false

I believe the return type are correct. When using these typed getters you should except the value to be in a correct format. If the configuration is not, then it's because it's not correct. Thus expecting an exception is, for me, the right way.
null is not a valid value for me if you do:

Config::integer('key');

The default value should also be null by default as it is right now, as it is an optional argument. If you want 0, '' or similar just pass the default value then.

Config::integer('int-key', 0);
Config::string('str-key', '');

@ghost
Copy link

ghost commented Feb 28, 2024

?int, ?string, ?bool should be the return types or change default to 0, '', and false

I believe the return type are correct. When using these typed getters you should except the value to be in a correct format. If the configuration is not, then it's because it's not correct. Thus expecting an exception is, for me, the right way. null is not a valid value for me if you do:

Config::integer('key');

The default value should also be null by default as it is right now, as it is an optional argument. If you want 0, '' or similar just pass the default value then.

Config::integer('int-key', 0);
Config::string('str-key', '');

I saw after I expanded the lines. That is why i removed the comment. Thanks.

EDIT.
Still the dockblock should be updated with @throws

@taylorotwell taylorotwell merged commit fc636bd into laravel:master Feb 28, 2024
@Jubeki Jubeki deleted the config-phpdoc branch February 28, 2024 18:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants