WordPress.org

Make WordPress Core

Opened 8 years ago

Closed 6 months ago

#21402 closed enhancement (fixed)

Remove PHP4 methods, format consistently

Reported by: wonderboymusic Owned by: SergeyBiryukov
Milestone: 5.4 Priority: normal
Severity: normal Version:
Component: Cache API Keywords: has-patch early
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 (3)

format-cache.diff (6.5 KB) - added by wonderboymusic 8 years ago.
21402.diff (781 bytes) - added by wonderboymusic 8 years ago.
21402.2.diff (829 bytes) - added by Mte90 4 years ago.
refreshed

Download all attachments as: .zip

Change History (28)

#1 @SergeyBiryukov
8 years ago

  • Component changed from General to Cache

#2 in reply to: ↑ description ; follow-up: @nacin
8 years 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.

#3 @wonderboymusic
8 years 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?

@wonderboymusic
8 years ago

#4 @wonderboymusic
8 years 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

#5 @ryan
7 years ago

  • Milestone changed from 3.6 to Future Release

#6 @wonderboymusic
7 years 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

#7 @nacin
7 years 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?

#8 @jeremyfelt
7 years ago

  • Keywords has-patch added

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

#9 @nacin
7 years 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.

#10 @chriscct7
5 years ago

  • Keywords needs-testing needs-refresh added

@Mte90
4 years ago

refreshed

#11 @Mte90
4 years ago

  • Keywords needs-refresh removed

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

#12 @Mte90
3 years ago

  • Keywords dev-feedback added

#13 @welcher
3 years ago

  • Keywords needs-unit-tests added; dev-feedback removed

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.

This ticket was mentioned in Slack in #core by mte90. View the logs.


3 years ago

This ticket was mentioned in Slack in #core by mte90. View the logs.


3 years ago

This ticket was mentioned in Slack in #core by jeffpaul. View the logs.


2 years ago

This ticket was mentioned in Slack in #core by mte90. View the logs.


2 years ago

#18 @SergeyBiryukov
2 years ago

  • Milestone changed from Future Release to 5.0

#19 @SergeyBiryukov
21 months ago

  • Milestone changed from 5.0 to 5.1

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

#20 @pento
18 months ago

  • Milestone changed from 5.1 to Future Release

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

#21 @SergeyBiryukov
11 months ago

#41615 was marked as a duplicate.

#22 @SergeyBiryukov
8 months ago

  • Keywords early added
  • Milestone changed from Future Release to 5.4
  • Owner set to SergeyBiryukov
  • Status changed from reopened to reviewing

This ticket was mentioned in Slack in #core by david.baumwald. View the logs.


6 months ago

#24 in reply to: ↑ 2 @SergeyBiryukov
6 months ago

  • Keywords needs-testing needs-unit-tests removed

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:

Relevant Stack Overflow threads:

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.

Last edited 6 months ago by SergeyBiryukov (previous) (diff)

#25 @SergeyBiryukov
6 months ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

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.

Note: See TracTickets for help on using tickets.