Clean up constructors and destructors

[hakre]

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.

[hakre]

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.

[hakre]

Improved Patch

[hakre]

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.

[hakre]

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

[hakre]

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).

[hakre]

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)).

[hakre]

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.

[scribu]

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

[hakre]

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

[dd32]

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)

[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.

[filosofo]

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.

[jacobsantos]

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.

[hakre]

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.

[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.

[hakre]

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.

[hakre]

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

[hakre]

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

[ryan]

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

[nacin]

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

[hakre]

Related: #14816

[hakre]

Related: [15639]
Related: [15637]

[hakre]

Related: #14920

[hakre]

Tag: MIGPHP5

[hakre]

see close of #14920. Reopen for 3.2?

[hakre]

Related: #16768 (current ticket)