Make WordPress Core

Opened 15 years ago

Closed 15 years ago

#11588 closed enhancement (duplicate)

show_message() should flush the output buffers when its called.

Reported by: dd32's profile dd32 Owned by:
Milestone: Priority: normal
Severity: normal Version: 2.9
Component: Administration Keywords:
Focuses: Cc:

Description

show_message() should flush the output buffers when its called, If a function is calling show_message(), its safe to assume that the function is wanting the output displayed now.

show_message() is mostly used within the various Upgraders.

See patch.

Attachments (1)

11588.diff (659 bytes) - added by dd32 15 years ago.

Download all attachments as: .zip

Change History (20)

@dd32
15 years ago

#1 @dd32
15 years ago

i'm not sure if mearly flush() is the best function to call.. checking previous tickets on this..

#2 @dd32
15 years ago

another option is wp_ob_end_flush_all().

Which has the benefit of disabling any php-level ob_*()'s, the main(and only) benefit is that if a gzip ob_* filter is used, then the client will get the message ASAP.. as the gzip gets disabled.

That is also its downside however.. removes the ability to ob_start(); ob_get_contents(); the entire upgrade process...

#3 @Denis-de-Bernardy
15 years ago

FWIW, the suggested patch doesn't flush hard enough.

I use this function, personally:

	function force_flush() {
		echo "\n\n<!-- Deal with browser-related buffering by sending some incompressible strings -->\n\n";
		
		for ( $i = 0; $i < 5; $i++ )
			echo "<!-- abcdefghijklmnopqrstuvwxyz1234567890aabbccddeeffgghhiijjkkllmmnnooppqqrrssttuuvvwwxxyyzz11223344556677889900abacbcbdcdcededfefegfgfhghgihihjijikjkjlklkmlmlnmnmononpopoqpqprqrqsrsrtstsubcbcdcdedefefgfabcadefbghicjkldmnoepqrfstugvwxhyz1i234j567k890laabmbccnddeoeffpgghqhiirjjksklltmmnunoovppqwqrrxsstytuuzvvw0wxx1yyz2z113223434455666777889890091abc2def3ghi4jkl5mno6pqr7stu8vwx9yz11aab2bcc3dd4ee5ff6gg7hh8ii9j0jk1kl2lmm3nnoo4p5pq6qrr7ss8tt9uuvv0wwx1x2yyzz13aba4cbcb5dcdc6dedfef8egf9gfh0ghg1ihi2hji3jik4jkj5lkl6kml7mln8mnm9ono -->\n\n";
		
		while ( ob_get_level() )
			ob_end_flush();
		
		@ob_flush();
		@flush();
		@set_time_limit(FS_TIMEOUT);
	} # force_flush()

and before it, at the very beginning of the various upgrader scripts:

	if ( function_exists('apache_setenv') )
		@apache_setenv('no-gzip', 1);
	@ini_set('zlib.output_compression', 0);
	@ini_set('implicit_flush', 1);

anything short of that will reveal that the flushing does not occur on some servers and some user agents.

#4 follow-up: @dd32
15 years ago

I agree that its not enough to completely flush it on every system.

I would however, Like to suggest that we agree on a very basic start which works on the majority of servers without switching to changing the environment or working around various browser bugs.

Once the basic issue has been dealt with, Then we can consider the extra's to make it work accross other specific configurations.

Personally, I'd like to suggest this from your quote:

@ob_flush();
@flush();

#5 in reply to: ↑ 4 @Denis-de-Bernardy
15 years ago

Replying to dd32:

I agree that its not enough to completely flush it on every system.

I would however, Like to suggest that we agree on a very basic start which works on the majority of servers without switching to changing the environment or working around various browser bugs.

Once the basic issue has been dealt with, Then we can consider the extra's to make it work accross other specific configurations.

Up to you. I hook into show_message() using the above-mentioned function anyway. :-P

#6 follow-up: @dd32
15 years ago

Up to you. I hook into show_message() using the above-mentioned function anyway. :-P

I realise that, But this is a defect for a bug in an open source application that thousands use, Your input is valued, But please do think of regular users rather than just those of your distribution if you're going to offer knowledge.

