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

fix: RequiresEnvironmentVariable should only allow strings #6079

Merged

Conversation

nikophil
Copy link
Contributor

@nikophil nikophil commented Dec 9, 2024

see #6074 (review)

some questions towards the behavior of the feature:

  • should I only test $_ENV or maybe $_ENV ?? getenv()?
  • when no value is provided, should I at least test the value of the env var is okish?
  • not sure what @mvorisek meant here by "in separate process"?

Copy link

codecov bot commented Dec 9, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.59%. Comparing base (7296d39) to head (aa67817).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff            @@
##               main    #6079   +/-   ##
=========================================
  Coverage     94.59%   94.59%           
- Complexity     6632     6634    +2     
=========================================
  Files           709      709           
  Lines         20862    20863    +1     
=========================================
+ Hits          19734    19735    +1     
  Misses         1128     1128           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@nikophil nikophil force-pushed the fix/requires-env-var branch 2 times, most recently from ad0bd10 to 366d30b Compare December 9, 2024 13:50
@nikophil nikophil force-pushed the fix/requires-env-var branch from 366d30b to aa67817 Compare December 9, 2024 13:53
@sebastianbergmann sebastianbergmann added this to the PHPUnit 12.0 milestone Dec 9, 2024
@sebastianbergmann
Copy link
Owner

when no value is provided, should I at least test the value of the env var is okish?

You mean when RequiresEnvironmentVariable is used without an expected value and only the existence of an environment variable is required? In that: no, I do not think that we need to perform any check beyond whether or not an environment variable of the specified name exists.

@sebastianbergmann
Copy link
Owner

should I only test $_ENV or maybe $_ENV ?? getenv()?

I almost never work with environment variables, so bear with me please: is there a situation where an environment variable is not accessible through $_ENV but is accessible through getenv() or vice versa?

@nikophil
Copy link
Contributor Author

nikophil commented Dec 10, 2024

I do not think that we need to perform any check beyond whether or not an environment variable of the specified name exists

Yes, that's what I've done.
The only check I've made is to ensure the env var is not an empty string, as suggested by @mvorisek

is there a situation where an environment variable is not accessible through $_ENV but is accessible through getenv() or vice versa?

I think when it is manually set, an env var can be added to only one or both: if only added to $_ENV, it is not accessible with getenv() and vice versa. symfony/dotenv does not even use putenv() by default

@sebastianbergmann sebastianbergmann merged commit b887eed into sebastianbergmann:main Dec 11, 2024
25 checks passed
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.

3 participants