-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Deprecate returning non-string values from a user output handler #18932
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
f4c3fde
340c7fd
720923e
cfe4d71
51892ba
6a2d6c1
ab5acde
25c35b8
8144c55
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -934,6 +934,7 @@ static inline php_output_handler_status_t php_output_handler_op(php_output_handl | |
return PHP_OUTPUT_HANDLER_FAILURE; | ||
} | ||
|
||
bool still_have_handler = true; | ||
/* storable? */ | ||
if (php_output_handler_append(handler, &context->in) && !context->op) { | ||
context->op = original_op; | ||
|
@@ -948,6 +949,7 @@ static inline php_output_handler_status_t php_output_handler_op(php_output_handl | |
if (handler->flags & PHP_OUTPUT_HANDLER_USER) { | ||
zval ob_args[2]; | ||
zval retval; | ||
ZVAL_UNDEF(&retval); | ||
|
||
/* ob_data */ | ||
ZVAL_STRINGL(&ob_args[0], handler->buffer.data, handler->buffer.used); | ||
|
@@ -959,17 +961,48 @@ static inline php_output_handler_status_t php_output_handler_op(php_output_handl | |
handler->func.user->fci.params = ob_args; | ||
handler->func.user->fci.retval = &retval; | ||
|
||
#define PHP_OUTPUT_USER_SUCCESS(retval) ((Z_TYPE(retval) != IS_UNDEF) && !(Z_TYPE(retval) == IS_FALSE)) | ||
if (SUCCESS == zend_call_function(&handler->func.user->fci, &handler->func.user->fcc) && PHP_OUTPUT_USER_SUCCESS(retval)) { | ||
/* user handler may have returned TRUE */ | ||
status = PHP_OUTPUT_HANDLER_NO_DATA; | ||
if (Z_TYPE(retval) != IS_FALSE && Z_TYPE(retval) != IS_TRUE) { | ||
convert_to_string(&retval); | ||
if (Z_STRLEN(retval)) { | ||
context->out.data = estrndup(Z_STRVAL(retval), Z_STRLEN(retval)); | ||
context->out.used = Z_STRLEN(retval); | ||
context->out.free = 1; | ||
status = PHP_OUTPUT_HANDLER_SUCCESS; | ||
if (SUCCESS == zend_call_function(&handler->func.user->fci, &handler->func.user->fcc) && Z_TYPE(retval) != IS_UNDEF) { | ||
if (Z_TYPE(retval) != IS_STRING) { | ||
// Make sure that we don't get lost in the current output buffer | ||
// by disabling it | ||
handler->flags |= PHP_OUTPUT_HANDLER_DISABLED; | ||
php_error_docref( | ||
NULL, | ||
E_DEPRECATED, | ||
"Returning a non-string result from user output handler %s is deprecated", | ||
ZSTR_VAL(handler->name) | ||
); | ||
// Check if the handler is still in the list of handlers to | ||
// determine if the PHP_OUTPUT_HANDLER_DISABLED flag can | ||
// be removed | ||
still_have_handler = false; | ||
int handler_count = php_output_get_level(); | ||
if (handler_count) { | ||
php_output_handler **handlers = (php_output_handler **) zend_stack_base(&OG(handlers)); | ||
for (int iii = 0; iii < handler_count; ++iii) { | ||
php_output_handler *curr_handler = handlers[iii]; | ||
if (curr_handler == handler) { | ||
handler->flags &= (~PHP_OUTPUT_HANDLER_DISABLED); | ||
still_have_handler = true; | ||
break; | ||
} | ||
} | ||
} | ||
} | ||
if (Z_TYPE(retval) == IS_FALSE) { | ||
/* call failed, pass internal buffer along */ | ||
status = PHP_OUTPUT_HANDLER_FAILURE; | ||
} else { | ||
/* user handler may have returned TRUE */ | ||
status = PHP_OUTPUT_HANDLER_NO_DATA; | ||
if (Z_TYPE(retval) != IS_FALSE && Z_TYPE(retval) != IS_TRUE) { | ||
convert_to_string(&retval); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This may fail with non-stringable objects, also I would prefer There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This part is unchanged from the original, I'd rather not change things There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK, but can you add a test with a non-stringable object being returned from an output handler? |
||
if (Z_STRLEN(retval)) { | ||
context->out.data = estrndup(Z_STRVAL(retval), Z_STRLEN(retval)); | ||
context->out.used = Z_STRLEN(retval); | ||
context->out.free = 1; | ||
status = PHP_OUTPUT_HANDLER_SUCCESS; | ||
} | ||
} | ||
} | ||
} else { | ||
|
@@ -996,10 +1029,17 @@ static inline php_output_handler_status_t php_output_handler_op(php_output_handl | |
status = PHP_OUTPUT_HANDLER_FAILURE; | ||
} | ||
} | ||
handler->flags |= PHP_OUTPUT_HANDLER_STARTED; | ||
if (still_have_handler) { | ||
handler->flags |= PHP_OUTPUT_HANDLER_STARTED; | ||
} | ||
OG(running) = NULL; | ||
} | ||
|
||
if (!still_have_handler) { | ||
// Handler and context will have both already been freed | ||
return status; | ||
} | ||
|
||
switch (status) { | ||
case PHP_OUTPUT_HANDLER_FAILURE: | ||
/* disable this handler */ | ||
|
@@ -1225,6 +1265,19 @@ static int php_output_stack_pop(int flags) | |
} | ||
php_output_handler_op(orphan, &context); | ||
} | ||
// If it isn't still in the stack, cannot free it | ||
bool still_have_handler = false; | ||
int handler_count = php_output_get_level(); | ||
if (handler_count) { | ||
php_output_handler **handlers = (php_output_handler **) zend_stack_base(&OG(handlers)); | ||
for (int iii = 0; iii < handler_count; ++iii) { | ||
php_output_handler *curr_handler = handlers[iii]; | ||
if (curr_handler == orphan) { | ||
still_have_handler = true; | ||
break; | ||
} | ||
} | ||
} | ||
|
||
/* pop it off the stack */ | ||
zend_stack_del_top(&OG(handlers)); | ||
|
@@ -1240,7 +1293,9 @@ static int php_output_stack_pop(int flags) | |
} | ||
|
||
/* destroy the handler (after write!) */ | ||
php_output_handler_free(&orphan); | ||
if (still_have_handler) { | ||
php_output_handler_free(&orphan); | ||
} | ||
php_output_context_dtor(&context); | ||
|
||
return 1; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,96 @@ | ||
--TEST-- | ||
ob_start(): Check behaviour with deprecation converted to exception | ||
--FILE-- | ||
<?php | ||
|
||
$log = []; | ||
|
||
set_error_handler(function (int $errno, string $errstr, string $errfile, int $errline) { | ||
throw new \ErrorException($errstr, 0, $errno, $errfile, $errline); | ||
}); | ||
|
||
function return_null($string) { | ||
global $log; | ||
$log[] = __FUNCTION__ . ": <<<" . $string . ">>>"; | ||
return null; | ||
} | ||
|
||
function return_false($string) { | ||
global $log; | ||
$log[] = __FUNCTION__ . ": <<<" . $string . ">>>"; | ||
return false; | ||
} | ||
|
||
function return_true($string) { | ||
global $log; | ||
$log[] = __FUNCTION__ . ": <<<" . $string . ">>>"; | ||
return true; | ||
} | ||
|
||
function return_zero($string) { | ||
global $log; | ||
$log[] = __FUNCTION__ . ": <<<" . $string . ">>>"; | ||
return 0; | ||
} | ||
|
||
$cases = ['return_null', 'return_false', 'return_true', 'return_zero']; | ||
foreach ($cases as $case) { | ||
$log = []; | ||
echo "\n\nTesting: $case\n"; | ||
ob_start($case); | ||
echo "Inside of $case\n"; | ||
try { | ||
ob_end_flush(); | ||
} catch (\ErrorException $e) { | ||
echo $e . "\n"; | ||
} | ||
echo "\nEnd of $case, log was:\n"; | ||
echo implode("\n", $log); | ||
} | ||
|
||
?> | ||
--EXPECTF-- | ||
Testing: return_null | ||
ErrorException: ob_end_flush(): Returning a non-string result from user output handler return_null is deprecated in %s:%d | ||
Stack trace: | ||
#0 [internal function]: {closure:%s:%d}(8192, 'ob_end_flush():...', %s, %d) | ||
#1 %s(%d): ob_end_flush() | ||
#2 {main} | ||
|
||
End of return_null, log was: | ||
return_null: <<<Inside of return_null | ||
>>> | ||
|
||
Testing: return_false | ||
Inside of return_false | ||
ErrorException: ob_end_flush(): Returning a non-string result from user output handler return_false is deprecated in %s:%d | ||
Stack trace: | ||
#0 [internal function]: {closure:%s:%d}(8192, 'ob_end_flush():...', %s, %d) | ||
#1 %s(%d): ob_end_flush() | ||
#2 {main} | ||
|
||
End of return_false, log was: | ||
return_false: <<<Inside of return_false | ||
>>> | ||
|
||
Testing: return_true | ||
ErrorException: ob_end_flush(): Returning a non-string result from user output handler return_true is deprecated in %s:%d | ||
Stack trace: | ||
#0 [internal function]: {closure:%s:%d}(8192, 'ob_end_flush():...', %s, %d) | ||
#1 %s(%d): ob_end_flush() | ||
#2 {main} | ||
|
||
End of return_true, log was: | ||
return_true: <<<Inside of return_true | ||
>>> | ||
|
||
Testing: return_zero | ||
0ErrorException: ob_end_flush(): Returning a non-string result from user output handler return_zero is deprecated in %s:%d | ||
Stack trace: | ||
#0 [internal function]: {closure:%s:%d}(8192, 'ob_end_flush():...', %s, %d) | ||
#1 %s(%d): ob_end_flush() | ||
#2 {main} | ||
|
||
End of return_zero, log was: | ||
return_zero: <<<Inside of return_zero | ||
>>> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we have a better variable name than
iii
?