Fatal error: Call to undefined method WP_Error::__destruct()

[westi]

Some fixes were missed when fixing #11168

From: miqrogroove

Missed WP_Widget_RSS and possibly others. See ​http://wordpress.org/support/topic/343166

Patch to be attached from scribu:

Fatal error: Call to undefined function: destruct() in /home/content/.../wp-includes/default-widgets.php on line 735

[westi]

Patch to fix the issue from scribu originally attached to #11168

[Denis-de-Bernardy]

see also #11219.

​http://core.trac.wordpress.org/ticket/11219#comment:10

/me sticks tongue.

[Denis-de-Bernardy]

isn't it amazing how a three liner patch that basically goes, if we try to convert utf-8 to utf-8 without mb_string or iconv, then just assume it's things are fine, could have made a difference? :-P

[miqrogroove]

Punting #11219 to version 3 was an obvious mistake. But apparently, us commoners do not understand "what constitutes various severity levels" around here.

[dd32]

Punting #11219 to version 3 was an obvious mistake.

It would've prevented a odd case, But it would not have 'fixed' this error, This error could occur under a variety of situations.. Utf8 being one cause of a error conditional.

#11219 should've really been reported directly to SimplePie rather than going through WordPress though..

[Denis-de-Bernardy]

Replying to dd32:

Punting #11219 to version 3 was an obvious mistake.

It would've prevented a odd case

*cough*... *chokes*... *nearly dies*... how is failing to convert utf-8 to utf-8 an *ODD* case?!?

[miqrogroove]

Replying to dd32:

rather than going through WordPress though..

/sigh Here we go again with taking zero ownership of legitimate errors.

[dd32]

how is failing to convert utf-8 to utf-8 an *ODD* case?!?

Its an Odd case in the sense of "Why the hell is SimplePie trying to convert utf8 to utf8 in the first place!"

/sigh Here we go again with taking zero ownership of legitimate errors.

Reporting it here is fine, But it needs to be reported upstream as well. I'm sick of people like you expecting ALL developers here to know EVERYTHING about 3rd party script. /ignoring ticket.

[Denis-de-Bernardy]

Replying to dd32:

how is failing to convert utf-8 to utf-8 an *ODD* case?!?

Its an Odd case in the sense of "Why the hell is SimplePie trying to convert utf8 to utf8 in the first place!"

Oh really?!? I really wonder how that might be. And, heck, wasn't that my patch, and my second patch, was seeking to fix in the first place?!?

Why the heck it got rejected in the first place is beyond me. I can say this much though: that it got rejected did not inspire me to promptly write new ones; and the two core devs who rejected it without giving much thoughts to it were dead wrong.

Reporting it here is fine, But it needs to be reported upstream as well. I'm sick of people like you expecting ALL developers here to know EVERYTHING about 3rd party script. /ignoring ticket.

Good for you if you can afford to not understand the software you're using. The fact of the matter is that a few of us end up facing the end users who experience these issues, and they end up needing to know those 3rd party scripts that we include.

Matt wrote me the other day saying he was disappointed that, even though I contributed to core, I was recommending to wait until WP 2.9.1. He had his reasons, I had mine. And until blatant workflow problems such as this one are solved once and for all, I goddam will continue my customers to NOT upgrade to WP unless it's a bug fix release.

[miqrogroove]

Replying to dd32:

you expecting ALL developers here to know EVERYTHING about 3rd party script.

My expectation is problems in WordPress being tracked to resolution. This idea that I want you to fix it personally is perhaps a misunderstanding.

[hakre]

destruct() is a so called magic function that is intended to be called by the php interpreter by destroying objects (garbage collector). A class itself should never relay upon it nor should it be manually called.

The code touched by this ticket put that to my attention. I already wrote about those mostly unnecessary destruct() routines in some other ticket (#10861), will check now if this applies to $rss->__destruct(); as well.

[hakre]

Now I checked the code. There is no need at all to manually execute $rss->__desctruct(); And calling unset($rss) at the end of the function while $rss is a local variable is just useless as well.

I'll attach a patch that is properly reflecting this.

[hakre]

paranoid variant

[hakre]

Added a paranoid variant as well for those who would be argue that errors in PHP should need taken care of in wordpress instead of upstreaming.

[miqrogroove]

Replying to hakre:

There is no need at all to manually execute $rss->__desctruct();

If the object has self-referencing members, as indicated in class SimplePie, then PHP4 will not automatically destroy it when it falls out of scope. I believe that is the purpose of those calls.

[ryan]

(In [12475]) Make sure we have a SimplePie object before calling destruct. Props scribu. see #11518

[ryan]

[12476] for 2.9.

Trying scribu's patch since that's what we do elsewhere.

[ryan]

See #11074 for why the destruct calls are there.

[miqrogroove]

Just to update matters here.. While I was making jokes at ryan's expense in the forum (but it's all in good fun!) it occurred to me that the SimplePie default timeout is 10 seconds, which we determined causes curl to crap itself when certain setops are used in PHP.

