WordPress.org

Make WordPress Core

Opened 2 years ago

Closed 2 years ago

#19073 closed defect (bug) (wontfix)

wp_die() can be a wrapper for WP_Error objects, but also triggers if no error is present

Reported by: F J Kaiser Owned by:
Milestone: Priority: normal
Severity: normal Version: 3.3
Component: General Keywords: has-patch
Focuses: Cc:

Description

The function wp_die( $message ); wraps up _default_wp_die_handler( $message ); and both only have the argument $message as required.

If you now pass a WP_Error( $code, $message ); object to the die handler, the function automatically extracts the title from the $data array - as long as it's an array and you've a key named 'title' and no $title argument was set when calling the function.

This means that you can do something like this:

function my_custom_fn( $args = array() ) {
    $error = '';
    if ( ! isset( $args['some_key'] ) )
        $error = new WP_Error( __FUNCTION__, 'You are missing some_key' );

    if ( is_wp_error( $error ) )
        wp_die( $error );

    // do other stuff
}

Problem with the above mentioned code is that _default_wp_die_handler( $message ); does...

$errors = $message->get_error_messages();
switch ( count( $errors ) ) :
case 0 :
	$message = '';
	break;

...and simply proceeds, even if no error is present. That's a senseless behavior.

If we would simply abort/return at this point (case: error-count is 0), we could use wp_die(); as on-demand wrapper for errors.

Attachments (1)

19073_1.patch (402 bytes) - added by F J Kaiser 2 years ago.

Download all attachments as: .zip

Change History (7)

F J Kaiser2 years ago

comment:1 F J Kaiser2 years ago

  • Cc 24-7@… added

Removed $message = ''; and set to simply return;, to interrupt output and only trigger wp_die(); if an error is present.

comment:2 F J Kaiser2 years ago

  • Keywords has-patch added

comment:3 follow-up: OoroonBai2 years ago

Why is it a senseless behavior? You can create errors without a message.

$error = new WP_Error( 'not_found', __('New error'), array( 'title' => 'Plugin Error', 'response' => '404', 'back_link' => true ) );
if( is_wp_error( $error ) ){
	
	wp_die( $error, '', $error->get_error_data() );
	
}

If someone translate the above string 'New error' with an empty string, than wp_die() would not not stop the script. But wp_die() should always stop the script when it is called.

(Sorry for my pidgin english)

comment:4 in reply to: ↑ 3 ; follow-up: F J Kaiser2 years ago

Replying to OoroonBai:

Why is it a senseless behavior? You can create errors without a message.

First, I'd say if you translate errors, you should also translate the error titles.

If someone translate the above string 'New error' with an empty string, than wp_die() would not not stop the script. But wp_die() should always stop the script when it is called.

If you got an incomplete translation and bundle it with your theme, then it's your fault as theme developer. You should never bundle unfinished parts, else you're before RC, even before beta status with your theme. When offering incomplete Errors due to missing translation strings it won't be of any help. And of how much help could a blank wp_die page be? IMHO: none. Errors are meant to help the developer. So: why display it anyway if you can't offer any help?

Sidenote: You don't have to offer the empty second argument for wp_die() and you also don't need to call the error data as 3rd. That's the reason why I'm after this: Just drop your error object in and you're fine. The rest will be done by wp_die() automatically.

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

Replying to F J Kaiser:

First, I'd say if you translate errors, you should also translate the error titles.

In an example some things are different from things you do in practice, just to point out the problem. Here we talk about the error-message, not the error-title.

If you got an incomplete translation and bundle it with your theme, then it's your fault as theme developer. You should never bundle unfinished parts, else you're before RC, even [...]

There are many ways why the message can be empty. Translatipon is one way, bad written plugins an other.
Imaging following code in a plugin:

<?php
/* collecting the vars */
$e_title = 'Where is it Error';
$e_message = '';               // Oops!!! Missing something...
$e_code = 'four-o-four';
$e_respond = 404;

/* some other code */

$error = new WP_error( $e_code, $e_message, array( 'title' => $e_title, 'respond' => $e_respond ) );

$some_important_var = false;

/* some other code */

if( 'yes, do it' != $some_important_var )
	wp_die( $error );

/* do some wired things like deleting tables, posts and so on */
?>

That's a terrible code. But there are many plugins with terrible code out there. Imaging what happend if you simply return if $e_message is not set. The plugin would not die without a message, it would delete tables, post or something else.
With the actual code, wp_die() stop the script whether $e_message is set or not.

If wp_die() needs a patch, than set $message to something like "No message". But never ever change the expected behavior (stopping the script) of wp_die().
Your actual patch returns nothing. Not true or false or something that can be checked. Not even an error is triggered. The script will be continued as nothing was happend. Try to debug such a situation, this will be hell on earth.

Sidenote: You don't have to offer the empty second argument for wp_die() and you also don't need to call the error data as 3rd. That's the reason why I'm after this: Just drop your error object in and you're fine. The rest will be done by wp_die() automatically.

Try it ;) If you want to send a respond-header, you have to set this with the 3rd argument. And you can only set a 3rd argument if you have set an (empty) second argument.
I used the following code to test it (added to the end of my theme functions.php). Try the first and the second wp_die(), you will see the different output (missing back-link). With firebug you can check the respond-header (correct 403/404 with the first, 500 with the second wp-die).

$error  = new WP_Error();
$error->add( 'forbidden', 'Forbidden', array( 'title' => 'Forbidden Error', 'response' => '403', 'back_link' => true ) );
$error->add( 'not_found', 'Not found', array( 'title' => 'Not-Found Error', 'response' => '404', 'back_link' => true ) );

 if( is_wp_error( $error ) ){

        //wp_die( $error, '', $error->get_error_data() );
	wp_die( $error );

 }

If you need respond-header which are set in WP_Error(), you have to set them explicitly in wp_die() with the 3rd argument.

comment:6 nacin2 years ago

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

If you pass a WP_Error object to wp_die(), best have a message. Not changing this -- too much can break and I don't agree with the change. If you're already calling wp_die(), you expect the script to halt.

Note: See TracTickets for help on using tickets.