Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Per #1463656-126: Add a Drupal kernel; leverage HttpFoundation and HttpKernel:
Also, not quite sure, but I think RequestCloseSubscriber::onTerminate() calls ob_flush() in all situations, although OB is only started in _drupal_bootstrap_page_header() if !drupal_is_cli().
That logic was ported over from drupal_page_footer(). Since we want to eliminate the ob_flush() anyway that may not be an issue, but if we do keep it, we might want to do it less often? If we keep it and do it for every request, we should document why we are doing so since it seems odd.
Comment | File | Size | Author |
---|---|---|---|
#11 | 1591682-output-buffering-11.patch | 1 KB | Niklas Fiekas |
#4 | 1591682_5_ob_flush.patch | 652 bytes | cosmicdreams |
#1 | 1591682_1_ob_flush.patch | 594 bytes | cosmicdreams |
Comments
Comment #1
cosmicdreams CreditAttribution: cosmicdreams commentedCould it be this simple?
Comment #2
cosmicdreams CreditAttribution: cosmicdreams commentedComment #3
Crell CreditAttribution: Crell commentedIf that's the change we want to make, sure. The question is whether that's the change we want to make. :-) Also, global function a listener == bad (although there's no alternative mechanism yet).
Comment #4
cosmicdreams CreditAttribution: cosmicdreams commentedI suppose the crux of this issue is the question, "Where do we NEED the ob_flush()?" Perhaps a good way of discovering that would be to upload a patch with it removed and see what breaks.
Comment #5
cosmicdreams CreditAttribution: cosmicdreams commentedNow that's surprising. I assumed something was going to break. I'm guessing this is because we don't have the proper test to check for this.
That begs the question: by not having ob_flush here, what kind of problems do we anticipate seeing? Performance issues? Session variables that bleed over to other requests? Security issues?
Comment #6
Crell CreditAttribution: Crell commentedSince that call is after the actual page output, I think it would only flush out any content printed in hook_exit(), such as that generated by devel. And then that would only matter on systems that have output buffering enabled in php.ini. And even then, output is flushed at the end of the request anyway by PHP.
So, I have no idea why we need that line. :-)
Comment #7
cosmicdreams CreditAttribution: cosmicdreams commentedso then....... RTBC?
Comment #8
Crell CreditAttribution: Crell commentedFor me, yes, but let's get some further input from others first.
Comment #9
catchOriginal issue where this was added #477944: Fix and streamline page cache and session handling, I did not read it through yet.
Comment #10
cosmicdreams CreditAttribution: cosmicdreams commentedI couldn't find where in that issue this issue was referenced but i did find this paragraph referencing ob_end_flush()
Which seems to call for the inclusion of some kind of ob_flush activity to be a dded to the drupal_page_footer() function.
Comment #11
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedLet's see.
batch.inc always cleans up, after itself:
theme.inc too:
php.module (<- yuck) too:
But wtf is bootstrap inc doing there? I can't find any matching ob_ operation, except that in the request terminate handler.
I want to see if anything breaks, when we remove that, too.
Comment #13
moshe weitzman CreditAttribution: moshe weitzman commentedStill an issue?
Comment #14
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedLooks like #1651010: Unification of drupal_exit() and drupal_page_footer() and performance boost for FPM enabled environments attacked this and then #1698108: Update Drupal's dependencies could remove all the hacks without breaking anything. I think this is fixed indeed. The worst thing that could happen is we notice something when doing #1597696: Consider whether HttpCache offers any significant benefit over the existing page cache.
Comment #14.0
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedspelling and grammatical errors