feat: create attribute #[WithEnvironmentVariable]#6126
feat: create attribute #[WithEnvironmentVariable]#6126nikophil wants to merge 2 commits intosebastianbergmann:mainfrom
#[WithEnvironmentVariable]#6126Conversation
| foreach ($environmentVariables as $environmentVariableName => $environmentVariableValue) { | ||
| assert($metadata instanceof WithEnvironmentVariable); | ||
|
|
||
| $this->backupEnvironmentVariables[$environmentVariableName] = $_ENV[$environmentVariableName] ?? getenv($environmentVariableName); |
There was a problem hiding this comment.
I was torn on this... in PHP, nothing prevents to have a different value for $_ENV['VAR'] and getenv('VAR').
Do you think I should store both representations, in order to have 100% the same environment afterand before the test?
There was a problem hiding this comment.
any hints how I should store this?
my first guess would be to store this in an array:
/**
* @var array{getenv: array<string, false|string>, global: array<string, false|string>}
*/
private array $backupEnvironmentVariables = ['getenv' => [], 'global' => []];but I don't like it. Another idea would be to use a value object:
final readonly class BackupEnvironmentVariable
{
public static function asGetenv(): self;
public static function asSuperGlobal(): self;
}if so, in which namespace should i put this class?
There was a problem hiding this comment.
I've updated the code to use a value object. (some tests are failing, I'll check them after we agree on the implementation)
| #[WithEnvironmentVariable('BAZ', '1')] | ||
| #[WithEnvironmentVariable('BAZ', '2')] | ||
| #[WithEnvironmentVariable('BAZ', '3')] | ||
| public function testMultipleAttributesKeepTheLastValue(): void |
There was a problem hiding this comment.
maybe we should throw an exception for this case?
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #6126 +/- ##
============================================
+ Coverage 95.29% 95.30% +0.01%
- Complexity 6755 6774 +19
============================================
Files 726 728 +2
Lines 21300 21361 +61
============================================
+ Hits 20298 20359 +61
Misses 1002 1002 ☔ View full report in Codecov by Sentry. |
9844b03 to
b582dda
Compare
| public static function setUpBeforeClass(): void | ||
| { | ||
| self::assertEnvironmentVariablesHaveDefaultValues(); | ||
| } | ||
|
|
||
| protected function setUp(): void | ||
| { | ||
| $this->assertEnvironmentVariablesHaveCustomValues(); | ||
| } | ||
|
|
||
| protected function tearDown(): void | ||
| { | ||
| $this->assertEnvironmentVariablesHaveCustomValues(); | ||
| } | ||
|
|
||
| public static function tearDownAfterClass(): void | ||
| { | ||
| self::assertEnvironmentVariablesHaveDefaultValues(); | ||
| } |
There was a problem hiding this comment.
not sure of what would be the desired behavior here 🤔
| #[WithEnvironmentVariable('BAZ', '1')] | ||
| #[RequiresEnvironmentVariable('BAZ', '1')] | ||
| public function testUsingAlongWithRequiresEnvironmentVariableAttribute(): void | ||
| { | ||
| $this->assertSame('1', $_ENV['BAZ']); | ||
| $this->assertSame('1', getenv('BAZ')); | ||
| } |
There was a problem hiding this comment.
not really sure how these two attributes should interact...
that's a bit odd to use them both on the same test, tho...
There was a problem hiding this comment.
Ideally, PHPUnit would emit a warning when WithEnvironmentVariable and RequiresEnvironmentVariable are 1) used on the same test and 2) refer to the same environment variable. But this can be done in a future PR.
#[WithEnvironmentVariable]#[WithEnvironmentVariable]
b582dda to
fa7faea
Compare
fa7faea to
48d3ead
Compare
48d3ead to
71dc843
Compare
|
Merged manually, thanks. |
cool, thanks :) out of curiosity: why do you sometimes close the MR and merge manually? BTW, the tests were failing 🤔 |
|
In this case I did it manually because I wanted to fix the build 😄 |
|
haha sorry for this, I was waiting for your approval about the implementation before fixing the tests 😅 anyway, I'm glad this was merged 👍 |
fixes #6116