From a08f9f10f72995704709aa15b8f712a88010aa0e Mon Sep 17 00:00:00 2001 From: Alex Dowad Date: Tue, 9 Jun 2020 11:34:40 +0200 Subject: [PATCH 1/4] assert always receives default description as 2nd argument if called with only 1 argument There seems to be no reason for the extra conditions here. --- Zend/zend_compile.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/Zend/zend_compile.c b/Zend/zend_compile.c index a2c8b9f23e741..08f596818f6da 100644 --- a/Zend/zend_compile.c +++ b/Zend/zend_compile.c @@ -3704,10 +3704,8 @@ static void zend_compile_assert(znode *result, zend_ast_list *args, zend_string } opline->result.num = zend_alloc_cache_slot(); - if (args->children == 1 && - (args->child[0]->kind != ZEND_AST_ZVAL || - Z_TYPE_P(zend_ast_get_zval(args->child[0])) != IS_STRING)) { - /* add "assert(condition) as assertion message */ + if (args->children == 1) { + /* add "assert(condition)" as default assertion description */ zend_ast_list_add((zend_ast*)args, zend_ast_create_zval_from_str( zend_ast_export("assert(", args->child[0], ")"))); From 74bc19c0163f418ced21baf3a711ee42a4d23402 Mon Sep 17 00:00:00 2001 From: Alex Dowad Date: Tue, 9 Jun 2020 11:42:37 +0200 Subject: [PATCH 2/4] Rename second parameter to assert, to reflect that it may be a Throwable object This parameter is not always a description, but may be a Throwable object which will be thrown if the assertion fails. --- ext/standard/assert.c | 28 +++++++++++++++------------- 1 file changed, 15 insertions(+), 13 deletions(-) diff --git a/ext/standard/assert.c b/ext/standard/assert.c index cc49786aa793a..0320b3914849d 100644 --- a/ext/standard/assert.c +++ b/ext/standard/assert.c @@ -139,8 +139,10 @@ PHP_MINFO_FUNCTION(assert) /* {{{ */ Checks if assertion is false */ PHP_FUNCTION(assert) { - zval *assertion; - zval *description = NULL; + /* `desc_or_throwable` may either be a description string which will be used in + * the message for a failed assertion, or a `Throwable` object which will be + * thrown if the assertion fails */ + zval *assertion, *desc_or_throwable = NULL; if (! ASSERTG(active)) { RETURN_TRUE; @@ -149,7 +151,7 @@ PHP_FUNCTION(assert) ZEND_PARSE_PARAMETERS_START(1, 2) Z_PARAM_ZVAL(assertion) Z_PARAM_OPTIONAL - Z_PARAM_ZVAL(description) + Z_PARAM_ZVAL(desc_or_throwable) ZEND_PARSE_PARAMETERS_END(); if (zend_is_true(assertion)) { @@ -173,12 +175,12 @@ PHP_FUNCTION(assert) ZVAL_FALSE(&retval); /* XXX do we want to check for error here? */ - if (!description) { + if (!desc_or_throwable) { call_user_function(NULL, NULL, &ASSERTG(callback), &retval, 3, args); zval_ptr_dtor(&(args[2])); zval_ptr_dtor(&(args[0])); } else { - ZVAL_STR(&args[3], zval_get_string(description)); + ZVAL_STR(&args[3], zval_get_string(desc_or_throwable)); call_user_function(NULL, NULL, &ASSERTG(callback), &retval, 4, args); zval_ptr_dtor(&(args[3])); zval_ptr_dtor(&(args[2])); @@ -189,22 +191,22 @@ PHP_FUNCTION(assert) } if (ASSERTG(exception)) { - if (!description) { + if (!desc_or_throwable) { zend_throw_exception(assertion_error_ce, NULL, E_ERROR); - } else if (Z_TYPE_P(description) == IS_OBJECT && - instanceof_function(Z_OBJCE_P(description), zend_ce_throwable)) { - Z_ADDREF_P(description); - zend_throw_exception_object(description); + } else if (Z_TYPE_P(desc_or_throwable) == IS_OBJECT && + instanceof_function(Z_OBJCE_P(desc_or_throwable), zend_ce_throwable)) { + Z_ADDREF_P(desc_or_throwable); + zend_throw_exception_object(desc_or_throwable); } else { - zend_string *str = zval_get_string(description); + zend_string *str = zval_get_string(desc_or_throwable); zend_throw_exception(assertion_error_ce, ZSTR_VAL(str), E_ERROR); zend_string_release_ex(str, 0); } } else if (ASSERTG(warning)) { - if (!description) { + if (!desc_or_throwable) { php_error_docref(NULL, E_WARNING, "Assertion failed"); } else { - zend_string *str = zval_get_string(description); + zend_string *str = zval_get_string(desc_or_throwable); php_error_docref(NULL, E_WARNING, "%s failed", ZSTR_VAL(str)); zend_string_release_ex(str, 0); } From ebfe425c504765dec2f597f4d578b87e7deadaf3 Mon Sep 17 00:00:00 2001 From: Alex Dowad Date: Tue, 9 Jun 2020 11:45:35 +0200 Subject: [PATCH 3/4] Unify duplicate code in assert It is not necessary to free `args[2]`, since it is always set to `null`. --- ext/standard/assert.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/ext/standard/assert.c b/ext/standard/assert.c index 0320b3914849d..f2b69d9ed25df 100644 --- a/ext/standard/assert.c +++ b/ext/standard/assert.c @@ -177,16 +177,12 @@ PHP_FUNCTION(assert) /* XXX do we want to check for error here? */ if (!desc_or_throwable) { call_user_function(NULL, NULL, &ASSERTG(callback), &retval, 3, args); - zval_ptr_dtor(&(args[2])); - zval_ptr_dtor(&(args[0])); } else { ZVAL_STR(&args[3], zval_get_string(desc_or_throwable)); call_user_function(NULL, NULL, &ASSERTG(callback), &retval, 4, args); zval_ptr_dtor(&(args[3])); - zval_ptr_dtor(&(args[2])); - zval_ptr_dtor(&(args[0])); } - + zval_ptr_dtor(&(args[0])); zval_ptr_dtor(&retval); } From 535143014ea06eb68f3be58161e1c025de794786 Mon Sep 17 00:00:00 2001 From: Alex Dowad Date: Tue, 9 Jun 2020 11:54:29 +0200 Subject: [PATCH 4/4] Pass assertion condition (as string) as 3rd argument of assertion callback The documentation on `assert` (https://www.php.net/manual/en/function.assert.php) states that an assertion callback will receive the assertion condition as its 3rd argument. This was easy to do when assertion conditions were represented as strings. However, with the implementation of the expectations RFC, this is no longer the case. Therefore, since that time, the documentation has been incorrect. Bug #79602 was opened regarding this issue. This is a somewhat hacky solution which adds a 3rd argument to `assert`, which is inserted during compilation. The 3rd argument is not documented, and is not intended to be; it is an internal implementation detail. Of course, the sucky thing is that the 'hidden' 3rd argument still appears in stack traces. If you try to call `assert` with too many arguments, its existence is also betrayed by the way PHP complains that `assert` should be called with 3 arguments. As a side point, the 'description' which is printed as part of the assertion warning message looks goofy if the 2nd argument to `assert` is actually a Throwable. --- Zend/tests/assert/expect_002.phpt | 2 +- Zend/tests/assert/expect_009.phpt | 2 +- Zend/tests/assert/expect_010.phpt | 2 +- Zend/tests/assert/expect_011.phpt | 2 +- Zend/tests/assert/expect_021.phpt | 40 +++++++++++++++++++++++++++++++ Zend/tests/exception_011.phpt | 2 +- Zend/zend_compile.c | 6 +++++ ext/standard/assert.c | 14 +++++++---- 8 files changed, 61 insertions(+), 9 deletions(-) create mode 100644 Zend/tests/assert/expect_021.phpt diff --git a/Zend/tests/assert/expect_002.phpt b/Zend/tests/assert/expect_002.phpt index 3da88e6ece557..a5e34a7e7ca7c 100644 --- a/Zend/tests/assert/expect_002.phpt +++ b/Zend/tests/assert/expect_002.phpt @@ -11,6 +11,6 @@ var_dump(true); --EXPECTF-- Fatal error: Uncaught AssertionError: assert(false) in %sexpect_002.php:%d Stack trace: -#0 %sexpect_002.php(%d): assert(false, 'assert(false)') +#0 %sexpect_002.php(%d): assert(false, 'assert(false)', 'false') #1 {main} thrown in %sexpect_002.php on line %d diff --git a/Zend/tests/assert/expect_009.phpt b/Zend/tests/assert/expect_009.phpt index 984a9bf29e8b0..d41c95e9149a9 100644 --- a/Zend/tests/assert/expect_009.phpt +++ b/Zend/tests/assert/expect_009.phpt @@ -19,7 +19,7 @@ new Two(); --EXPECTF-- Fatal error: Uncaught AssertionError: assert(false) in %sexpect_009.php:%d Stack trace: -#0 %sexpect_009.php(%d): assert(false, 'assert(false)') +#0 %sexpect_009.php(%d): assert(false, 'assert(false)', 'false') #1 %sexpect_009.php(%d): Two->__construct() #2 {main} thrown in %sexpect_009.php on line %d diff --git a/Zend/tests/assert/expect_010.phpt b/Zend/tests/assert/expect_010.phpt index c3665212fc1dc..506a4116f4873 100644 --- a/Zend/tests/assert/expect_010.phpt +++ b/Zend/tests/assert/expect_010.phpt @@ -17,7 +17,7 @@ new Two(); --EXPECTF-- Fatal error: Uncaught AssertionError: assert(false) in %sexpect_010.php:%d Stack trace: -#0 %sexpect_010.php(%d): assert(false, 'assert(false)') +#0 %sexpect_010.php(%d): assert(false, 'assert(false)', 'false') #1 %sexpect_010.php(%d): One->__construct() #2 {main} thrown in %sexpect_010.php on line %d diff --git a/Zend/tests/assert/expect_011.phpt b/Zend/tests/assert/expect_011.phpt index e68852e69f8cc..3b6ed6ea57748 100644 --- a/Zend/tests/assert/expect_011.phpt +++ b/Zend/tests/assert/expect_011.phpt @@ -24,7 +24,7 @@ new Two(); --EXPECTF-- Fatal error: Uncaught AssertionError: [Message]: MyExpectations in %sexpect_011.php:%d Stack trace: -#0 %sexpect_011.php(%d): assert(false, '[Message]: MyEx...') +#0 %sexpect_011.php(%d): assert(false, '[Message]: MyEx...', 'false') #1 %sexpect_011.php(%d): One->__construct() #2 {main} thrown in %sexpect_011.php on line %d diff --git a/Zend/tests/assert/expect_021.phpt b/Zend/tests/assert/expect_021.phpt new file mode 100644 index 0000000000000..5cc467fd13c18 --- /dev/null +++ b/Zend/tests/assert/expect_021.phpt @@ -0,0 +1,40 @@ +--TEST-- +test use of assert_handler callback +--INI-- +zend.assertions=1 +assert.exception=0 +assert.warning=0 +assert.bail=0 +--FILE-- + +--EXPECTF-- +file: %sexpect_021.php +line: %d +assertion: 10 < 5 +description: assert(10 < 5) + +file: %sexpect_021.php +line: %d +assertion: 3 == 4 +description: Assertion Description + +file: %sexpect_021.php +line: %d +assertion: 5 * 5 < 25 +description: CustomError: Assertion Exception in %sexpect_021.php:%d +Stack trace: +#0 {main} \ No newline at end of file diff --git a/Zend/tests/exception_011.phpt b/Zend/tests/exception_011.phpt index a9905cc2f7134..90f0e1f222ce5 100644 --- a/Zend/tests/exception_011.phpt +++ b/Zend/tests/exception_011.phpt @@ -15,6 +15,6 @@ Content-type: text/html; charset=UTF-8 --EXPECTF-- Fatal error: Uncaught AssertionError: assert(false) in %sexception_011.php:%d Stack trace: -#0 %sexception_011.php(%d): assert(false, 'assert(false)') +#0 %sexception_011.php(%d): assert(false, 'assert(false)', 'false') #1 {main} thrown in %sexception_011.php on line %d diff --git a/Zend/zend_compile.c b/Zend/zend_compile.c index 08f596818f6da..a1b27bb50dfbf 100644 --- a/Zend/zend_compile.c +++ b/Zend/zend_compile.c @@ -3710,6 +3710,12 @@ static void zend_compile_assert(znode *result, zend_ast_list *args, zend_string zend_ast_create_zval_from_str( zend_ast_export("assert(", args->child[0], ")"))); } + if (args->children == 2) { + /* may be used as the 3rd argument to an assert callback if one is set */ + zend_ast_list_add((zend_ast*)args, + zend_ast_create_zval_from_str( + zend_ast_export("", args->child[0], ""))); + } zend_compile_call_common(result, (zend_ast*)args, fbc); diff --git a/ext/standard/assert.c b/ext/standard/assert.c index f2b69d9ed25df..002ccd2bc1bb3 100644 --- a/ext/standard/assert.c +++ b/ext/standard/assert.c @@ -142,16 +142,17 @@ PHP_FUNCTION(assert) /* `desc_or_throwable` may either be a description string which will be used in * the message for a failed assertion, or a `Throwable` object which will be * thrown if the assertion fails */ - zval *assertion, *desc_or_throwable = NULL; + zval *assertion, *desc_or_throwable = NULL, *assertion_str = NULL; if (! ASSERTG(active)) { RETURN_TRUE; } - ZEND_PARSE_PARAMETERS_START(1, 2) + ZEND_PARSE_PARAMETERS_START(1, 3) Z_PARAM_ZVAL(assertion) Z_PARAM_OPTIONAL Z_PARAM_ZVAL(desc_or_throwable) + Z_PARAM_ZVAL(assertion_str) /* inserted into call by `zend_compile_assert` */ ZEND_PARSE_PARAMETERS_END(); if (zend_is_true(assertion)) { @@ -170,7 +171,11 @@ PHP_FUNCTION(assert) ZVAL_STRING(&args[0], SAFE_STRING(filename)); ZVAL_LONG(&args[1], lineno); - ZVAL_NULL(&args[2]); + if (assertion_str) { + ZVAL_STR(&args[2], zval_get_string(assertion_str)); + } else { + ZVAL_NULL(&args[2]); + } ZVAL_FALSE(&retval); @@ -182,7 +187,8 @@ PHP_FUNCTION(assert) call_user_function(NULL, NULL, &ASSERTG(callback), &retval, 4, args); zval_ptr_dtor(&(args[3])); } - zval_ptr_dtor(&(args[0])); + zval_ptr_dtor(&args[0]); + zval_ptr_dtor(&args[2]); zval_ptr_dtor(&retval); }