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

In ob_get_status() fix type key #4868

Closed
wants to merge 1 commit into from

Conversation

tstarling
Copy link
Contributor

As documented and implemented in PHP PHP, type=0 is internal, and type=1 is for user handlers. This function got it the wrong way around.

This is an untested fix for https://phabricator.wikimedia.org/T89918 . MediaWiki relies on seeing type=1 to know which output buffers it can safely cancel in wfResetOutputBuffers(). When HHVM gives type=0 instead, MediaWiki fails to disable output buffering, and thus hits an OOM when it tries to stream out large output, e.g. a 200MB PDF file in the current test case.

@facebook-github-bot
Copy link
Contributor

This pull request has been imported into Phabricator, and discussion and review of the diff will take place at https://reviews.facebook.net/D33735

As documented and implemented in PHP PHP, type=0 is internal, and type=1
is for user handlers. This function got it the wrong way around.

Also reorder the array keys and add keys for chunk_size and buffer_used
per their undocumented PHP behaviour. When ob_start() is called without
arguments, give type=0 and name="default output handler", matching the
PHP behaviour. This gives a minimal diff when
zend/bad/tests/output/ob_get_status.php and zend/bad/tests/output/ob_013.php
are run. It now only complains about the omission of "flags" and
"buffer_size", both of which are implementation details that I don't
think should have been included in the PHP output.

Make ob_get_status_empty.php into a more specific test for facebook#2690, so
that it does not restrict details of the return value of
ob_get_status() which are unrelated to that bug.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants