WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 4 years ago

#11597 closed defect (bug) (invalid)

__destruct() is not designed to clean up circular references

Reported by: hakre Owned by:
Milestone: Priority: normal
Severity: normal Version: 2.9
Component: General Keywords: has-patch
Focuses: Cc:

Description

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.

Attachments (2)

11597.patch (3.6 KB) - added by hakre 4 years ago.
Full Patch
11597-simplepie-only.patch (1.5 KB) - added by hakre 4 years ago.
Changes in SimplePie only

Download all attachments as: .zip

Change History (16)

hakre4 years ago

Full Patch

hakre4 years ago

Changes in SimplePie only

comment:1 hakre4 years ago

Created a first patch to reflect which changes are needed.

comment:2 follow-up: nacin4 years ago

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.

comment:3 in reply to: ↑ 2 hakre4 years ago

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?

comment:4 hakre4 years ago

  • Keywords has-patch added; needs-patch removed

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

comment:5 link924 years ago

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

comment:6 dd324 years ago

  • Keywords close added

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.

comment:7 hakre4 years ago

  • Keywords close removed

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.

comment:8 follow-up: Denis-de-Bernardy4 years ago

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

comment:9 miqrogroove4 years ago

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

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.

comment:10 hakre4 years ago

  • Resolution invalid deleted
  • Status changed from closed to reopened

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

comment:11 in reply to: ↑ 8 ; follow-up: hakre4 years ago

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.

comment:12 hakre4 years ago

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.

comment:13 in reply to: ↑ 11 Denis-de-Bernardy4 years ago

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

comment:14 nacin4 years ago

  • Resolution set to invalid
  • Status changed from reopened to closed
Note: See TracTickets for help on using tickets.