Add wp_trigger_error() and tests #5144

hellofromtonya · 2023-09-05T21:54:40Z

Introduces wp_trigger_error() as a wrapper around PHP's native trigger_error().

As a wrapper, it's lean and not opinionated about the message.

The What

Does not trigger the error if WP_DEBUG is not true.
Includes wp_trigger_error_run action to allow hooking in for backtracing and deeper debug.

_doing_it_wrong() includes a similar action.

Pass the function name.
Currently the function name is passed to it, similar to _doing_it_wrong(). The function's name is added to the start of the message to tie the function to the message.

The function name could be computed from:

$stack = debug_backtrace( DEBUG_BACKTRACE_IGNORE_ARGS, 2 );

But then I wondered:

Are there use cases where the developer does not want the function name put into the message, but rather would like to construct the message themselves?

Or are there use cases where the developer does not want the function name at all in the message?

For both of these cases, the developer can pass an empty string for the $function_name parameter.

No WordPress version is passed to it.

Unlike _doing_it_wrong(), this new function is a wrapper to only trigger the error when in WP_DEBUG mode. Where _doing_it_wrong() intends to loudly alert developers "Hey you're doing it wrong - fix it", wp_trigger_error() is not opinionated and does not add wording. Rather, it passes the given message to trigger_error().

but "milder" messaging

Let the messaging come from the calling code, i.e. at the point in the call stack where the error/warning/notice/deprecation happens. Keep it simple.

References:

PHP native trigger_error() https://www.php.net/manual/en/function.trigger-error.php
Error constants are found here https://www.php.net/manual/en/errorfunc.constants.php

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

This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

tests/phpunit/tests/functions/wpTriggerError.php

felixarntz

@hellofromtonya Implementation looks great to me.

I left a comment in https://core.trac.wordpress.org/ticket/57686#comment:20, I'd love for us to define the scope for when to use the new function a bit better. Potentially the outcome could even be added to the function doc-block as that would convey the message to any 3P developer browsing the docs too.

costdev

Looks great @hellofromtonya!

azaozz · 2023-09-06T19:09:09Z

Yep, this looks okay, quite similar to the earlier PR: #5122.

Was thinking it would probably be better to use the new wp_trigger_error() in _doing_it_wrong(). Perhaps this would make it a bit clearer which one to use (i.e. the general use function is trigger_error, while doing_it_wrong is for special cases). Also maybe some more inline docs/explanations about the intended use.

azaozz · 2023-09-06T19:18:10Z

src/wp-includes/functions.php

