Drop usages of E_RECOVERABLE_ERROR #6106
Conversation
Girgias
force-pushed
the
recoverable-error-to-proper-error
branch
from
717fe60
to
f71484b
Compare
| @@ -2540,7 +2540,7 @@ ZEND_API bool ZEND_FASTCALL zend_object_is_true(zval *op) /* {{{ */ | |||
| if (zobj->handlers->cast_object(zobj, &tmp, _IS_BOOL) == SUCCESS) { | |||
| return Z_TYPE(tmp) == IS_TRUE; | |||
| } | |||
| zend_error(E_RECOVERABLE_ERROR, "Object of class %s could not be converted to bool", ZSTR_VAL(zobj->ce->name)); | |||
| zend_type_error("Object of class %s could not be converted to bool", ZSTR_VAL(zobj->ce->name)); | |||
There was a problem hiding this comment.
This is generally fine, but we will need to perform an exception-safety audit on all users for zend_is_true/zval_is_true.
There was a problem hiding this comment.
Not sure how feasible it is do perform this before next Tuesday when RC1 gets tagged as there does seems to be a couple of crucial usages (zend_compare, OpCache) looking at: https://heap.space/search?project=php-src&refs=zval_is_true and https://heap.space/search?project=php-src&refs=zend_is_true
There was a problem hiding this comment.
I think I've checked all of them but I'm very unsure about how to go about it with some cases.
Girgias
force-pushed
the
recoverable-error-to-proper-error
branch
2 times, most recently
from
762a98b
to
f9e0482
Compare
Girgias
force-pushed
the
recoverable-error-to-proper-error
branch
3 times, most recently
from
0716269
to
16a0b30
Compare
|
I think we need to punt this one to 8.1. |
| @@ -604,8 +604,18 @@ ZEND_API zend_result ZEND_FASTCALL zend_ast_evaluate(zval *result, zend_ast *ast | |||
| break; | |||
| } | |||
| ZVAL_BOOL(result, zend_is_true(&op2)); | |||
| /* op2 is an object which cannot be converted to bool */ | |||
There was a problem hiding this comment.
Objects definitely can't occur in this context.
|
If RC1 is delayed by 2 weeks, it is feasible for this to make it into 8.0? |
|
@Girgias what's the status on this one ? milestone coming up ... |
I'll have another look at this within this week, thanks for reminding me. |
|
8.1.0alpha1 is out this week. How's this looking for inclusion in a future 8.1 milestone release? |
Girgias
force-pushed
the
recoverable-error-to-proper-error
branch
from
8ce4e2a
to
ff0cf44
Compare
This drops the last usage of E_RECOVERABLE_ERROR in php-src
If stream context value can be an Object will need to look at this again
Girgias
force-pushed
the
recoverable-error-to-proper-error
branch
from
ff0cf44
to
e657b50
Compare
| $db->exec('CREATE TABLE test(id int NOT NULL PRIMARY KEY, val boolean)'); | ||
| $db->exec('INSERT INTO test VALUES(1, TRUE)'); | ||
| $db->exec('INSERT INTO test VALUES(2, FALSE)'); |
There was a problem hiding this comment.
I'm not sure about this test case. I think this test case should be present in every driver separately as some of them have the boolean type and some don't. Also, binding using PDO::PARAM_BOOL might not be properly supported for some of them. I'd also expect this test case to happen with emulated and without emulated prepares.
The exception checking you have added looks like the wrong solution. See MySQL execution for example, which silently fails and causes a syntax error. We don't want to do that. It's just my opinion, what's yours?
There was a problem hiding this comment.
Some of the tests in ext/pdo are skipped for certain drivers (namely OCI) and I prefer doing this because if a new driver gets added/modified we'll know this case will be handled.
The exception check might be incorrect because I blindly added it everywhere where a call which could throw comes up, so any improvements are welcomed. :-)
There was a problem hiding this comment.
Ok, then how about instead of using boolean data type, use int instead. I think it's more widely supported. Then also you can change TRUE/FALSE to 1/0.
| @@ -259,6 +259,10 @@ safe: | |||
| switch (param_type) { | |||
| case PDO_PARAM_BOOL: | |||
| plc->quoted = zend_is_true(parameter) ? ZSTR_CHAR('1') : ZSTR_CHAR('0'); | |||
| // TODO Can an exception occur here? | |||
| if (EG(exception)) { | |||
There was a problem hiding this comment.
How about adding ret = -1; here to at least stop the execution?
There was a problem hiding this comment.
That's probably even necessary, been a while since I looked at this code.
|
There has not been any recent activity in this PR. It will automatically be closed in 7 days if no further action is taken. |
There were only 2 remaining usages.
As from comment #6049 (comment)