WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 4 years ago

#16769 closed enhancement (wontfix)

Make WP_Error extending Exception to make it throw-able

Reported by: hakre Owned by:
Milestone: Priority: lowest
Severity: normal Version: 3.1
Component: General Keywords:
Focuses: Cc:

Description

Does it makes sense to make the WP_Error class extending the standard PHP Exception so we can throw it when needed? Feedback appreceated.

Attachments (2)

16769.patch (590 bytes) - added by hakre 4 years ago.
Making WP_Error extending Exception
16769.2.patch (654 bytes) - added by hakre 4 years ago.
Making WP_Error extending Exception (Updated)

Download all attachments as: .zip

Change History (18)

comment:1 @hakre4 years ago

Related: #16782 - Contains a patch with a WP_Exception class.

comment:2 @scribu4 years ago

  • Milestone Awaiting Review deleted
  • Resolution set to duplicate
  • Status changed from new to closed

Closing as dup of #16782, to keep discussion in one place.

comment:3 @hakre4 years ago

  • Resolution duplicate deleted
  • Status changed from closed to reopened

This is not a duplicate of #16782, re-opening to keep things apart preventing clutter

@hakre4 years ago

Making WP_Error extending Exception

@hakre4 years ago

Making WP_Error extending Exception (Updated)

comment:4 @hakre4 years ago

I've attached a patch that extends WP_Error from Exception.

comment:5 follow-up: @westi4 years ago

  • Milestone set to Future Release
  • Priority changed from normal to lowest
  • Severity changed from normal to trivial
  • Summary changed from PHP5-port - Make WP_Error extending Exception to make it throw-able to Make WP_Error extending Exception to make it throw-able

Switching to an Exception and throwing it changes the whole way in which we use WP_Error.

I'm not convinced we need to do this.

  • What is the benefit?
  • How is this backwards compatible with old code?

comment:6 in reply to: ↑ 5 @hakre4 years ago

  • Severity changed from trivial to normal

Replying to westi:

Switching to an Exception and throwing it changes the whole way in which we use WP_Error.

Well actually prior to PHP 5 exceptions were not available but the suggestion given with the patch is not changing much so far, it's just extending the current.

I'm not convinced we need to do this.

  • What is the benefit?

To have an WP based, throw-able error class. If you don't know what the benefit of a concrete exception class is, please let us know.

  • How is this backwards compatible with old code?

I expect it to be 100% compatible. The WP_Error class can be instantiated as known and returned from functions. If you see some scenarios where this should not be the case, please report them in this ticket.

comment:7 follow-up: @filosofo4 years ago

We currently have poor-man's exceptions in a number of places. For example, here a WP_Error object is returned, but nothing in core WP does anything with that return value. So upon error the logic branches, an error object is created, but nothing can be done with the helpful information in the way that a thrown exception could be caught.

comment:8 in reply to: ↑ 7 ; follow-ups: @westi4 years ago

Replying to filosofo:

We currently have poor-man's exceptions in a number of places. For example, here a WP_Error object is returned, but nothing in core WP does anything with that return value. So upon error the logic branches, an error object is created, but nothing can be done with the helpful information in the way that a thrown exception could be caught.

The real bug is that nothing actually checks for the WP_Error that is returned.

Switching to throwing the Error Object instead makes it harder to track error handling correctly in my experience.

Ideally everywhere we could return a WP_Error we should be checking for it in the calling code and anywhere we don't is a bug.

comment:9 in reply to: ↑ 8 @hakre4 years ago

Replying to westi:

The real bug is that nothing actually checks for the WP_Error that is returned.

If you consider that a bug, please open a ticket. We can reference it here while keeping the discussion on topic. The suggestion made in this ticket is not about rewriting the code wherever WP_Error is in use.

This is not going to be constructive at the moment.

comment:10 in reply to: ↑ 8 ; follow-up: @filosofo4 years ago

Replying to westi:

The real bug is that nothing actually checks for the WP_Error that is returned.