+	}
+
+	/**
+	 * Fires when the given function triggers a user-level error/warning/notice/deprecation message.


It's not a filter, it's an action :)

Actually wondering which may be more useful here:

Seems an action can only be used to do debug_backtrace() (although there is a warning about using it in PHP 7+).

A filter can also be used for debug_backtrace() but can also be used to short-circuit the rest of the function. Could that be useful in some cases?

A filter can also be used for debug_backtrace() but can also be used to short-circuit the rest of the function. Could that be useful in some cases?

What would be a valid use case to short-circuit trigger_error() when WP_DEBUG is on?

Each of the _deprecated_*() and _doing_it_wrong() already have a short-circuit built into them. I'm thinking wp_trigger_error() doesn't need a short-circuit, as it's just a trigger_error() wrapper.

hellofromtonya · 2023-09-06T19:18:55Z

Was thinking it would probably be better to use the new wp_trigger_error() in _doing_it_wrong(). Perhaps this would make it a bit clearer which one to use (i.e. the general use function is trigger_error, while doing_it_wrong is for special cases). Also maybe some more inline docs/explanations about the intended use.

@azaozz I agree. Follow-up patches and commits will happen once it's introduced into Core. I noted follow-up actions here https://core.trac.wordpress.org/ticket/57686#comment:24.

azaozz · 2023-09-06T19:21:29Z

src/wp-includes/functions.php

+		$message = sprintf( '%s(): %s', $function_name, $message );
+	}
+
+	trigger_error( $message, $error_level );


This would probably be a bit better if it was using esc_html() (if available) for the message.

None of the existing trigger_error() instances in Core use esc_html() including _doing_it_wrong() and _deprecated_*() functions. Should they be?

The error messages are intended for the server logs. For those messages, escaping isn't necessary.

What about when errors are posted to the screen?

I think the messages are hardcoded, not pulled from the database or user input. Right? If yes, then escaping shouldn't be necessary either.

Thinking about this more ... trigger_error() is for hardcoded PHP error/warning/notice/deprecation messages. I don't think they need escaping.

Passing a $message of '<script>alert( "Howdy!" );</script>' will output an alert to the browser window. It's theoretically possible that a wp_trigger_error() call could include a $message parameter containing user input or the like. Escaping late here might be worth doing.

However it's worth noting that the other functions don't currently do this, but would start to if we implement escaping here and make the planned change from their trigger_error() calls to wp_trigger_error().

Just tested.

Update: Silly me - I already added esc_html( $message ) while testing. 🤦‍♀️

Ignore this comment please.

~~The output of trigger_error() is a string. Thus any HTML is output as a string, rather than a valid HTML element within the browser.~~

~~For example:~~

wp_trigger_error( '', 'Message <strong>should not include</strong> <script>alert( "Howdy!" );</script> the function as this is not within a function scope.' );

~~outputs in Chrome and Edge as:~~

<br> <b>Notice</b>": Message <strong>should not include</strong> <script>alert( "Howdy!" );</script> the function as this is not within a function scope. in " <b>/var/www/src/wp-includes/functions.php</b> on line <b>6080</b><br>

~~outputs in Firefox as:~~

<br> <b>Notice</b>: Message <strong>should not include</strong> <script>alert( "Howdy!" );</script> the function as this is not within a function scope. in <b>/var/www/src/wp-includes/functions.php</b> on line <b>6080</b><br>

~~As it's a string and not valid HTML, escaping shouldn't be needed.~~

Appears it does need to be escaped if the message is to be displayed in the browser.

Warning HTML entities in message are not escaped. Use htmlentities() on the message if the error is to be displayed in a browser.

Heh, yes, thinking this is the reason why most (all?) other strings/messages that come from PHP or WP, like in die() are HTML escaped.

Applying this new function in each _deprecated_*() and _doing_it_wrong() function ensures their messages are also escaped. Cool.

Done ee89718 ✅

Great catch @azaozz ! With that, I think this PR is ready for commit.

For using it in _doing_it_wrong() and _deprecated_*() functions, I think I'll do a follow-up PR as the batch likely needs a proper discussion.

hellofromtonya · 2023-09-06T22:07:47Z

Thank you everyone for your contributions! Committed via https://core.trac.wordpress.org/changeset/56530.

Add wp_trigger_error and tests

1112623

hellofromtonya requested review from azaozz, felixarntz and costdev September 5, 2023 22:12

mukeshpanchal27 reviewed Sep 6, 2023

View reviewed changes

tests/phpunit/tests/functions/wpTriggerError.php Show resolved Hide resolved

hellofromtonya added 2 commits September 6, 2023 08:58

Update test annotations

44fea64

Remove invalid error level check

25dafc7

felixarntz approved these changes Sep 6, 2023

View reviewed changes

costdev approved these changes Sep 6, 2023

View reviewed changes

azaozz reviewed Sep 6, 2023

View reviewed changes

Escape the message

ee89718

hellofromtonya closed this Sep 6, 2023

hellofromtonya deleted the try/wp_trigger_error branch September 6, 2023 22:08

hellofromtonya mentioned this pull request Sep 26, 2023

Introduce wp_trigger_error() #5122

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add wp_trigger_error() and tests #5144

Add wp_trigger_error() and tests #5144

hellofromtonya commented Sep 5, 2023 •

edited

felixarntz left a comment

costdev left a comment

azaozz commented Sep 6, 2023

azaozz Sep 6, 2023

hellofromtonya Sep 6, 2023

hellofromtonya Sep 6, 2023

hellofromtonya commented Sep 6, 2023

azaozz Sep 6, 2023

hellofromtonya Sep 6, 2023 •

edited

hellofromtonya Sep 6, 2023

costdev Sep 6, 2023

hellofromtonya Sep 6, 2023 •

edited

hellofromtonya Sep 6, 2023

azaozz Sep 6, 2023 •

edited

hellofromtonya Sep 6, 2023

hellofromtonya Sep 6, 2023

hellofromtonya Sep 6, 2023

hellofromtonya commented Sep 6, 2023

Add wp_trigger_error() and tests #5144

Add wp_trigger_error() and tests #5144

Conversation

hellofromtonya commented Sep 5, 2023 • edited

The What

felixarntz left a comment

Choose a reason for hiding this comment

costdev left a comment

Choose a reason for hiding this comment

azaozz commented Sep 6, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hellofromtonya commented Sep 6, 2023

Choose a reason for hiding this comment

hellofromtonya Sep 6, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hellofromtonya Sep 6, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

azaozz Sep 6, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hellofromtonya commented Sep 6, 2023

hellofromtonya commented Sep 5, 2023 •

edited

hellofromtonya Sep 6, 2023 •

edited

hellofromtonya Sep 6, 2023 •

edited

azaozz Sep 6, 2023 •

edited