Remove PHP4 methods, format consistently

[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

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

[nacin]

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.

[wonderboymusic]

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?

[wonderboymusic]

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

[wonderboymusic]

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

[nacin]

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?

[jeremyfelt]

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

[nacin]

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.

[Mte90]

refreshed

[Mte90]

Removed now the minimum support version of wordpress is from the 5.x branch not php4.

[welcher]

Looks like the consensus here is to get this moving forward. @markjaquith @wonderboymusic any reason we shouldn't? In the meantime, I think this still needs testing and perhaps some unit tests.

[SergeyBiryukov]

Switching milestone due to the focus on the new editor (Gutenberg) for WordPress 5.0.

[pento]

Punting to future release, this needs pretty thorough testing, to ensure we don't break weird setups.

[SergeyBiryukov]

#41615 was marked as a duplicate.

[SergeyBiryukov]

Replying to nacin:

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.

In my testing, both seem to be the case.

• Since PHP 5.1, shutdown is called before the output buffer is implicitly flushed by the termination of the script:
  ​https://3v4l.org/u4VR7

  <?php
  register_shutdown_function( 'shutdown_func' );
  
  function ob_callback( $buffer ) {
      $buffer .= __FUNCTION__ . ' ';
      return $buffer;
  }
  
  function shutdown_func() {
      echo __FUNCTION__ . ' ';
  }
  
  ob_start( 'ob_callback' );
  ?>
  
  Output for 5.1.0 - 7.4.1
  shutdown_func ob_callback 
  Output for 4.3.0 - 5.0.5
  ob_callback shutdown_func 
  

• Since PHP 5.2, objects are destroyed before the output buffer is flushed by the termination of the script:
  ​https://3v4l.org/eO8t0

  <?php
  class foo {
  	function bar() {
  	}
  
  }
  
  $GLOBALS['fb'] = new foo;
  
  function foo_bar( $buffer ) {
  	$buffer .= "gettype( 'fb' ): " . gettype( $GLOBALS['fb'] );
  	return $buffer;
  }
  
  ob_start( 'foo_bar' );
  ?>
  
  Output for 5.2.0 - 7.4.1
  gettype( 'fb' ): NULL
  Output for 4.3.0 - 5.1.6
  gettype( 'fb' ): object
  

• Our fiddling with the destructor does not exactly help. Interestingly, it only works on PHP 7.0.6+:
  ​https://3v4l.org/L4O6I

  <?php
  class foo {
  	function __construct() {
  		register_shutdown_function( array( $this, '__destruct' ) );
  	}
  
  	function bar() {
  	}
  
  	function __destruct() {
  		return true;
  	}
  }
  
  $GLOBALS['fb'] = new foo;
  
  function foo_bar( $buffer ) {
  	$buffer .= "gettype( 'fb' ): " . gettype( $GLOBALS['fb'] );
  	return $buffer;
  }
  
  ob_start( 'foo_bar' );
  ?>
  
  Output for 4.3.0 - 5.1.6, 7.0.6 - 7.4.1
  gettype( 'fb' ): object
  Output for 5.2.0 - 7.0.5
  gettype( 'fb' ): NULL
  

• On the other hand, explicitly flushing the buffer on shutdown works on PHP 5.1+ and all later versions:
  ​https://3v4l.org/m8rAm

  <?php
  register_shutdown_function( 'ob_end_flush' );
  
  class foo {
  	function bar() {
  	}
  
  }
  
  $GLOBALS['fb'] = new foo;
  
  function foo_bar( $buffer ) {
  	$buffer .= "gettype( 'fb' ): " . gettype( $GLOBALS['fb'] );
  	return $buffer;
  }
  
  ob_start( 'foo_bar' );
  ?>
  
  Output for 5.1.0 - 7.4.1
  gettype( 'fb' ): object
  

Relevant PHP issues:

• ​#36759 Objects destructors are invoked in wrong order when script is finished
• ​#39381 __destruct() bug
• ​#39546 Objects destroyed before output buffering
• ​#40104 Workflow Issue: Object destruction vs Output buffer
• ​#42132 suggest a fix for global object got destructed in output buffer handler

Relevant Stack Overflow threads:

• ​What determines when a class object is destroyed in PHP?
• ​Class object not working inside ob_start() callback
• ​In which order are objects destructed in PHP?

The discussion on #3354, starting from comment:32:ticket:3354, appears to corroborate these results.

To summarize, [5462] was the real fix here, not [4686]. 21402.2.diff​ should be good to go.

[SergeyBiryukov]

In 47107:

Cache API: Remove WP_Object_Cache::__destruct() and wpdb::__destruct().

Originally added in [4686], these constructor/destructor pairings were designed to prevent the objects from being destroyed before shutdown, when output buffers are flushed.

A deeper investigation reveals that this approach didn't quite work as expected and was later made redundant by introducing wp_ob_end_flush_all() in [5462].

Props wonderboymusic, nacin, Mte90, SergeyBiryukov.
Fixes #21402.