One of the great things about thrown exceptions is that you can reduce the number of error checks. Currently we have an error-checking structure roughly like this:

function a (){
   $b = b();
   if ( is_wp_error( $b ) ) {
      return $b;
   } else {
      $c = c( $b );
      if ( is_wp_error( $c ) ) {
         return $c;   
      } else {
         echo $c->property;
      }
}
function b (){...}
function c (){...}

$result = a();
if ( is_wp_error( $result ) ) {
   echo $result->get_error_message();
}

With thrown exceptions, we never have to check whether the returned object is an error object. PHP takes care of that for us:

function a (){
   $b = b();
   $c = c( $b );
   echo $c->property;
}
function b (){...}
function c (){...}

try {
   $result = a();
} catch( WP_Exception $err ) {
   echo $result->getMessage();
}

The second example is easier to grok and does less work to achieve the same result.

Switching to throwing the Error Object instead makes it harder to track error handling correctly in my experience.

One simple thing we can do to address that is have different exception classes for different code purposes, and throw the relevant kind of exception:

class WP_DB_Exception extends WP_Exception {}
class WP_Taxonomy_Exception extends WP_Exception {}
class WP_Capabilities_Exception extends WP_Exception {}

So you might show a WP_DB_Exception message only when in debug mode, but always show the WP_Capabilities_Exception. (These aren't great examples but you get the idea.)

And there is the Exception getTrace method if you really want to track the origin of the Exception in the code.

comment:11 in reply to: ↑ 10 @westi4 years ago

Replying to filosofo:

Replying to westi:

The real bug is that nothing actually checks for the WP_Error that is returned.

One of the great things about thrown exceptions is that you can reduce the number of error checks. Currently we have an error-checking structure roughly like this:

I know you can do this.

But it is a programming style that I hate.

It makes debugging and groking the error handling in a complex application harder rather than simpler.

I don't see a strong benifit for WordPress in switching to exceptions over handling errors correctly where they occur.

I never want a theme author to have to know what an Exception even is!

comment:12 follow-up: @scribu4 years ago

I agree with westi. The only place exceptions would be somewhat useful would be in the taxonomy API, but since most of the functions there can also be called directly, you would probably end up with more try / catch blocks than is_wp_error() checks.

comment:13 @hakre4 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

It's interesting to see which opinions get exchanged here but I don't think it's possible to smash that easily the one against the other. The overall topic of Error and Exception Handling is far less than trivial.

So thanks for sharing your opinions and participating in this ticket.

Mine is quite simple: Switching to Exceptions for error handling in PHP makes code more clean, easier to test, simpler and needs less code. It enables one to write more complex applications. Debugging benefits highly from using exceptions.

Next to that exceptions can be used to signal more than only error exceptions.

But the question was: does it make sense for Wordpress? For those who fear change, most certainly not, they would highly dislike to adopt. Introducing something with no acceptance is a no-go. For those who think that it introduces more work, the acceptance will be comparable low.

As this ticket has already seen it's active period I will close it as fixed as my original question is answered and extensive feedback has been given:

No, there is no actual need of an exception class that is part of WordPress seen.

Feel free to re-open.

comment:14 in reply to: ↑ 12 @hakre4 years ago

Replying to scribu:

.. The only place exceptions would be somewhat useful would be in the taxonomy API, but since most of the functions there can also be called directly, you would probably end up with more try / catch blocks than is_wp_error() checks.

I do not know that much about the taxnonomy API, but a place where returning error values does not work is the instantiation of objects. Constructors can never return a WP_Error object nor false on error (as long as the class of the object is not WP_Error).

comment:15 @dd324 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

comment:16 @dd324 years ago

  • Milestone Future Release deleted
  • Resolution set to wontfix
  • Status changed from reopened to closed

As no change has been made to WordPress here, and the Fixed status is reserved for an issue being fixed (either in core, or beign dealt with externally on WordPress.org or similar); I'm going to close this as wontfix, this leaves for the potential for revisiting someday.

Note: See TracTickets for help on using tickets.