These errors are likely being caused by the bug already fixed.

[miqrogroove]

(I guess that's what westi meant by "the default for feeds")

Anyway patch looks good. Thanks for the reminder that PHP5 doesn't call self-referenced destructors until 5.3.

[hakre]

Replying to miqrogroove:

Replying to hakre:

There is no need at all to manually execute $rss->__desctruct();

If the object has self-referencing members, as indicated in class SimplePie, then PHP4 will not automatically destroy it when it falls out of scope. I believe that is the purpose of those calls.

Yes, there was once a bug in the garbage collector in PHP, you're right. But it's a fault in class SimplePie to name their own clean-up function destruct() because destruct() is a magic to be called by the garbage collector. I think that should be patched and the function renamed when done properly.

[hakre]

Just for documentation:

Caution

PHP reserves all function names starting with {{}} as magical. It is recommended that you do not use function names with {{}} in PHP unless you want some documented magic functionality.

​Taken from the PHP Manual

Normally I would strongly argument for upstream, but SimplePie looks dead. Do we have an own branch of SimplePie within the project?

[hakre]

Replying to miqrogroove:

(I guess that's what westi meant by "the default for feeds")

Anyway patch looks good. Thanks for the reminder that PHP5 doesn't call self-referenced destructors until 5.3.

Well as long as the object still exists, __destructors are never called because the object itself still exists, even if some variable that references a (one) instance is unset. That's by definition and ​not a bug. The Bug which - as quote here in trac and referenced ​there on the simplepie wiki - was "fixed in 5.3" (​PHP BUG #33595) is actually an improvement of the garbage collector (GC). The GC is called from time to time to free memory (but not explicitly on unset). As already written, it's the GC which is invoking the __destruct() function and those functions should never be manually called.

In the discussion on the PHP ticket you find some similar comments like here in a more general sense.

[miqrogroove]

Yes, many languages do not destruct dynamically-allocated memory, nor automatically call parent constructors and destructors of inherited classes. It is standard practice to explicitly call constructors and destructors whenever needed in those situations. The most complex OOP patterns even have an explicit disposal system to handle interdependency and synchronization issues.

[hakre]

Replying to miqrogroove:

It is standard practice to explicitly call constructors and destructors whenever needed in those situations.

Just to add a counter-point from the other direction:

It is bad practice to use the magic function __destruct() for any other purpose than the intended one.

In this case, it's not the propper function name even if the name somehow seems fitting. That's important because we have a misuse in simplepie here. As written, it should be replaced with some other named function (maybe destroy()?) to correct this. The magic function __destruct() is not designed to clean up circular references.

To close the circle: The intention is correct that self-references need to be destroyed (otherwise the memory would stay in use with PHP, even as well in 5.3 see below). The current implementation factually does this but the pseudo-destructor function has the wrong name.

---

Just to show that it's a feature not a bug with 5.3 (while digging into this and in case that's usefull background information for any dev here), there is actually need to enable garbage collection for circular references because it's optional. It's the ​gc_enable function. Introduced in ​5.3.0 30-June-2009.

---

Regarding SimplePie fixes, in #11219 we do not do any upstream for simplepie any longer.

[hakre]

Created a ticket for the SimplePie Problem: #11597

[miqrogroove]

Replying to hakre:

Just to add a counter-point from the other direction:

It is bad practice to use the magic function __destruct() for any other purpose than the intended one.

I believe that's a misinterpretation of the docs. What the PHP docs say literally is not to use function names beginning with unless you want the magic behavior for that function. I've never seen anything that says you can't or shouldn't call on a magic function.

[hakre]

Replying to miqrogroove:

Replying to hakre:

Just to add a counter-point from the other direction:

It is bad practice to use the magic function __destruct() for any other purpose than the intended one.

I believe that's a misinterpretation of the docs. What the PHP docs say literally is not to use function names beginning with unless you want the magic behavior for that function. I've never seen anything that says you can't or shouldn't call on a magic function.

I just did wrote what I did wrote, no need to misread it or to add something else to it:

It is bad practice to use the magic function __destruct() for any other purpose than the intended one.

That's what I'm concerned about. The intended purpose of __destruct() is the one that it get's called by the PHP garbage collector whenever the GC seems it fit to call that function. For example this must not be related to a specific unset operation.

If you want to clean up circular references as discussed in #11597 as well, that function is just not fitting for clearing up memory precisely nor is that the defined intention.