[go: up one dir, main page]

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

Calling ob_clean() after ob_start('gz_handler') does not set Content-Encoding #7953

Closed
timint opened this issue Jan 17, 2022 · 9 comments
Closed

Comments

@timint
Copy link
timint commented Jan 17, 2022

Description

The following code:

<?php

  ob_start('ob_gzhandler');
  ob_clean();
  
  echo 'Hello World';

Resulted in this output:

��� �H���W��/�I�V��J�

But I expected this output instead:

Hello World

There is no mention anywhere in the documentation that clearing the output buffer resets the Content-Encoding HTTP Header. If this is by design it should be stated, but it feels like an error.

PHP Version

PHP 8.0.10

Operating System

Windows 11 x64

@damianwadley
Copy link
Member

ob_gzhandler isn't some special string that output buffering recognizes - it's an actual function - so it's not output buffering itself that is at fault. It's like you wrote your own callback that added a response header the first time it was called - that header will still be there after output buffering ends.

I think it would be possible for ob_gzhandler to detect the PHP_OUTPUT_HANDLER_CLEAN flag and to unset the header and reset back to the initial state. (Equally possible in userland.) Or to remove the header automatically if there's been no output, but that would mean no more effectively-empty responses with just the compression container and instead actually-empty responses with no content at all, but any good client should be totally okay with that.

@timint
Copy link
Author
timint commented Jan 17, 2022

Yes, I'm a PHP developer since 15 years back. I know it's a callback function. What I don't get is why ob_clean() unsets the header when stackoverflow and PHP documentations is full of comments that ob_clean() does not affect headers.

Do you not see a problem with this?

@damianwadley
Copy link
Member

Hmm, looks like I remembered ob_gzhandler backwards: it is a special string after all. It's an alias that goes through slightly different handling in that it will initialize a context, which ob_gzhandler the function does too but in a slightly different way. Thus why ob_start("ob_gzhandler") and ob_start(fn($buffer, $phase) => ob_gzhandler($buffer, $phase)); aren't the same.

Looking into it more, it's not that the headers are removed but that they aren't added at all. I'm also seeing odd behavior with a flush + clean in that the special "ob_gzhandler" issues a "Failed to delete buffer" notice while the wrapped ob_gzhandler() call does not.

So it seems like there's some iffy behavior with how it handles the CLEAN phases in general.

@damianwadley damianwadley changed the title Calling ob_clean() after ob_start('gz_handler') unsets Content-Encoding Calling ob_clean() after ob_start('gz_handler') does not set Content-Encoding Jan 17, 2022
cmb69 added a commit to cmb69/php-src that referenced this issue Jan 17, 2022
If an output handler has not yet been started, calling `ob_clean()`
causes it to start.  If that happens, we must not forget to set the
`Content-Encoding` and `Vary` headers.
@cmb69 cmb69 linked a pull request Jan 17, 2022 that will close this issue
@cmb69 cmb69 self-assigned this Jan 17, 2022
@timint
Copy link
Author
timint commented Jan 17, 2022

Well done 👍

@cmb69
Copy link
Member
cmb69 commented Jan 17, 2022

I'm also seeing odd behavior with a flush + clean in that the special "ob_gzhandler" issues a "Failed to delete buffer" notice while the wrapped ob_gzhandler() call does not.

The special ob_gzhandler makes the handler immutable (i.e. uncleanable and unremoveable) after the headers have been sent. The wrapped ob_gzhandler doesn't do that. I'm not sure whether uncleanability is required, but at least the handler must be unremoveable, or otherwise output after removing the handler causes issues:

ob_start(fn($buffer, $phase) => ob_gzhandler($buffer, $phase));
echo "Hello";
ob_flush();
ob_end_clean();
echo "world";

Note to self: check ob_iconv_handler regarding these issues.

@cmb69
Copy link
Member
cmb69 commented Jan 20, 2022

check ob_iconv_handler regarding these issues.

Indeed, it has basically the same issue, namely that the Content-Type header is not properly set, if ob_clean() is called right after ob_start().

The "wrapper" issue isn't a thing, though, since ob_iconv_handler() isn't a real function, but only an ob handler alias.

@timint
Copy link
Author
timint commented Jan 20, 2022

I just want to point out ob_clean() doesn't need to be right after. Any use of ob_clean() long after ob_start('gzhandler') should cause the same problem.

<?php

  ob_start('ob_gzhandler');

  ...

  echo 'Say something regretful';
  ob_clean();
  
  echo 'Hello World';

Result:

 ��� �H���W��/�I�V��J�

@cmb69
Copy link
Member
cmb69 commented Jan 20, 2022

I just want to point out ob_clean() doesn't need to be right after. Any use of ob_clean() long after ob_start('gzhandler') should cause the same problem.

Ah, right! More correctly, it is about calling ob_clean() before output has been flushed.

@timint
Copy link
Author
timint commented Jan 20, 2022

Correct, that is how I stumbled upon this.

@cmb69 cmb69 closed this as completed in 9bd468d Feb 3, 2022
cmb69 added a commit that referenced this issue Feb 3, 2022
* PHP-8.0:
  Fix GH-7953: ob_clean() only does not set Content-Encoding
cmb69 added a commit that referenced this issue Feb 3, 2022
* PHP-8.1:
  Fix GH-7953: ob_clean() only does not set Content-Encoding
cmb69 added a commit to cmb69/php-src that referenced this issue Apr 12, 2022
The fix for phpGH-7953 introduced a regression by being to deliberate
adding the respective headers.  These must only be added, if the
handler starts, but is not finalizing.
cmb69 added a commit that referenced this issue Apr 25, 2022
The fix for GH-7953 introduced a regression by being to deliberate
adding the respective headers.  These must only be added, if the
handler starts, but is not finalizing.

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

Successfully merging a pull request may close this issue.

4 participants
@timint @cmb69 @damianwadley and others