Implement GH-7922: print_r should not produce a fatal error #7964
Conversation
Fataling if `::__debugInfo()` doesn't return an array appears to be overly restrictive; we can handle that like returning null, namely by throwing an `Error` exception and returning an empty array instead.
|
Need to check the failing tests. |
There was a problem hiding this comment.
zend_print_zval_r_to_buf didn't check for EG(exception) before, so I'm assuming it's probably fine not to start checking for EG(exception) (e.g. __debugInfo manually throwing, e.g. in a destructor in a local scope after return [];).
But it might be intuitive to stop IS_ARRAY/IS_OBJECT loops anyway if there is an exception?
It may be useful to test partial output behavior for throwing in nested values?
| } | ||
|
|
||
| zend_error_noreturn(E_ERROR, ZEND_DEBUGINFO_FUNC_NAME "() must return an array"); | ||
|
|
||
| return NULL; /* Compilers are dumb and don't understand that noreturn means that the function does NOT need a return value... */ |
There was a problem hiding this comment.
Possibly no longer needed if switch is comprehensive, haven't tested.
| *is_temp = 1; | ||
| ht = Z_ARR(retval); | ||
| ht = zend_new_array(0); |
There was a problem hiding this comment.
Not familiar enough with this code to be sure: Does this code predate zend_empty_array (which would be non-reference-counted and have to have *is_temp = 0)
#define ZVAL_EMPTY_ARRAY(z) do { \
zval *__z = (z); \
Z_ARR_P(__z) = (zend_array*)&zend_empty_array; \
Z_TYPE_INFO_P(__z) = IS_ARRAY; \
} while (0)
| return Z_ARRVAL(retval); | ||
| } | ||
| default: | ||
| zend_throw_error(NULL, ZEND_DEBUGINFO_FUNC_NAME "() must return an array"); |
There was a problem hiding this comment.
Does this need to be thrown if EG(exception) is already non-null after the call? It seems more intuitive (for userland expectations) to have the original exception than to replace it.
|
There has not been any recent activity in this PR. It will automatically be closed in 7 days if no further action is taken. |
|
@cmb69 can you please reopen this PR or is there any big issue with the impl? |
|
There have been failing tests, and @TysonAndre's comments need to be addressed. I currently don't have time for this. Feel free to take over the patch, and submit a new PR. |
|
Sadly, my knowledge of php internals is limited :( |
|
@cmb69 when I will address the comments, can it target PHP 8.0 as a bugfix? |
New features should always target master. |
Fataling if
::__debugInfo()doesn't return an array appears to beoverly restrictive; we can handle that like returning null, namely by
throwing an
Errorexception and returning an empty array instead.PS: if null is returned, we don't throw an exception like before. Exception is only thrown if not array|null.