Skip to content
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

Strict types renders env() unusable #878

Open
withinboredom opened this issue Jan 25, 2024 · 7 comments · May be fixed by #880
Open

Strict types renders env() unusable #878

withinboredom opened this issue Jan 25, 2024 · 7 comments · May be fixed by #880

Comments

@withinboredom
Copy link

Strict types hide and mask a lot of errors if not used correctly (((int) null) === 0 && ((string) null === '') for example, where a null would -- and probably should -- be an error). At the same time, PHP coercion is exceptionally well-documented and predictable. This is our reason for not using strict types, but I understand many people live by it.

Env variables are strings (at least in Linux) by default and will be passed as such. However, there's no way to cast these strings to ints, and due to strict types, they will not be coerced as expected:

ConnectionOptions::class => \DI\autowire( ConnectionOptions::class )
->constructorParameter( 'host', \DI\env( 'DATABASE_HOST' ) )
->constructorParameter( 'port', \DI\env( 'DATABASE_PORT' ) )
->constructorParameter( 'db', \DI\env( 'DATABASE') )
->constructorParameter( 'user', \DI\env( 'DATABASE_USER' ) )
->constructorParameter( 'password', \DI\env( 'DATABASE_PASSWORD' ) ),

This will fail to be constructed unless the programmer accepts a string for port, which would be pretty silly. Strict types apply on the caller, not the receiver, and only applies to scalar values as object values are always strict by default.

Since strict types only applies to scalar values and only when calling a function from that file, there are only several function calls where this applies in the entire file ObjectCreator.php, one of those being the constructor itself. It doesn't do any reflection actually to validate/cast the type either, so this will naturally fail:

Argument #2 ($port) must be of type int, string given, called in /app/vendor/php-di/php-di/src/Definition/Resolver/ObjectCreator.php on line 129

This library would be far more flexible and valuable to avoid strict types in this file, as it doesn't have much-realized value.

@mnapoli
Copy link
Member

mnapoli commented Jan 27, 2024

I'd rather solve this differently, for example by explicitly saying the env value should be casted to int (or something similar).

@withinboredom
Copy link
Author

As mentioned in the issue, casting null to int renders 0 and not null.

@withinboredom
Copy link
Author

I think this is what you're proposing:

https://3v4l.org/lL97f

While if we let PHP do it's thing:

https://3v4l.org/keP3I

We end up with a more correct implementation.

@mnapoli
Copy link
Member

mnapoli commented Jan 27, 2024

No, I meant something like:

->constructorParameter('port', \DI\env( 'DATABASE_PORT' )->asInt())

And '' and null should be turned into null.

@withinboredom
Copy link
Author

Ah, yes, that would be beautiful

@withinboredom
Copy link
Author

@mnapoli, do you have time to implement this or should I create a PR?

@mnapoli
Copy link
Member

mnapoli commented Feb 12, 2024

I will not implement this myself, so feel free to tackle this.

@withinboredom withinboredom linked a pull request Feb 12, 2024 that will close this issue
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 a pull request may close this issue.

2 participants