If you have a better understanding of the situation than I, as you obviously do, Then i'd prefer you to offer a solution. But i'd like to see a minimalist solution which fixes it under the most basic servers first, and moves on from there.

#7 in reply to: ↑ 6 @Denis-de-Bernardy
15 years ago

Replying to dd32:

Up to you. I hook into show_message() using the above-mentioned function anyway. :-P

I realise that, But this is a defect for a bug in an open source application that thousands use, Your input is valued, But please do think of regular users rather than just those of your distribution if you're going to offer knowledge.

I thought no less, and that was precisely my point in suggesting the above code. I originally tried to flush things more or less the way you are suggesting. It only works provided that:

  • The server does not enable output buffering
  • The server does not gzip the output (meaning it technically ignores flush() statements)
  • The user agent doesn't wait for a certain amount of data before rendering (which is to say most UA nowadays)

The code I'm suggesting has been evolving since towards early 2007. It has been tested by hundreds of users.

Again, it's just for information. You're welcome to use any or all of it; or to ignore it, and delay your arriving to similar code in a few months or years.

#8 @Denis-de-Bernardy
15 years ago

Just for the note, too: the line in there that increases the timeout has more to do with preventing the upgrade from failing due to server timeouts that it has to do with flushing. The full code that I use actually reconnects the FTP whenever a message is displayed, in order to work around the fact that FTP times out on a per session basis. I'll post that code too if there is any interest.

#9 @dd32
15 years ago

  • Keywords dev-feedback added

You're welcome to use any or all of it; or to ignore it, and delay your arriving to similar code in a few months or years.

I see it as overkill to go around the mulberry bush so many times, We did this exact same thing last time on this very issue 14 months ago. End result was no commiter has been willing to go near the ticket.

I do not believe that it needs to be flushed under every single different environment, If a user has enabled output compression or a server has server-level ziping in place, There is not reliable way to prevent that, Those in your code will prevent it some of the time. thats it.

Can a core-dev step in here and take what they think from this lot and commit something? Anything? If it gets fixed for 80% (Or heck, even 50%) its a good start to work off.

#10 @hakre
15 years ago

I suggest to close this ticket as invalid. Output buffering is no-where used inside the core code and we should not introduce it in a function only because that function is doing output. I mean we have tons of output related functions. do we flush there?

What are the criterions? Output buffering should be completely ignored to have at least a save spot for hackers.

#11 @hakre
15 years ago

  • Type changed from defect (bug) to feature request

It's more of a feature request than a bug.

#12 @nacin
15 years ago

show_message() is specifically for the upgraders. Upgrading can take a while, and we deliberately call show_message() to write information to the browser updating the user on the status of the upgrades, as the package is downloaded, unpacked, etc.

From what I have gathered from dd32, the original intention of show_message() was to send output to the browser. Not including a flush() here was an accidental omission when the upgraders were written.

Hence why we should flush here even though we don't anywhere else in core.

#13 follow-up: @matt
15 years ago

This is a solution, but to the wrong problem. The correct approach would be to make the upgrade progress called async through JS with (hopefully) frequent status updates.

#14 in reply to: ↑ 13 @Denis-de-Bernardy
15 years ago

Replying to matt:

This is a solution, but to the wrong problem. The correct approach would be to make the upgrade progress called async through JS with (hopefully) frequent status updates.

actually, async isn't the best of idea for mass plugin upgrades. you can end up in a situation where concurrent loads update the active_plugins option and mess it up. those need to be handled sequentially.

what could be reasonable to do is to handle the upgrade process in its own iframe. but that ought to be a separate ticket.

#15 @matt
15 years ago

It can still be sequential, that's orthogonal to the issue. If you want to start another ticket for a real fix then we can close this one WONTFIX.

#16 @Denis-de-Bernardy
15 years ago

  • Keywords bug-hunt added

#17 @Denis-de-Bernardy
15 years ago

  • Keywords featured added; bug-hunt removed

#18 @nacin
15 years ago

  • Type changed from feature request to enhancement

Related #11232.

#19 @nacin
15 years ago

  • Keywords has-patch dev-feedback featured removed
  • Milestone 3.0 deleted
  • Resolution set to duplicate
  • Status changed from new to closed

I'm just going to close this as a duplicate of #11232.

Note: See TracTickets for help on using tickets.