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

Add output buffer length check before attempting to end it #705

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kexxt
Copy link

@kexxt kexxt commented Sep 14, 2021

Hello, this PR is related to #704. Please let me know if any tests need to be adjusted.

Thank you!

@staabm
Copy link
Contributor

staabm commented Sep 14, 2021

This fix looks bogus to me. I think you should investigate why/when this zlib error happens (e.g. in php-src)

@kexxt
Copy link
Author

kexxt commented Sep 14, 2021

Got it, I appreciate your input.

@kexxt kexxt closed this Sep 14, 2021
@denis-sokolov denis-sokolov reopened this Sep 15, 2021
@denis-sokolov
Copy link
Collaborator

I’m not sure it’s obviously bogus. We want to call ob_end_clean as long as there’s buffering present. ob_get_length and ob_get_level both seem like valid options to detect that, and if one’s more practical, we could use it.

It’d be nice to understand the root cause of it better. 🤔

@staabm
Copy link
Contributor

staabm commented Sep 15, 2021

ob_get_level returns the number of nested output buffers (how many times ob_start() was called).
ob_get_length returns the number of bytes of the current active output buffer, no matter how nested the ob_start() calls are.

I think thats a totally different thing

@kexxt
Copy link
Author

kexxt commented Sep 15, 2021

Basically I reach a point where ob_get_length returns 0 but ob_get_level returns 2. When it tries to ob_get_clean, zlib error occurs.

@denis-sokolov
Copy link
Collaborator

@staabm, sure, they do different things, but they both are good candidates to use in that conditional. After all, is it correct to keep flushing until we have no flushing enabled or until we have nothing buffered? Not obvious to me.

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

4 participants