Default assert.exception to 1#5925
Default assert.exception to 1#5925php-pulls merged 1 commit intophp:masterphp/php-src:masterfrom morrisonlevi:levim/assert-throwmorrisonlevi/php-src:levim/assert-throwCopy head branch name to clipboard
Conversation
| @@ -77,7 +77,7 @@ PHP_INI_BEGIN() | ||
| STD_PHP_INI_BOOLEAN("assert.bail", "0", PHP_INI_ALL, OnUpdateBool, bail, zend_assert_globals, assert_globals) | ||
| STD_PHP_INI_BOOLEAN("assert.warning", "1", PHP_INI_ALL, OnUpdateBool, warning, zend_assert_globals, assert_globals) |
There was a problem hiding this comment.
I think not. Exception takes priority over warning, so it doesn't issue a warning and then throw; it only throws. This way reverting to PHP < 8 behavior is only assert.exception=0.
There was a problem hiding this comment.
It seems pretty odd from a documentation perspective to have assert.warning=1 without actually enabling warnings. Ideally this would have been assert.mode=exception or so, because really only one of assert.bail, assert.exception and assert.warning makes sense at a time. But given things as they are, I would set it to zero.
|
Doesn't this change need to go through the RFC process? EDIT: It looks like something related was started here, but never completed. |
|
@GrahamCampbell, there was no object on internals so no |
|
Isn't that what the purpose of the RFC system is to establish? |
|
@GrahamCampbell Do you have a specific concern about the change itself? |
|
I think discussion over the default value |
|
IMO we should leave it as This area needs overhauled but nobody cared to do it in time for PHP 8.0. For example, |
|
@GrahamCampbell well yes and no, but not everything is going through the rfc system if there is a consensus like there was with this. It is all very subjective to a case by case basis. Edit: If we go with the RFC process for the warning part here, then that part will not make it for 8.0, something I would argue that is related to this. If your argument for using the RFC issue is that more expert eyes see it, then perhaps those expert eyes should be reading internals actual in the first place and I put their concern there, as it’s meant for. |
nikic
left a comment
There was a problem hiding this comment.
Okay, I don't care particularly strongly about the assert.warning part. This LGTM.
As discussed on the mailing list, this changes the default of
assert.exceptionfrom0to1, meaning exceptions will now throw by default. To restore the old value setassert.exception=0in an INI file.I opted to leave existing tests as written and instead just changed the
--INI--section to have the old value.