Skip to content
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

Output Buffering: Add behavioural tests and some refactoring to ob_start() #14646

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

Girgias
Copy link
Member

@Girgias Girgias commented Jun 23, 2024

No description provided.

@Girgias Girgias marked this pull request as ready for review June 24, 2024 12:39
@SakiTakamachi
Copy link
Member

SakiTakamachi commented Jul 11, 2024

Since optional arguments are parsed multiple times, why not receive $callback as a zval and then parse it?

edit:
Might be a bit annoying as the error order changes...

@Girgias
Copy link
Member Author

Girgias commented Jul 11, 2024

Doing multiple argument parsing is not new and generally how we deal with overloaded signatures, and ideally the normal callback function should work (which is not the case for zlib) and actually exist (which is not the case for the iconv one)

@@ -0,0 +1,24 @@
--TEST--
ob_start(): ensure buffers can't be added from within callback.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably a copy-paste leftover as this test is about ob_start() throwing on a negative buffer size, right?

@@ -0,0 +1,21 @@
--TEST--
Returning resource in output handler suppresses warning
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think resources are converted to strings without raising a warning. Source ref (_convert_to_string()):

case IS_RESOURCE: {
zend_string *str = zend_strpprintf(0, "Resource id #" ZEND_LONG_FMT, (zend_long)Z_RES_HANDLE_P(op));
zval_ptr_dtor(op);
ZVAL_NEW_STR(op, str);
break;

@@ -0,0 +1,22 @@
--TEST--
Errors in output handler are suppressed
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably read something like Errors in output handler are suppressed if handler does not return false as the following handler will return the undefined variable warning as well:

function handler(string $buffer, int $phase) {
    echo "In handler";
    // Undefined variable
    $undef++;
    
    return false;
}

@haszi
Copy link
Contributor

haszi commented Jul 13, 2024

I'm not sure whether it makes sense to add tests for these but during shutdown a handler returning an array or a fatal error being raised in a handler produce a slightly different output than what the existing tests show.

@haszi
Copy link
Contributor

haszi commented Jul 17, 2024

I think it would also make sense to add a basic test for what output is buffered and what is not. Something like https://3v4l.org/XoAGC

@Girgias
Copy link
Member Author

Girgias commented Jul 17, 2024

I've seen the comments, just haven't had time.

I will add some more tests, namely to test shutdown

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants