-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
#[RequiresEnvironmentVariable]
#6074
#[RequiresEnvironmentVariable]
#6074
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #6074 +/- ##
============================================
+ Coverage 94.57% 94.59% +0.01%
- Complexity 6615 6632 +17
============================================
Files 707 709 +2
Lines 20809 20862 +53
============================================
+ Hits 19681 19734 +53
Misses 1128 1128 ☔ View full report in Codecov by Sentry. |
#[RequiresEnvironmentVariable]
final readonly class RequiresEnvironmentVariable | ||
{ | ||
private string $environmentVariableName; | ||
private null|bool|int|string $value; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Environment variable can be strictly only string.
What is the meaning of null
, bool
, int
?
Can I test environment variable presence/absence /wo specific value?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on https://github.com/sebastianbergmann/phpunit/pull/6074/files#diff-8f0e8d461aea381bf08d49440fbc5da892d38eb782518cd44bfe1dddce7638f4R119 it seems bool
and int
should be removed and as never possible - see https://www.php.net/manual/en/function.getenv.php return type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can I test environment variable presence/absence /wo specific value?
you can only test environment variable presence or with a specific value
it seems bool and int should be removed and as never possible
this was more or less my concern here. I'm also wondering if we should only check the env var in $_ENV
, or use getenv()
or check at least one of them
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any thoughts @sebastianbergmann? I can provide a patch to my PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this was more or less my concern here. I'm also wondering if we should only check the env var in
$_ENV
, or usegetenv()
or check at least one of them
Please test the behaviour with E2E test and some tests in separate process. The separate process must see the same environment variables in both linux and Windows.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair enough.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you think in the case of "no value", I check if the env var exist, or should I check it is ok-ish? empty string and '0'
would not pass
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my opinion:
-
If an environment variable is tested for a presence (/wo value defined in the Attribute), I would expect
'0'
to be consideted as set. -
For binary
'1'
/'0'
detection I would expect user to write#[RequiresEnvironmentVariable('foo', '1')]
explicitly. -
''
should be considered as unset as in many operating systemsFOO=
unsets the variable. Also, in CI parametrization/matrix empty value might be set, but with "no variable" meaning.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'' should be considered as unset as in many operating systems FOO= unsets the variable
yeah, good point
Implements #6065