WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 4 years ago

Last modified 3 years ago

#10861 closed defect (bug) (wontfix)

Clean up constructors and destructors

Reported by: hakre Owned by:
Milestone: Priority: normal
Severity: normal Version: 2.8.4
Component: General Keywords:
Focuses: Cc:

Description

I assume this once was a try to have a descructor implementation for PHP 4 as well. But the way the destructor was registered in the constructor is broken. Even PHP 5 registeres a shutdown function (the destructor) while the real implementation is already in there.

Therefore register shutdown function is only needed in PHP 4. The provided patch reflects that (partially).

Then it is plain wrong to have a PHP 5 destructor return a value. It returns void by definition. This is properly reflected in the patch now. BTW, having a return value on a shutdown function is pretty useless.

Attachments (7)

10861.patch (1.1 KB) - added by hakre 5 years ago.
10861.2.patch (1.1 KB) - added by hakre 5 years ago.
Improved Patch
10861_wp-diff.patch (475 bytes) - added by hakre 5 years ago.
10861_wp-dependencies.patch (627 bytes) - added by hakre 5 years ago.
10861_wp-capabilities.patch (1.0 KB) - added by hakre 5 years ago.
10861_cache.patch (1.2 KB) - added by hakre 5 years ago.
10861_dependencies_scripts_styles.patch (1.4 KB) - added by hakre 5 years ago.
Unnecessary code removed (there weren't even any arguments)

Download all attachments as: .zip

Change History (33)

hakre5 years ago

comment:1 hakre5 years ago

The constructor (PHP 4, PHP 5) still might have errors. In case PHP 5 prefers the class names function instead of the __construct() function (for example for backwards compability reasons, I was not able to clarify that in all details), the register_shutdown_function will be called with PHP 4 and PHP 5. It should only called with PHP 4.

Additionally, having an empty function (wpdb::__destruct()) does not make any sense. This leads to asking oneself: what is all the fuzz about? Constructor, Destructor, NIL. I assume this needs a cleanup.

This would also lead to a solution for some other important point: Never register a __destruct() function as a shutdown function. This will most often lead to problems (the function is called two times) and errors (the object is already destroyed when the shutdown function is (to be) executed).

Therefore I suggest to remove this all from the object, I will provide a second patch for it.

Last edited 3 years ago by hakre (previous) (diff)

hakre5 years ago

Improved Patch

comment:2 hakre5 years ago

Added an improved patch that removes the obsolete destructor function. Next to this it is not necessary to have the PHP4 constructor return something explicitly.

hakre5 years ago

comment:3 hakre5 years ago

While being in the topic, I stumbled over a little improvement to be made in WP_Text_Diff_Renderer_Table.

comment:4 hakre5 years ago

WP_Dependencies was full of logic by doing nothing. After this patch it's therefore documented that this is done by intention (while reducing the code for doing nothing).

comment:5 hakre5 years ago

this ones more or less as a suggestion to document those functions that function for PHP4 and PHP5 (in contrast to those that manually call the PHP5 constructor in PHP4 which does not happen in PHP5 (verified)).

hakre5 years ago

comment:6 hakre5 years ago

This one is nicer and more fitting to the first one. A uselesst destructor function that is not properly implemented in any case. In this case, this was already tagged as todo. So if this was already on someones list, take a peek.

comment:7 hakre5 years ago

  • Summary changed from wpdb constructor and desctructor concept is broken to wpdb / WP_Object_Cache constructor and desctructor concept is broken

comment:8 scribu5 years ago

  • Milestone changed from Unassigned to 2.9
  • Summary changed from wpdb / WP_Object_Cache constructor and desctructor concept is broken to Clean up constructors and destructors

Perhaps you should wrap them up in a single patch for easier handling.

comment:9 hakre5 years ago

Having one patch can be an option, but I would like to see more feedback first. I think it's important that some of the core coders take a look and comitters should signal wether or not such changes are welcomed.

code quality related: #10758

comment:10 follow-up: dd325 years ago

attachment 10861_wp-dependencies.patch added
WP_Dependencies was full of logic by doing nothing. After this patch it's therefore documented that this is done by intention (while reducing the code for doing nothing).

I think that code is to call the child classes that extend it perhaps? (Not sure its needed though.. Just checking)

comment:11 in reply to: ↑ 10 ; follow-up: jacobsantos5 years ago

Replying to dd32:

I think that code is to call the child classes that extend it perhaps? (Not sure its needed though.. Just checking)

That doesn't exactly make sense. The child classes can define their own arguments. Actually, the entire WP_Dependencies violates proper object-oriented practices, so it makes sense it would be ass backward. I think it tries too hard to be a Iterator, except without PHP5, it fails in its attempt.

comment:12 in reply to: ↑ 11 filosofo5 years ago

Replying to jacobsantos:

Replying to dd32:

I think that code is to call the child classes that extend it perhaps? (Not sure its needed though.. Just checking)

That doesn't exactly make sense. The child classes can define their own arguments. Actually, the entire WP_Dependencies violates proper object-oriented practices, so it makes sense it would be ass backward. I think it tries too hard to be a Iterator, except without PHP5, it fails in its attempt.

call_user_func_array( array(&$this, '__construct'), $args );

is a hack to keep from having to specify the parent class as the constructor in PHP 4.

WP_Scripts is the child of WP_Dependencies. If WP_Scripts used the PHP 4 class-name constructor, WP_Scripts, it would have to call the PHP 4 parent constructor explicitly, like so: parent::WP_Dependencies($args);, and then you have maintenance issues.

Instead, the hack omits the PHP 4 child class-name constructors, so when WP_Scripts is instantiated, PHP 4 calls WP_Dependencies() as its constructor, which in turn calls the child __construct() constructors.

comment:13 follow-up: jacobsantos5 years ago

That makes more sense, except it is not needed. The WP_Styles and WP_Scripts doesn't have any arguments, and putting the PHP4 constructors would not hinder readability in any way. It would make sense if the WP_Dependencies was going to be used in many more places, but it is specific container for redundant code, so that is unlikely.

PS: I was thinking of the *_Walker classes when I was talking about the iterators.

comment:14 hakre5 years ago

I see many maintenance issues with the current code but not with the proposed changes. parent::WP_Dependencies() is pretty straight forward and since the extending class always is aware of the parent class (it's hardcoded), there is no need to have that with a variable call. It's just useless.

The "variable" code looks pretty much copied over from some other project without propper checking wether it is useable here or not. It might be that the other project needs such a flexibility but it's perfectly unfitting in these cases here in WP.

There is just no need to put hacks in where no hack is needed. Each extending class knows exactly it's parent and can call without any problems the PHP4 constructor. We do not need any PHP5 __construct functions at all, PHP5 handles the PHP4 constructors just properly.

This will also help to migrate the code more properly to PHP5 some day. There is no need of a mix of PHP4 and PHP5 constructors. That's just overhead. In those cases where this can be corrected without changing much code I've made that. I have not checked the *_Walker classes until now.

Last edited 3 years ago by hakre (previous) (diff)

comment:15 in reply to: ↑ 13 ; follow-up: filosofo5 years ago

Replying to jacobsantos:

That makes more sense, except it is not needed. The WP_Styles and WP_Scripts doesn't have any arguments,

WP_Styles and WP_Scripts constructors call wp_default_styles() and wp_default_scripts(), respectively.

comment:16 in reply to: ↑ 15 hakre5 years ago

Replying to filosofo:

Replying to jacobsantos:

That makes more sense, except it is not needed. The WP_Styles and WP_Scripts doesn't have any arguments,

WP_Styles and WP_Scripts constructors call wp_default_styles() and wp_default_scripts(), respectively.

I've checked that, it's true. But IMHO this is the opposite of a straight forward way doing things. I will create a patch reflecting one of the two cases while looking for a valid PHP4 implementation that PHP5 is able to deal with the same way.

hakre5 years ago

Unnecessary code removed (there weren't even any arguments)

comment:17 ryan4 years ago

  • Milestone changed from 2.9 to 3.0

comment:18 hakre4 years ago

  • Milestone changed from 3.0 to 2.9

Hi ryan, patch is ready to commit. What's up?

comment:19 ryan4 years ago

  • Milestone changed from 2.9 to 3.0

2.9 is close. Most remaining tickets are being postponed unless they are blockers.

comment:20 nacin4 years ago

  • Milestone 3.0 deleted
  • Resolution set to wontfix
  • Status changed from new to closed

I don't see anything here we need to fix. It can be reopened but > 3.0 at this point.

comment:21 hakre4 years ago

Related: #14816

comment:22 hakre4 years ago

Related: [15639]
Related: [15637]

comment:23 hakre4 years ago

Related: #14920

comment:24 hakre4 years ago

Tag: MIGPHP5

comment:25 hakre4 years ago

see close of #14920. Reopen for 3.2?

comment:26 hakre3 years ago

Related: #16768 (current ticket)

Note: See TracTickets for help on using tickets.