Stop relying on the $mode argument #21986
Conversation
When registering the error handler, simple-phpunit might be used, and in that case, the bootstrap process will not have environment variables defined inside phpunit.xml.dist . This means `$mode` might differ when registering the error handler, and when an error is triggered. This raises a question: should the $mode argument be removed to avoid similar errors in the future?
|
@nicolas-grekas it would imply doing the following change : -$getMode = function () use ($mode) {
+$getMode = function () {
static $memoizedMode = false;
if (false !== $memoizedMode) {
return $memoizedMode;
}
- if (false === $mode) {
- $mode = getenv('SYMFONY_DEPRECATIONS_HELPER');
- }
+ $mode = getenv('SYMFONY_DEPRECATIONS_HELPER');
if (DeprecationErrorHandler::MODE_WEAK !== $mode && DeprecationErrorHandler::MODE_WEAK_VENDORS !== $mode && (!isset($mode[0]) || '/' !== $mode[0])) {
$mode = preg_match('/^[1-9][0-9]*$/', $mode) ? (int) $mode : 0;
}
return $memoizedMode = $mode;
};Not sure if dropping support for that kind of usage would be acceptable, so I think I see your point now. |
|
The env var is not and should become the only way to configure the bridge. |
|
Failed to parse your sentence 😕 |
Not sure, I think it's fair to use if it has a valid value and try to get the env variable only if is
Actually the bug occurs with IMO this piece of could should work seamlessly with both PhpUnit and SimplePhpUnit. Two ways to avoid the issue you had [doing this kind of error]:
|
|
@nicolas-grekas Oh you meant "and should not"? If yes I understand and agree. |
|
@theofidry I did not reproduce the bug with phpunit though… did you? When you call phpunit, the bootstrap script is just |
|
@greg0ire I did in https://github.com/theofidry/phpunitbridge-demo1. Tbh I didn't even try with |
|
Isn't this a bugfix and should then be merged into lower branches too? |
|
It's a bugfix, but the feature is not released yet |
|
Sorry, missed that this change was introduced in #21539. 👍 |
|
No problem, thanks for caring! |
|
Thank you @greg0ire. |
This PR was merged into the 3.3-dev branch. Discussion ---------- Stop relying on the $mode argument When registering the error handler, simple-phpunit might be used, and in that case, the bootstrap process will not have environment variables defined inside phpunit.xml.dist . This means `$mode` might differ when registering the error handler, and when an error is triggered. This raises a question: should the $mode argument be removed to avoid similar errors in the future? | Q | A | ------------- | --- | Branch? | master | Bug fix? | yes | New feature? | no | BC breaks? | yes/no | Deprecations? | yes/no <!-- don't forget updating UPGRADE-*.md files --> | Tests pass? | yes/no | Fixed tickets | #21980 | License | MIT | Doc PR | n/a <!-- - Bug fixes must be submitted against the lowest branch where they apply (lowest branches are regularly merged to upper ones so they get the fixes too). - Features and deprecations must be submitted against the master branch. - Please fill in this template according to the PR you're about to submit. - Replace this comment by a description of what your PR is solving. --> Commits ------- 0d180d5 Stop relying on the $mode argument
When registering the error handler, simple-phpunit might be used, and in
that case, the bootstrap process will not have environment variables
defined inside phpunit.xml.dist . This means
$modemight differ whenregistering the error handler, and when an error is triggered.
This raises a question: should the $mode argument be removed to avoid
similar errors in the future?