Skip to content

Conversation

@ddevsr
Copy link
Collaborator

@ddevsr ddevsr commented Feb 27, 2025

Description
default not sync with return

env('myconfig.isLogApi', config(MyConfig::class)->isLogApi); // bool
env('myconfig.httpVersion', config(MyConfig::class)->httpVersion); // float
env('myconfig.timeout', config(MyConfig::class)->timeout); // int

Checklist:

  • Securely signed commits
  • Component(s) with PHPDoc blocks, only if necessary or adds value
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

@ddevsr ddevsr added the refactor Pull requests that refactor code label Feb 27, 2025
@ddevsr ddevsr requested a review from michalsn February 27, 2025 02:29
Copy link
Contributor

@neznaika0 neznaika0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We get rid of "mixed" wherever possible. array<int|string, mixed> is better.

Copy link
Member

@paulbalandan paulbalandan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hey, can this be better suited if we use generics instead?

@ddevsr
Copy link
Collaborator Author

ddevsr commented Feb 27, 2025

Do you mean like this? i think better

--- a/system/Common.php
+++ b/system/Common.php
@@ -371,9 +371,11 @@ if (! function_exists('env')) {
      * retrieving values set from the .env file for
      * use in config files.
      *
-     * @param array<int|string, mixed>|bool|float|int|string|null $default
+     * @template T
      *
-     * @return array<int|string, mixed>|bool|float|int|string|null        
+     * @param T|null $default
+     *
+     * @return T|null
      */
     function env(string $key, $default = null)
     {

@paulbalandan
Copy link
Member

I think you can drop the null part in the phpdocs: see https://phpstan.org/r/b764e608-6ea7-4edf-aa2d-5821cc80057b. But please try again with your examples.

@ddevsr
Copy link
Collaborator Author

ddevsr commented Feb 27, 2025

When i set default from my config property is passed. The problem is $default not set, developer needed recasting like (string) env('myconfig.title').

EDIT : Now all passed using @return ($default is null ? array<int|string, mixed>|bool|float|int|string|null : T)

@ddevsr
Copy link
Collaborator Author

ddevsr commented Feb 27, 2025

@paulbalandan I'm not familiar with psalm

@ddevsr ddevsr requested a review from paulbalandan February 27, 2025 10:22
Copy link
Member

@paulbalandan paulbalandan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, please disregard the generics idea. I tried tinkering it on the phpstan playground and I got no luck.

@ddevsr
Copy link
Collaborator Author

ddevsr commented Mar 3, 2025

Local and runner already passed

https://phpstan.org/r/a5f5dca8-279a-4e60-ae66-6468e77afb14

@paulbalandan
Copy link
Member

The thing is $default does not determine the return of the function if it is null as it can be a valid return value itself if the key is not found.

@paulbalandan paulbalandan merged commit 6acd5b9 into codeigniter4:develop Mar 7, 2025
49 checks passed
@ddevsr ddevsr deleted the env-phpdocs branch March 8, 2025 14:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

refactor Pull requests that refactor code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants