WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 3 months ago

#16782 closed enhancement (wontfix)

PHP5-Port - WP_Error Class

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

Description

A port of the class to PHP5.

Related: #16781, #6768, #16769

Attachments (4)

16782.patch (3.7 KB) - added by hakre 3 years ago.
First port with a new WP_Exception thrown in constructor as it can not return as written previously.
16782.2.patch (4.1 KB) - added by hakre 3 years ago.
Second port, similar to first one, constructor cleaned up and a missed @access tag removed.
16782.3.patch (3.4 KB) - added by hakre 3 years ago.
Standard PHP Exception
16782.4.patch (3.5 KB) - added by hakre 3 years ago.
Like previous plus WP_Error extending Exception

Download all attachments as: .zip

Change History (15)

hakre3 years ago

First port with a new WP_Exception thrown in constructor as it can not return as written previously.

comment:1 hakre3 years ago

  • Keywords dev-feedback added

Please review the patch. I would like to get feedback on the issue: How to deal with constructors return something which is not possible with PHP. Normally it's thrown an exception which is something new, therefore the patch contains a general WP_Exception.

Feedback appreciated. The WP_Error class is a nice and small class to start with such IMHO.

hakre3 years ago

Second port, similar to first one, constructor cleaned up and a missed @access tag removed.

comment:2 scribu3 years ago

I don't see any benefit in having a WP_Exception class.

hakre3 years ago

Standard PHP Exception

comment:3 hakre3 years ago

Replying to scribu:

I don't see any benefit in having a WP_Exception class.

That concrete class is just to make visible that it's an exception thrown from WP. Can be replaced with any other exception class (see #16769), but that's out of the scope of this ticket. I therefore added another patch that is just throwing an exception.

Do you see a benefit in throwing an exception at all?

comment:4 follow-up: filosofo3 years ago

I think there are at least two possible benefits to using exceptions in WP:

  • It would allow us to handle error conditions in fewer places, which means more consistent error handling.
  • It's probably much faster to catch exceptions at a few points than to do the many is_wp_error checks we do now, because we could write code that assumes a certain type of return value and let PHP handle the branch in logic when the exception is thrown instead.

comment:5 in reply to: ↑ 4 hakre3 years ago

Replying to filosofo:

I think there are at least two possible benefits to using exceptions in WP:

thanks for your feedback. Next to these two cases, how should be dealt with the invalid return statements we have in some constructors? With exceptions as I suggested or do you have some other idea?

comment:6 follow-up: scribu3 years ago

I see the value in throwing exceptions, but I don't see why we couldn't just throw WP_Error instances.

That's why I closed the other ticket.

comment:7 scribu3 years ago

In hindsight, I should have done it the other way around. Anyway...

comment:8 in reply to: ↑ 6 hakre3 years ago

Replying to scribu:

I see the value in throwing exceptions, but I don't see why we couldn't just throw WP_Error instances.

At the time being, WP_Error is not throw-able and to throw it is not an accepted practice. Not that I want to prevent you or anybody else doing so, infact I've suggested that in #16769 but I haven't done any test-code and I can not even say if that does rudimentary work or not.

That's why I closed the other ticket.

Ok, I can understand that now, but please keep in mind that this ticket is not about making WP_Error extending Exception but to just port the class to PHP5 for the constructor and visibility.

For the constructor though it looks like everybody in this ticket agrees so far that throwing an exception in case for a incomplete instantiation is good.

hakre3 years ago

Like previous plus WP_Error extending Exception

comment:9 nacin3 years ago

  • Type changed from defect (bug) to enhancement

comment:10 frederick.ding9 months ago

  • Cc frederick.ding added

comment:11 nacin3 months ago

  • Milestone Awaiting Review deleted
  • Resolution set to wontfix
  • Status changed from new to closed
Note: See TracTickets for help on using tickets.