Trigger errors when non-existent methods are called on objects.

[dd32]

Numerous WordPress classes include a __call() function for back-compat, however, simply return false on invalid calls.

For example, the following code outputs 'No' even though it should be a fatal error.

$wp_query = new WP_Query();

echo $wp_query->delete_all_posts() ? 'Yes' : 'No';

This can cause logic errors, where someone were to use something like: WP_User::has_role( 'something' ) and expect to receive true/false, but only ever get false. (Note that has_role() doesn't exist and won't exist: 10:ticket:22624)

Trac ticket: https://core.trac.wordpress.org/ticket/53066

[dd32]

The code in the above PR was copied almost verbatim from SimplePie::__call().

Thanks for your reply @jrfnl

1. The return false after the trigger_error() calls can be removed as it is dead code and will never be executed.

I agree, but I was considering that technically the error could be short circuited. I guess I should've just removed it.

2. Have you considered throwing an Exception instead of using trigger_error() ? What were the arguments to settle on trigger_error() anyway ?

I copied it verbatim from Simplepie in core.
It appears to mimic what the PHP behaviour would be without the __call() method existing. If you'd like to suggest an exception instead, I can change to that no problem.

Have you considered throwing an Exception instead of using trigger_error() ? What were the arguments to settle on trigger_error() anyway ?

I copied it verbatim from Simplepie in core.
It appears to mimic what the PHP behaviour would be without the call() method existing. If you'd like to suggest an exception instead, I can change to that no problem.

Well, yes, I would like to suggest that, but with reason. If I read the ticket correctly, adding the error is intended to prevent developer error by informing them when they are _doing it wrong_, i.e. calling a non-existent method.

Consider then the situation where a new method would be introduced in one of these classes, say in WP 6.2 and a plugin wants to use this new method, but also still needs to (wants to) support WP 5.8-6.1.

In WP 5.8-6.1, the method doesn't exist yet, so will fall back to __call() and receive the fatal error, so to prevent this, some checking needs to be done before the call to the method can be made and as the method may only be available via __call(), not all typical ways to check will work.

If it was an Exception, however, the plugin dev could have used a try-catch without a second thought, or even the try-catch-finally trifecta.

Demo:

• trigger_error(): ​https://3v4l.org/PQvYp
• throw new [BadMethodCall]Exception: ​https://3v4l.org/XtmLn

All together, it is my opinion that using an Exception will allow plugin developers more freedom in their choice of how to code, as well as allowing for them to write cleaner code.

@dd32 What do you think ?

If you agree, we may need to discuss what Exception to use for this and/or whether to define a custom exception in WP Core.

All together, it is my opinion that using an Exception will allow plugin developers more freedom in their choice of how to code, as well as allowing for them to write cleaner code.

Right, it does.
But I don't see any need to vary from the expected PHP behaviour, unless it's something that we're going to apply to every WP class & object.
I personally see it as over-engineering to alter the PHP behaviour just because it could make it easier for someone at some future date when a method will be added (despite the lack of similar methods having being added in a long time).

Consider that there'd be a inconsistent approach needed to catch it between different classes, eg:

if ( is_callable( [ $wp, 'method' ] ) ) {
    echo $wp->method();
} else {
    fallback_code();
}

try {
    echo $wp_query->method();
} catch( Exception $e ) {
    fallback_code();
}

// Or something out of left field, more in line with WordPress coding:
$result = $wp_user->method(); //  __call(..) { return new WP_Error( 'method_does_not_exist', __( 'The called method %s does not exist on WP_User' ) ); }
if ( is_wp_error( $result ) ) {
   fallback_code();
}

tl;dr: I believe trigger_error() is appropriate in magic methods to mimic PHP behaviour, as it provides a consistent approach between classes. Exceptions are good and all, but I believe they should be used appropriately and not just because they can be used - just like you wouldn't use WP_Error in this case although it's more inline with current WordPress error handling.

Consider that there'd be a inconsistent approach needed to catch it between different classes, eg:

That's actually untrue.

When this would be an Exception, both ways of calling the method - surrounded by defensive coding or using try-catch - would work, so people wouldn't need to change their coding style, but they now have a _choice_, while when using trigger_error(), only defensive coding will work and try-catch is made impossible.

The whole point of having catchable Exceptions is that they will behave the same as Fatal errors when _uncaught_, but give a developer the freedom to catch them and handle them appropriately if so desired.

unless it's something that we're going to apply to every WP class & object

I'd be very much in favour of such a project, though that is outside of the scope of this ticket.

Consider that there'd be a inconsistent approach needed to catch it between different classes, eg:

That's actually untrue.

You're right that it would work either way, but that's not the inconsistent approach I'm pointing out, That introducing exceptions for these classes would result in some classes would be try-catch'able, and most others wouldn't.

I'm strongly against introducing Exceptions for a PHP behaviour.

I'm also against introducing Exceptions in WordPress for this specific case, where Exceptions are not used within WordPress at all anywhere else (aside from third party libraries, whose exceptions we hide from Developers behind API wrappers).

I'm strongly against introducing Exceptions for a PHP behaviour.

Why ? PHP itself has done so for all PHP native errors by now...

I'm also against introducing Exceptions in WordPress for this specific case, where Exceptions are not used within WordPress at all anywhere else (aside from third party libraries, whose exceptions we hide from Developers behind API wrappers).

Help me out. Isn't the reason for this historic ? I.e. Basically WP needed Exceptions, but also still needed to support PHP 4.x, so to get round the limitations of PHP 4.x, WP_Error was introduced as a sort-of "poor man's Exception" ?

To me, the fact that Exceptions _couldn't_ be introduced in the past does not sound like a valid reason why they _shouldn't_ be introduced now.