__destruct() is not designed to clean up circular references

[hakre]

SimplePie misuses the magic function name __destruct to clean up it's own circular references.

Analysis and Discussion (as well as see also): See #11518

Solution: The code can be refactored by renaming the function to destroy.

[hakre]

Full Patch

[hakre]

Changes in SimplePie only

[hakre]

Created a first patch to reflect which changes are needed.

[nacin]

Quoting ​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 nacin:

Quoting ​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.

Then quote my answer to that as well nacin. I think that's not too hard for you, isn't it?

[hakre]

free would be also another good name for the function came just into my mind... .

[link92]

I'm with miqrogroove on this, and I don't intend on changing this upstream.

[dd32]

and I don't intend on changing this upstream.

To me, Thats enough of a reason for WordPress not to change it. We're using the 3rd party library, it makes no sense to change the code here.

If you'd like to dicuss that furthur, IMO, It belongs on a simplepie-centric discussion forum.

[hakre]

Upstream is not an argument any longer - SimplePie is dead and we maintain our own fork now (Ref. [12528]).

I will stick with the destroy naming so far. In case you stumble over other code.

[Denis-de-Bernardy]

I'd have one tiny objection to the current patch: a destruct() function that calls destroy() should be kept around.

[miqrogroove]

Cross-posting from #11518 :

I believe hakre's argument is based on a misinterpretation of the docs. What the PHP docs say literally is not to define 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.

This ticket seems to exist for the sake of fixing something that isn't broken, and I think it's invalid.

[hakre]

Assumption over assumption so nothing for close. Thanks miqrogroove, just ask if you have doubts.

[hakre]

Replying to Denis-de-Bernardy:

I'd have one tiny objection to the current patch: a destruct() function that calls destroy() should be kept around.

Since destroy() has to be called by user code whenever the object is to be unset, this should not be necessary - taken for granted that pattern is to be used here.

[hakre]

In .net a function name for objects that take care of cleaning up on their own is called Dispose. This info might be of use for finding a fitting name.

[Denis-de-Bernardy]

Replying to hakre:

Replying to Denis-de-Bernardy:

I'd have one tiny objection to the current patch: a destruct() function that calls destroy() should be kept around.

Since destroy() has to be called by user code whenever the object is to be unset, this should not be necessary - taken for granted that pattern is to be used here.

then again, doing so *is* necessary when you've circular references. using unset, or even setting the object to null, just won't work.

you can notice this by creating two objects, and giving each one a destructor. make the destructor call destroy(), and have that echo something. create A = new test, B = new test. A->b = B; B->a = A; then unset A and B, echo test, and watch how the script ends -- it'll call the two destructors when the script dies.

calling the function destroy() or free() rather than destruct() is mostly cosmetic, and that I agree it's prettier is the only reason I'm not re-closing. then again, this ticket has been opened for 2 months with no apparent interest from core devs... or rather, it got two votes against from the new ones...