WordPress.org

Make WordPress Core

Opened 21 months ago

Last modified 7 months ago

#21402 reopened enhancement

Remove PHP4 methods, format consistently

Reported by: wonderboymusic Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version:
Component: Cache API Keywords: has-patch
Focuses: Cc:

Description

While I was looking around in cache.php, I noticed the TODO to remove the destructor that does nothing and the constructor that registers it to do nothing

Also, the whitespace was different for almost every function - yet the functions all have similar code, this cleans it up

Attachments (2)

format-cache.diff (6.5 KB) - added by wonderboymusic 21 months ago.
21402.diff (781 bytes) - added by wonderboymusic 15 months ago.

Download all attachments as: .zip

Change History (11)

comment:1 SergeyBiryukov21 months ago

  • Component changed from General to Cache

comment:2 in reply to: ↑ description nacin20 months ago

Replying to wonderboymusic:

While I was looking around in cache.php, I noticed the TODO to remove the destructor that does nothing and the constructor that registers it to do nothing

This is a fun one. This constructor/destructor pairing (also exists in wpdb) is designed to prevent the object from being destroyed before shutdown, during which we flush output buffers.

Behold, this will produce a fatal error:

<?php

class foo {
  function bar() {}
}
$GLOBALS['fb'] =& new foo;

function foo_bar($buffer) {
  $GLOBALS['fb']->bar();
  return $buffer;
}

ob_start('foo_bar');

By playing around with register_shutdown_function() we are to get it working again:

<?php

register_shutdown_function( 'ob_end_flush' );

class foo {
	function __construct() {
		register_shutdown_function( array( &$this, '__destruct' ) );
	}

	function bar( $buffer ) {
		return $buffer;
	}

	function __destruct() {
		return true;
	}
}

$GLOBALS['fb'] = new foo;

function foo_bar($buffer) {
	$GLOBALS['fb']->bar( $buffer );
	return $buffer;
}

ob_start('foo_bar');

echo "This works.";

In 5.3.x, I'm also able to get this working:

<?php
register_shutdown_function( 'ob_end_flush' );
class foo {
	function bar( $buffer ) {
		return $buffer;
	}
}
$GLOBALS['fb'] = new foo;
function foo_bar($buffer) {
	$GLOBALS['fb']->bar( $buffer );
	return $buffer;
}
ob_start('foo_bar');
echo "This works.";

So the last one leads me to believe that either there was a change prior to 5.3 that affected orders of destruction, shutdown, and output buffering (would have been in 5.0-5.1)... or that [5462] was sufficient all along, and that [4841] was never gonna work. See #3354.

I'm fine with cleaning this up if we can figure it out.

comment:3 wonderboymusic19 months ago

in 5.3 it's flush > shutdown > destruct

register_shutdown_function( 'my_shutdown' );
register_shutdown_function( 'ob_end_flush' );

function my_shutdown() {
	error_log( 'shutdown' );
}

class foo {
	function bar( $buffer ) {
		return $buffer;
	}
	
	function __destruct() {
		error_log( 'destruct' );
	}
}

$GLOBALS['fb'] = new foo;

function foo_bar($buffer) {
	error_log( 'flush' );
	$GLOBALS['fb']->bar( $buffer );
	return $buffer;
}

ob_start('foo_bar');
echo "This works.";
exit();

Is it the same in PHP 5.2.6?

wonderboymusic15 months ago

comment:4 wonderboymusic15 months ago

  • Milestone changed from Awaiting Review to 3.6

attachment:21402.diff​ is refreshed against trunk - ditches all the whitespace stuff from the previous patch, just ditches the unnecessary PHP4 code

comment:5 ryan11 months ago

  • Milestone changed from 3.6 to Future Release

comment:6 wonderboymusic8 months ago

  • Keywords has-patch removed
  • Milestone Future Release deleted
  • Resolution set to maybelater
  • Status changed from new to closed

Closing my own ticket, this is really the least of our concerns

comment:7 nacin8 months ago

  • Milestone set to 3.7
  • Resolution maybelater deleted
  • Status changed from closed to reopened

Actually, it seems like this would be really nice to clean up. The test scripts work fine on 5.2, 5.3, and 5.4 for me. Let's just rip out the code, it's probably time?

comment:8 jeremyfelt7 months ago

  • Keywords has-patch added

21402.diff still applies cleanly. Should probably go in now or get punted.

comment:9 nacin7 months ago

  • Milestone changed from 3.7 to Future Release

There has been talk about removing wp_ob_end_flush_all(). See #18525. Let's make sure we remove the right pieces here. Fatal errors suck, even on shutdown.

Note: See TracTickets for help on using tickets.