WordPress.org

Make WordPress Core

Opened 6 years ago

Last modified 2 months ago

#21402 reopened enhancement

Remove PHP4 methods, format consistently

Reported by: wonderboymusic Owned by:
Milestone: 5.0 Priority: normal
Severity: normal Version:
Component: Cache API Keywords: has-patch needs-testing needs-unit-tests
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 6 years ago.
21402.diff (781 bytes) - added by wonderboymusic 5 years ago.
21402.2.diff (829 bytes) - added by Mte90 17 months ago.
refreshed

Download all attachments as: .zip

Change History (21)

#1 @SergeyBiryukov
6 years ago

  • Component changed from General to Cache

#2 in reply to: ↑ description @nacin
6 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
6 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
5 years ago

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

  • Milestone changed from 3.6 to Future Release

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

  • Keywords has-patch added

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

#9 @nacin
5 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
3 years ago

  • Keywords needs-testing needs-refresh added

@Mte90
17 months ago

refreshed

#11 @Mte90
17 months ago

  • Keywords needs-refresh removed

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

#12 @Mte90
16 months ago

  • Keywords dev-feedback added

#13 @welcher
11 months 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.


9 months ago

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


8 months ago

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


3 months ago

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


2 months ago

#18 @SergeyBiryukov
2 months ago

  • Milestone changed from Future Release to 5.0
Note: See TracTickets for help on using tickets.