Make WordPress Core

Opened 12 years ago

Closed 12 years ago

Last modified 12 years ago

#25155 closed defect (bug) (wontfix)

All functions returning string|WP_Error -- there is no WP_Error::__toString

Reported by: tivnet's profile tivnet Owned by:
Milestone: Priority: normal
Severity: minor Version: 2.1
Component: General Keywords:
Focuses: Cc:

Description

Example from class-wp-terms-list-table.php :

$actions['view'] = '<a href="' . get_term_link( $tag ) . '">' . __( 'View' ) . '</a>';

What happens if get_term_link returns WP_Error, in this case?

a) That part of the code, and all similar places should be fixed, I guess - to check for error before printing.
b) A WP_Error::__toString method might help in some cases. It may return an empty string, or some string constant, or WP_Error::get_error_message

Change History (4)

#1 in reply to: ↑ description ; follow-up: @nacin
12 years ago

Replying to tivnet:

What happens if get_term_link returns WP_Error, in this case?

get_term_link() only returns WP_Error if the term doesn't exist. In this case, we know the term exists.

I'm not sure if __toString() for WP_Error is a good idea. Right now, failing to catch a WP_Error often leads to a fatal error — which is often good, rather than bad, as it alerts developers to problems as they write their code. But more so, as you state — I'm not sure if it should be get_error_message(), or an empty string.

#2 in reply to: ↑ 1 ; follow-up: @tivnet
12 years ago

Replying to nacin:

In this case, we know the term exists.

That's great, as long as the same people, who remember every bit of the core code, remain on the project.... A safe approach would be to check anyway.

I'm not sure if __toString() for WP_Error is a good idea. Right now, failing to catch a WP_Error often leads to a fatal error — which is often good, rather than bad, as it alerts developers to problems as they write their code. But more so, as you state — I'm not sure if it should be get_error_message(), or an empty string.

Yes, but still, a function cannot return string|Something, if Something cannot be converted to a string. That's a major "boo-boo" in my opinion, and any PHP code validator should explode with warnings about that. (At least my IDE complains quite loudly).

I remember days when errors were raised, but not returned as objects ;-)

As for the fatal errors, they usually happen on production... when you are on vacation in a desert, with no internet connection.

#3 in reply to: ↑ 2 @c3mdigital
12 years ago

  • Resolution set to wontfix
  • Status changed from new to closed
  • Version set to 2.1

Replying to tivnet:

Replying to nacin:

In this case, we know the term exists.

That's great, as long as the same people, who remember every bit of the core code, remain on the project.... A safe approach would be to check anyway.

In this case WE KNOW the term exists because it is in core and if the term didn't exist you would get a notice for trying to get property of non object and a `Catchable fatal error: Object of class WP_Error could not be converted to string message inside your list table (with WP_DEBUG set to false). If the term doesn't exist when calling that line you have much bigger problems than an array to string error.

When using functions in your own code that could return WP_Error you get to decide how to handle it. You could ignore it by doing:
if ( ! is_wp_error( $thingy ) ) { //do stuff or you could convert it to a string and return it.

We shouldn't be supporting bad coding habits in core. Closing as wontfix.

#4 @helen
12 years ago

  • Milestone Awaiting Review deleted

Right - in the given example, if a taxonomy term list table is trying to show a row for something that isn't a term, we have bigger problems. There are plenty of checks for is_wp_error() around core where it is needed.

Note: See TracTickets for help on using tickets.