Introduce wp_trigger_error() to compliment _doing_it_wrong()

[azaozz]

The _doing_it_wrong() function was introduced 12 years ago (see #15824 and [16939]) for "telling people the code they have written won't work". Since then it has been used quite a bit, currently at around 180 places in core as it is a convenient way to trigger a PHP user error.

However that convenience has resulted in shifting the initial purpose, which was to tell developers they are doing something wrong. Lately _doing_it_wrong() seems to be used in any context, not specifically to "tell off" the developers and get them to fix their code.

In that terms I think it would be good to have a "general use" function that has similar functionality but "milder" messaging. Such function can be used by the current _doing_it_wrong(), and should be enhanced a little to handle the combinations of all PHP debugging constants: WP_DEBUG, WP_DEBUG_DISPLAY, and WP_DEBUG_LOG.

[peterwilsoncc]

Coincidentally, I was discussing this with another contributor recently as a suitable helper function so would very much like to see it.

My preference would be for core to swap all instances of _doing_it_wrong() to use the new function as "you're doing it wrong" is the least helpful way of advising developers how to improve their code.

Unfortunately throwing deprecation notices in _doing_it_wrong() isn't really possible as then WordPress would be throwing notices on it's notices and, ugh, that would be ugly.

[costdev]

@azaozz should be enhanced a little to handle the combinations of all PHP debugging constants: WP_DEBUG, WP_DEBUG_DISPLAY, and WP_DEBUG_LOG.

Interesting! What are your thoughts on how this would behave for the different combinations?

[oglekler]

This is an enhancement, and we are in 12 days until Beta 1 after which we will not add new enhancement to the release, so, because there is no patch so far, I am moving this ticket to 6.4.

And it looks like this ticket needs some clarity on what needs to be done as well. I am adding reporter-feedback as well. @azaozz you may have missed previous comment.

[hellofromTonya]

I agree with @azaozz that another level of error handling is needed rather than _doing_it_wrong(). There's a mix of approaches for handling input (property / parameter) validation such as ensure the incoming value / data is of the right type.

An architectural proposal for handling this type of validation is being drafted (coming soon). I think this ticket fits nicely into that proposal.

I'm going to move this ticket to Future Release until there's a consensus on the proposal and impacts to this scope of this ticket.

[hellofromTonya]

An immediate use case happened during this alpha cycle. ​After discussions with @flixos90 and @azaozz, I'm pulling this ticket back into 6.4.

The implementation for now can address the immediate needs and what has been proposed. Then when it's time to implement input validation, the function can be modified accordingly. There's low risk as the intended behavior of triggering trigger_error() will remain.

[flixos90]

Sounds reasonable to me. So for now that function would simply be something like the following I assume?

<?php
function wp_trigger_error( $message, $error_level = E_USER_NOTICE ) {
        if ( ! WP_DEBUG ) {
                return;
        }
        trigger_error( $message, $error_level );
}

[azaozz]

Replying to hellofromTonya:

Was just about to do this but you beat me to it :)

The implementation for now can address the immediate needs

Yep, exactly. I actually have an older patch implementing this in the simplest way possible. Will attach PR later today.

[azaozz]

Replying to flixos90:

Yea, pretty much but thinking it can also append to/enhance the message about what got broken, similarly to doing_it_wrong. Let me make that PR :)

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

TODO/TBD: Wondering if it's worth it to add a simple backtrace when WP_DEVELOPMENT_MODE is true. Tested with only printing function names, no args as they can be huge sometimes. The code for it would be like:

	if ( WP_DEVELOPMENT_MODE ) {
		// Output a simple backtrace.
		ob_start();
		debug_print_backtrace( DEBUG_BACKTRACE_IGNORE_ARGS );
		$backtrace = ob_get_contents();
		ob_end_clean();

		// This may be called very early. See above about use of `__()`.
		if ( function_exists( 'esc_html' ) ) {
			$backtrace = esc_html( $backtrace );
		}

		// Space it a bit for better readability.
		$backtrace = str_replace( "\n", "\n\n", $backtrace );

		echo '<p style="white-space: pre-wrap;">';
		echo $backtrace;
		echo '</p>';
	}

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

Notes:

• Does not trigger the error if WP_DEBUG is not true.

• If given an invalid error level, uses the E_USER_NOTICE default.

Why? trigger_error() only works with E_USER family of constants. If any other value or a non-integer is given, PHP will throw a Warning (< PHP 8) | Fatal (PHP 8+). ​https://3v4l.org/ZFAS8

• 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.

Considered using backtrace to get the [class]function name. But used the same approach as _doing_it_wrong(), i.e. function name is passed to it.

The function name could be an empty string. There may be cases where code is executing outside of a function or a general error such as invalid data may be needed.

• 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.

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

[hellofromTonya]

Replying to flixos90:

Sounds reasonable to me. So for now that function would simply be something like the following I assume?

<?php
function wp_trigger_error( $message, $error_level = E_USER_NOTICE ) {
        if ( ! WP_DEBUG ) {
                return;
        }
        trigger_error( $message, $error_level );
}

@flixos90 @azaozz ​I also created a PR. It takes a different approach than _doing_it_wrong(), as it's a wrapper around trigger_error().

It's lean and not opinionated about the message itself, but rather passes the message to the PHP native trigger_error().

I did include the function name parameter to help identify what function/method triggered the error, especially when logging at logs. That parameter could be eliminated using similar code in Requests to get the [class]function name:

$stack = debug_backtrace( DEBUG_BACKTRACE_IGNORE_ARGS, 2);

@azaozz also ​created an implementation which reuses _doing_it_wrong() code.

The big question:

Should wp_trigger_error() be a trigger_error() wrapper or a milder verbiage form of _doing_it_wrong()?

Thoughts?

[costdev]

The big question:
Should wp_trigger_error() be a trigger_error() wrapper or a milder verbiage form of _doing_it_wrong()?

Thinking outside the context of "we shouldn't be using _doing_it_wrong() here", I think a wrapper for PHP's trigger_error() makes sense. _doing_it_wrong() is informing extenders of incorrect usage, whereas wp_trigger_error() could trigger errors for any reason, so I think WP_DEBUG is a sufficient guard without the need for an additional filter included in ​PR 5122.

I'm not sure that a $where/$function_name parameter is appropriate for wp_trigger_error(). _doing_it_wrong() states: "Marks something as being incorrectly called", whereas errors can be triggered anywhere, including the root-level of a file (such as ​here). If we do include it, I'd suggest we don't make it a required first parameter as this distances it from trigger_error() which most extenders will have in mind when calling wp_trigger_error(), and they may not want/need a function name to be included. trigger_error() will already include the file and line number.

An initial lean implementation with "minimum requirements" could be:

1. Accepts string $message, int $error_level = E_USER_NOTICE.
2. Bails if WP_DEBUG is disabled.
3. Fires a wp_trigger_error_run action to align with _deprecated_function(), _deprecated_constructor(), _deprecated_class(), _deprecated_argument(), _deprecated_hook(), and _doing_it_wrong().
4. Triggers the error.

If an invalid error level is passed, PHP will already output an error which will only occur when we've already ensured that WP_DEBUG is enabled, so I don't think we need the reset included in ​PR 5144.

[hellofromTonya]

Replying to costdev:

I'm not sure that a $where/$function_name parameter is appropriate for wp_trigger_error(). _doing_it_wrong() states: "Marks something as being incorrectly called", whereas errors can be triggered anywhere, including the root-level of a file (such as ​here). If we do include it, I'd suggest we don't make it a required first parameter as this distances it from trigger_error() which most extenders will have in mind when calling wp_trigger_error(), and they may not want/need a function name to be included. trigger_error() will already include the file and line number.

The file and line number will reference where trigger_error() is invoked within wp_trigger_error(), which makes debugging more difficult.

Passing the function's name adds it to the start of the error message to help with debug efforts. If it's not needed or not within a function scope, then an empty string can be passed instead.

For example:

function wp_trigger_error( $function_name, $message, $error_level = E_USER_NOTICE ) {

        if ( '' !== $function_name ) {
                $message = sprintf( '%s(): %s', $function_name, $message );
        }

        trigger_error( $message, $error_level );
}

function test_triggering_error() {
    wp_trigger_error( __FUNCTION__, 'message should include the function that triggered the error.' );
}

wp_trigger_error( '', 'Message should not include the function as this is not within a function scope.' );

test_triggering_error();

which results in:

Notice: Message should not include the function as this is not within a function scope. in /var/www/src/wp-includes/functions.php on line 6078

Notice: test_triggering_error(): message should include the function that triggered the error. in /var/www/src/wp-includes/functions.php on line 6078

Notice the file and line number are in wp_trigger_error() and not where the error is triggered. See it in action ​https://3v4l.org/3I4KF.

[costdev]

The file and line number will reference where trigger_error() is invoked within wp_trigger_error(), which makes debugging more difficult.

Of course it does My bad on that!

What do you think about moving $function_name to be a third, optional parameter so it's not necessary to include an empty string as the first parameter if it's not needed? This would be closer to PHP's native trigger_error() also.

Sort of a difference between:

Trigger an error from $function_name with this $message and $error_level.

and

Trigger an error with this $message and $error_level from $function_name.

It's not a big deal either way, just throwing it out there for discussion 🙂

[hellofromTonya]

Replying to costdev:

What do you think about moving $function_name to be a third, optional parameter so it's not necessary to include an empty string as the first parameter if it's not needed? This would be closer to PHP's native trigger_error() also.

I think keeping it as the first required parameter aligns well to other trigger_error() handling functions in Core such as _deprecated_function(), _deprecated_argument(), etc.

I suspect the majority of the error triggering use cases will occur within a function/method scope, rather than in global scope.

For those in global scope or when the developer does not want the function to appear in that position within the message, an empty string can be passed.

What do you think @costdev?

[costdev]

I think keeping it as the first required parameter aligns well to other trigger_error() handling functions in Core such as _deprecated_function(), _deprecated_argument(), etc.

Oh snap! You know I'm a stickler for consistency! ➕

I suspect the majority of the error triggering use cases will occur within a function/method scope, rather than in global scope.

I agree!

For those in global scope or when the developer does not want the function to appear in that position within the message, an empty string can be passed.

I think that's what started my thinking on this. "What's the main parameter of this function and what supplement's it?" - The "main" one for me was $message, with $function_name and $error_level supplementing it, so passing an empty string for the first parameter felt weird.

Since the $function_name will be included in nearly all cases, and for consistency's sake, I'm happy to go with $function_name, $message, $error_level = E_USER_NOTICE.

[flixos90]

Just adding my 2 cents reading the above conversation, I'm also on board with the currently proposed signature of $function_name, $message, $error_level = E_USER_NOTICE.

I would like us to come up with a clear method of differentiating between when to use _doing_it_wrong() vs when to use wp_trigger_error() though. I think it's clear the first one is to indicate to developers that they are doing something wrong. So by definition wp_trigger_error() probably covers the rest of errors. It would be great to come up with a list of examples, either anecdotally or maybe there are _doing_it_wrong() instances in core where we feel they should use wp_trigger_error() instead. Having such examples would help us better define the scope of when the new function should be used.

[hellofromTonya]

Replying to flixos90:

I would like us to come up with a clear method of differentiating between when to use _doing_it_wrong() vs when to use wp_trigger_error() though. I think it's clear the first one is to indicate to developers that they are doing something wrong. So by definition wp_trigger_error() probably covers the rest of errors. It would be great to come up with a list of examples, either anecdotally or maybe there are _doing_it_wrong() instances in core where we feel they should use wp_trigger_error() instead. Having such examples would help us better define the scope of when the new function should be used.

I agree.

The way I think of _doing_it_wrong() is:
When extenders invoke a function incorrectly, causing it to bail out and not run as expected. The message exists to alert developers: Hey, you're doing xyz wrong, your code won't work, and please fix your code. Examples:

• A function won't run because the required array key or structure was not passed to it.
• An incompatible data type is passed which can't be type juggled or handled: WP_Style_Engine_Processor::add_store().
• Something isn't supported: wp_cache_flush_runtime().

For all other cases, wp_trigger_error() is available. Examples:

• PHP 8.x deprecations: functionality will continue to work as it did previously: magic methods in WP_User_Query, WP_Text_Diff_Renderer_Table,
• PHP error parity: wp_strip_all_tags().
• Changes that are or could be automatically handled: Request wp-includes/class-requests.php.
• Something went wrong which may or may not be due to incorrect code (maybe invalid data or user input): _filter_block_template_part_area(), WP_Theme_JSON::get_user_data(), WP_Theme_JSON::set_spacing_size(),
• An extension may not be available on the server: prep_atom_text_construct().

There are many cases of strict type checks using _doing_it_wrong() which could include automatic handling and use wp_trigger_error(). For example, there are many "strict" data type checks such as for string that could handle scalars or even null. Rather than bailing out if not a string, if it's a scalar, handle it, trigger wp_trigger_error(), and keep going.

[hellofromTonya]

​The lean version of `wp_trigger_error()` has approvals from @flixos90 and @costdev. I too like this approach.

@azaozz what do you think? Are you okay with the ​lean approach in PR 5144?

Yep, this looks okay, quite similar to the earlier PR: ​https://github.com/WordPress/wordpress-develop/pull/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.

[hellofromTonya]

In talking with @azaozz, he's okay with ​lean version. He's also suggesting to replace trigger_error() in each of the _deprecated_*() and _doing_it_wrong() functions.

As follow patches and commits:

• Each of the trigger_error() instances can be replaced with wp_trigger_error().

• Each _doing_it_wrong() should be evaluated to determine if it should or could use wp_trigger_error() instead. For example, instead of bailing out when not receiving a string data type, handle it automatically if it's scalar type and trigger wp_trigger_error(). IMO there is a valid use case for _doing_it_wrong() (see comment:21) when the code won't run and it's clearly a plugin or theme doing it wrong.

• Once there's consensus on which function to use and when, then each function's docblock can be improved to include that information (which will then be available in the developers docs).

Marking ​https://github.com/WordPress/wordpress-develop/pull/5144 for commit.

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.

[flixos90]

Replying to hellofromTonya:
I agree with all your outlined examples in terms of difference in scope between the two functions, except I have concerns on the following (if I understand correctly):

There are many cases of strict type checks using _doing_it_wrong() which could include automatic handling and use wp_trigger_error(). For example, there are many "strict" data type checks such as for string that could handle scalars or even null. Rather than bailing out if not a string, if it's a scalar, handle it, trigger wp_trigger_error(), and keep going.

Not sure about this one. Even when a core function is "forgiving" on handling other data types than the intended one, if we choose to raise a PHP warning or error, I'd argue that's just as much the developer doing it wrong as it is in other places. If a function is documented to receive a string and you pass an integer, you are doing it wrong. This is not to say by any means core should not be forgiving when it can, but my point is if we raise a warning in such a case, it should still be _doing_it_wrong() IMO.

[hellofromTonya]

In 56530:

General: Introduce wp_trigger_error().

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. It accepts an E_USER family error level, meaning it is not limited to only notices.

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().

wp_trigger_error() is meant for every trigger_error() instance. It can be used:

• in _doing_it_wrong() and each _deprecated_*() function.
• for PHP 8.x deprecations.
• for PHP error parity.
• for less severe "doing it wrong" instance that do not require bailing out.
• when a component or extension is not available on the server
• for instances where it's not clear if a plugin's or theme's code is the root cause.
• and more.

Technical details:

• 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.

• Accepts an E_USER error level, but defaults to E_USER_NOTICE.

• Requires a function name, though can be an empty string. As the output message generated by trigger_error() references the file and line number where it was invoked, passing the function's name provides more information where the error/warning/notice/deprecation happened. It's intended to help with debug.

• A WordPress version number is not included.

• As messages can appear in the browser, the message is escaped using esc_html(). As noted in ​the PHP manual: "HTML entities in message are not escaped. Use htmlentities() on the message if the error is to be displayed in a browser."

References:

• ​PHP manual for `trigger_error()`.
• ​E_USER constants (error level) in the PHP manual.

Props azaozz, hellofromTonya, flixos90, costdev, peterwilsoncc, oglekler, mukesh27.
See #57686.

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

Uses wp_trigger_error() in _doing_it_wrong() and each _deprecated_*() function, i.e. instead of trigger_error().

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

[hellofromTonya]

Hmm, esc_html() for the message being passed to trigger_error() is not the right approach.

Why? Messages (like the ones generated from the _deprecated_*() and _doing_it_wrong() functions have HTML in them. When they appear in the browser, they visually look different. When they appear in the logs, the HTML tags are escaped, making them difficult to read and understand.

Example:

Consider the following:

add_action( 'init', 'test_escaping_of_trigger_error' );
function test_escaping_of_trigger_error() {
        _doing_it_wrong( __FUNCTION__, 'A non-empty string is required for blah.', '6.4' );
}

Before: without esc_html()

Before using esc_html(), the Notice reads as follows and the included HTML tags are respected in the browser:

The HTML markup:

<b>Notice</b>
:  Function test_escaping_of_trigger_error was called <strong>incorrectly</strong>. A non-empty string is required for blah. Please see <a href="https://wordpress.org/documentation/article/debugging-in-wordpress/">Debugging in WordPress</a> for more information. (This message was added in version 6.4.) in <b>/var/www/src/wp-includes/functions.php</b> on line <b>6027</b><br>

How it looks in the browser:

Notice: Function test_escaping_of_trigger_error was called incorrectly. A non-empty string is required for blah. Please see ​Debugging in WordPress for more information. (This message was added in version 6.4.) in /var/www/src/wp-includes/functions.php on line 6027

How it appears in the server logs:

[07-Sep-2023 17:47:06 UTC] PHP Notice: Function test_escaping_of_trigger_error was called <strong>incorrectly</strong>. A non-empty string is required for blah. Please see <a href="​https://wordpress.org/documentation/article/debugging-in-wordpress/">Debugging in WordPress</a> for more information. (This message was added in version 6.4.) in /var/www/src/wp-includes/functions.php on line 6027

After: With esc_html()

Compare to when the message is passed through esc_html():

The HTML markup:

<b>Notice</b>
:  Function test_escaping_of_trigger_error was called &lt;strong&gt;incorrectly&lt;/strong&gt;. A non-empty string is required for blah. Please see &lt;a href="https://wordpress.org/documentation/article/debugging-in-wordpress/"&gt;Debugging in WordPress&lt;/a&gt; for more information. (This message was added in version 6.4.) in <b>/var/www/src/wp-includes/functions.php</b> on line <b>6027</b><br>

How it looks in the browser:

Notice: Function test_escaping_of_trigger_error was called <strong>incorrectly</strong>. A non-empty string is required for blah. Please see <a href="​https://wordpress.org/documentation/article/debugging-in-wordpress/">Debugging in WordPress</a> for more information. (This message was added in version 6.4.) in /var/www/src/wp-includes/functions.php on line 6027

How it appears in the server logs:

[07-Sep-2023 17:53:09 UTC] PHP Notice: Function test_escaping_of_trigger_error was called &lt;strong&gt;incorrectly&lt;/strong&gt;. A non-empty string is required for blah. Please see &lt;a href=&quot;​https://wordpress.org/documentation/article/debugging-in-wordpress/&quot;&gt;Debugging in WordPress&lt;/a&gt; for more information. (This message was added in version 6.4.) in /var/www/src/wp-includes/functions.php on line 6027

Which escaping approach?

A different escaping approach than esc_html() is needed. Which one?

Maybe wp_kses_post()? @flixos90 @azaozz @costdev what do you think?

[hellofromTonya]

In 56542:

Code Modernization: Use wp_trigger_error() in WP_List_Table magic methods.

Replaces trigger_error() with wp_trigger_error() in each of the WP_List_Table magic methods.

[56349] added the dynamic properties deprecation messages to the __get(), __set(), __isset(), __unset() magic methods. Since that commit, wp_trigger_error() was introduced (see [56530]) as a wrapper for trigger_error().

Follow-up to [56349], [56356], [56530].

See #58896, #57686.

[hellofromTonya]

In 56543:

Code Modernization: Use wp_trigger_error() in WP_User_Query magic methods.

Replaces trigger_error() with wp_trigger_error() in each of the WP_User_Query magic methods.

[56353] added the dynamic properties deprecation messages to the __get(), __set(), __isset(), __unset() magic methods. Since that commit, wp_trigger_error() was introduced (see [56530]) as a wrapper for trigger_error().

Follow-up to [56353], [56530].

See #58897, #57686.

[hellofromTonya]

In 56544:

Code Modernization: Use wp_trigger_error() in WP_Text_Diff_Renderer_Table magic methods.

Replaces trigger_error() with wp_trigger_error() in each of the WP_Text_Diff_Renderer_Table magic methods.

[56354] added the dynamic properties deprecation messages to the __get(), __set(), __isset(), __unset() magic methods. Since that commit, wp_trigger_error() was introduced (see [56530]) as a wrapper for trigger_error().

Follow-up to [56354], [56530].

See #58898, #57686.

[flixos90]

@hellofromTonya Regarding the escaping, my 2 cents is that we should avoid escaping in these functions entirely, for the following reasons:

• None of these functions have historically escaped their messages, and given this was never raised as a problem (as far as I'm aware) I'm not convinced now is a good reason to start auto-escaping.
• Automatically escaping, as great as it would be if possible, is unreliable, as we can tell from this alone. Escaping is not a "one size fits all" thing: Sometimes it is about escaping all HTML (esc_html()), while other times it is about stripping all HTML except a certain set of tags or attributes (see wp_kses()).
• Other related functions like wp_die() do not automatically escape either, instead developers should escape the content that they pass to it as needed.
• trigger_error() does not escape what is passed to it, so why should wp_trigger_error() do so? I think that mixes up responsibilities, in addition to the above problems.

While it would be great if somehow WordPress core could automatically escape anything anywhere, in reality this is something that is responsibility of the individual developer since in most cases it can't be centrally handled. We already have WordPress coding standards that flag any missing escaping, and I think we should continue to rely on those.

So I think we should remove the esc_html() call from wp_trigger_error().

[azaozz]

Replying to hellofromTonya:

Hmm, esc_html() for the message being passed to trigger_error() is not the right approach.

Maybe can just document it that the string is expected to be HTML escaped (or "safe")?

None of these functions have historically escaped their messages, and given this was never raised as a problem

Sure, but the PHP manual warns otherwise :)
Thinking that documenting that the function requires an escaped/HTML safe string would be enough?

[costdev]

Maybe can just document it that the string is expected to be HTML escaped (or "safe")?

That sounds good to me. 🙂

[hellofromTonya]

Maybe can just document it that the string is expected to be HTML escaped (or "safe")?

I agree. As ​previously noted in original PR (threaded comments):

None of the existing trigger_error() instances in Core use esc_html() including _doing_it_wrong() and _deprecated_*() functions.... The error messages are intended for the server logs.
trigger_error() is for hardcoded PHP error/warning/notice/deprecation messages. I don't think they need escaping.

Within Core itself, the instances are hardcode and thus trusted. I agree with @flixos90 that the responsibility then lies with any customized code that is dynamically creating the messages (i.e. non-hardcoded messages).

Adding that information into the function's docblock can provide guidance.

~PR coming.~ PR ​https://github.com/WordPress/wordpress-develop/pull/5175

wp_trigger_error():

Removes the message escaping and documents the message is expected to be escaped for HTML.

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

[hellofromTonya]

​PR 5175 removes the message escaping and documents the incoming message should be escaped for browser output before invoking wp_trigger_error().

[peterwilsoncc]

If WP is to document that the data should be escaped for potential output, then I think the function should simply escape the message. It's one of those instances in which WP can protect both developers and users from developer mistakes, so those of us working on this ticket should take it.

Escaping the message can lead to some messy messages in log files but I think it's safer to presume that the message will be displayed. As the message may contain user data then it's best to treat it unsafe.

Ideally PHP would escape for debug_display but not for log files but unfortunately that's not the case.

[hellofromTonya]

Escaping the message can lead to some messy messages in log files but I think it's safer to presume that the message will be displayed. As the message may contain user data then it's best to treat it unsafe.

The messy messages would be in the browser and log files. Users, extenders, and contributors would all experience significantly less readable and less understandable messages.

IMO the messy messages are significantly less readable and understandable. They will confuse users, extenders, and contributors, potentially causing more investigative and debug efforts.

Escaping the messages impacts all of Core's messages too, including all _deprecated_*() and _doing_it_wrong() messages. But all of these messages are hardcoded, not dynamic. IMO Core itself should be trusted and its messages should be readable.

I do understand your point @peterwilsoncc: to ensure non-Core messages are made safe by default within Core. But I think it comes with too hefty of side-effect, especially for a potential unknown edge case of a non-escaped dynamic message that might have nefarious HTML in it.

I think the approach taken (i.e. to document extenders need to escape any of their messages that have non-hardcoded content) fits well with the intent of trigger_error().

I'll mark ​PR 5175 for commit. But please let's keep discussing. If there's consensus to go a different direction, then a follow-up commit can happen.

[hellofromTonya]

To escape or not escape trigger_error() messages:

IMO the discussion of escaping trigger_error() messages is beyond the scope of this ticket.

Why? Core does not and has not escaped messages or parts of a message passed to its instances of trigger_error(). This includes in the _deprecated_*() functions or _doing_it_wrong(). Thus a change here in this ticket impacts those messages.

If there's a desire to continue the escaping discussion, I'd suggest opening a new Trac ticket. BTW I do think it's a good discussion, one to have.

[peterwilsoncc]

User input without escaping

[peterwilsoncc]

User input with escaping

[peterwilsoncc]

Usesr input with double escaping.

[peterwilsoncc]

The messy messages would be in the browser and log files. Users, extenders, and contributors would all experience significantly less readable and less understandable messages.

This is incorrect, with escaping & even double escaping the displayed message is clearer in the browser when using the default PHP implementation for displaying errors. Without escaping the message doesn't display what was input.

With xdebug enabled, you are correct, the messages are double escaped but WP can't assume that xdebug is running on production sites.

IMO the discussion of escaping trigger_error() messages is beyond the scope of this ticket.

Why? Core does not and has not escaped messages or parts of a message passed to its instances of trigger_error(). This includes in the _deprecated_*() functions or _doing_it_wrong(). Thus a change here in this ticket impacts those messages.

Again I disagree, without escaping those of us on this ticket are choosing to introduce a cross-site-scripting vector. As WP handles double-escaping if an extender is doing the right thing & following the advice in the proposed docblock, then there is no effect on the display.

Had a hardening ticket being raised for the other functions then I think escaping would have been added without hesitation. This new function is an opportunity for WP to stop doing_it_wrong.

This is a ​gist of the mini-plugin I was using to generate the images, it was running with ​PR#5175 checked out.

[hellofromTonya]

_doing_it_wrong() with and without escaping the trigger_error() message.

[hellofromTonya]

@peterwilsoncc _doing_it_wrong_example.png​ is an example in the browser of _doing_it_wrong() with and without escaping the message passed to trigger_error(). Notice that with escaping, the linked article is no longer selectable. It looks like this in the server log:

[13-Sep-2023 15:58:32 UTC] PHP Notice:  Function test_escaping_of_trigger_error was called <strong>incorrectly</strong>. A non-empty string is required for blah. Please see <a href="https://wordpress.org/documentation/article/debugging-in-wordpress/">Debugging in WordPress</a> for more information. (This message was added in version 6.4.) in /var/www/src/wp-includes/functions.php on line 6080

What do you think of how it displays when escaping both in the browser and logs? Do you think it's less readable? If yes, do you think it's problematic?

[hellofromTonya]

Looking at the balance between message hardening vs readability in a different way ...

​PR 5161 switches trigger_error() to wp_trigger_error() in _doing_it_wrong() and each of the _deprecated_*() functions (in wp-includes/functions.php.

What if those instances don't use wp_trigger_error()?
What if instead those instances escape the parts of the message that are incoming to each function, but don't escape the hardcoded additions to the message done in each function?

For example, consider _doing_it_wrong(). The incoming arguments, i.e. $function_name, $message, and $version, could be escaped individually as part of the final message composition. In this way, the added HTML is not escaped and thus readability is not impacted.

But all messages passed to wp_trigger_error() would be escaped.

Could this approach achieve the balance of hardening and readability?

@peterwilsoncc @azaozz @flixos90 @costdev what do you think?

[flixos90]

@hellofromTonya That approach could potentially work, though still escaping is not a one-size-fits-all, so we need to think through whether universally running esc_html() on all error messages makes sense.

How common is it to trigger errors/warnings that include e.g. a <strong> tag or something similar?

Fundamentally, for those concerns I am personally not convinced of a central escaping solution. If that made sense, then why don't we already have something like wp_echo( $str ) and disallow all usage of the native echo? I think it is partly because universal escaping does not satisfy the use-cases. Developers have to be able to pick between esc_html(), wp_kses() etc., for what suits best the kind of message they are using.

[costdev]

Could this approach achieve the balance of hardening and readability?

I believe so, provided that we choose the right escaping function. I don't believe esc_html() is the right one for these functions.

Core uses <code>, <strong> and <a> in _doing_it_wrong(), trigger_error() and similar.
​Plugins and ​Themes typically use <code>, <strong> and in some cases <br>.

We could either allow a limited number of tags with:

wp_kses(
    $message,
    array( 'code', 'strong', 'br', 'a' => array( 'href' ) ), // $allowed_html
    array( 'http', 'https' ) // $allowed_protocols
);

or discourage the use of HTML in these messages - though this would remove <a> included in both Core and extender calls, and the others which may be desired when displayed as a UI message to make things more readable than wrapping in ".

@flixos90 Do you think supporting <code>, <strong>, <a href>, <br>, and possibly <em> could work to cover the vast majority of cases?

[hellofromTonya]

Thanks everyone! Sorry for my scattered thoughts, i.e. more of a brainstorming session.

I want to point out something.

These functions (_doing_it_wrong(), wp_trigger_error(), _deprecated_*()) are developer and debugging tools. The message is not displayed anywhere unless WP_DEBUG is turned on. Thus, there should be no or little risk to users should a message contain something nefarious in it.

As a tool, I think it's okay to not escape the message and document the incoming arguments should be clean.

[azaozz]

Replying to hellofromTonya:

These functions (_doing_it_wrong(), wp_trigger_error(), _deprecated_*()) are developer and debugging tools. The message is not displayed anywhere unless WP_DEBUG is turned on.

Yep, thinking the same. These strings/messages are targeted at developers and are not (should not ever be) displayed in production. If they were targeted at users/for use in production I think they should have been escaped and made "safe" by all means. However thinking it would probably be enough to document the fact that these functions expect HTML safe/escaped strings as they generally fall under "developer tools" rather than "production code".

At the same time thinking that @peterwilsoncc thoughts and @costdev's approach above make sense too. Probably would be good to have another (hardening) ticket for it so it covers all similar cases.

[flixos90]

I think the approach @costdev suggested could be okay as a tradeoff between security and limiting behavior. By allowing a few tags and attributes commonly used in "inline" messages, we "escape" while still being a bit flexible.

The only drawback of that which I could see is if a developer uses these functions for a message like "The <div class="test"></div> tag has some problem.", in which case that HTML, which is a crucial part of the message, would be stripped. That is possibly an edge-case though. If we go with a wp_kses() based approach, we could still document that. In case the error message intentionally contains HTML that is part of the actual message, I would argue then it would be the developers responsibility anyway to encode this, even or particularly when it's being displayed somewhere.

I almost wish there was a function like wp_kses() but which doesn't strip other disallowed HTML and instead escapes it. But that's probably going too far. So I would be okay with the approach suggested by @costdev.

[peterwilsoncc]

What if those instances don't use wp_trigger_error()?
What if instead those instances escape the parts of the message that are incoming to each function, but don't escape the hardcoded additions to the message done in each function?

I think this might be a good compromise.

It handles existing Core patterns and allows contributors to figure out a solution to PHP's logging/displaying conflict at a later date.

Do you think supporting <code>, <strong>, <a href>, <br>, and possibly <em> could work to cover the vast majority of cases?

This could work as a compromise. KSES has a _deprecated_* and _doing_it_wrong call so it would probably need to be done in conjunction with the above approach.

These strings/messages are targeted at developers and are not (should not ever be) displayed in production. If they were targeted at users/for use in production I think they should have been escaped and made "safe" by all means.

I agree, they ought not be used in production, but there have been reports of it being the case in the past so my main motivation is for core to protect people from themselves.

Let us take a look at the KSES approach and see if it's good ("us" being the folks working on this ticket).

Implements the balance between security and readability:

• Replaces esc_html() with wp_kses() with a list of allowed HTML tags and protocols.
• Documents extenders need to first escape HTML that is disallowed before passing the message to the function. Else, the disallowed tags and protocols will be stripped from the message.

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

[hellofromTonya]

​https://github.com/WordPress/wordpress-develop/pull/5303 implements @costdev's approach to use wp_kses() and @flixos90 approach to document extenders need to escape disallowed HTML first before passing to the function.

​https://github.com/WordPress/wordpress-develop/pull/5161 replaces the trigger_error() instances with wp_trigger_error() in the _deprecated_*() and _doing_it_wrong() functions.

These 2 patches accomplish the goals of the discussions in this ticket and the consensus and compromises made.

I'll go ahead and commit ​https://github.com/WordPress/wordpress-develop/pull/5161.

@flixos90 @costdev can you please review ​https://github.com/WordPress/wordpress-develop/pull/5303 for commit ahead of today's Beta 1.

Thank you everyone!

Going with ​https://github.com/WordPress/wordpress-develop/pull/5303 instead.

[hellofromTonya]

In 56705:

General: Use wp_trigger_error() in _doing_it_wrong() and _deprecated_*().

Uses wp_trigger_error() in _doing_it_wrong() and each _deprecated_*() function, i.e. instead of trigger_error().

To avoid redundancy, uses wp_trigger_error() once. How? Saves each message to $message variable and then passes it to wp_trigger_error() at the end of the function.

Functions:

• _doing_it_wrong()
• _deprecated_function()
• _deprecated_constructor()
• _deprecated_class()
• _deprecated_file()
• _deprecated_argument()
• _deprecated_hook()

Follow-up to [56530].

Props azaozz, costdev, flixos90, hellofromTonya, peterwilsoncc.
See #57686.

Committed via https://core.trac.wordpress.org/changeset/56705.

[hellofromTonya]

In 56707:

General: Use wp_kses() in wp_trigger_error().

Uses wp_kses() instead of esc_html() to allow a list of HTML tags and protocols in the message rather than escaping them.

Why? To retain message readability in the browser and server logs, especially given that Core itself adds HTML to messages in functions, e.g. _doing_it_wrong() and each of the _deprecated_*() functions.

HTML tags allowed:

• a href
• br
• code
• em
• strong

Protocols allowed: http and https.

To inform extenders, it also documents that any other HTML tags or protocols need to be escaped before passing the message to this function to avoid them being stripped from the message.

Follow-up to [56530], [56705].

Props azaozz, costdev, flixos90, hellofromTonya, peterwilsoncc.
Fixes #57686.

Committed via https://core.trac.wordpress.org/changeset/56707.

Closing in favor of a wrapper approach in ​https://github.com/WordPress/wordpress-develop/pull/5122, that was committed instead.

[hellofromTonya]

Thank you everyone! I think this ticket is complete.

I've opened #59455 for the ongoing initiative to replace all trigger_error() instances with wp_trigger_error(). It's a blessed task in the spirit of other similar initiatives in each release, such as #58833, #58976, #58955, etc.

[webcommsat]

@hellofromTonya is there a dev note for this using the [commit message]https://core.trac.wordpress.org/ticket/57686#comment:57.

Given that this information is also aimed at extenders and plugin authors would need to be aware of the allowed HTML tags, is this something for the email to plugin authors which is being collated in Marcomms? This dev note is being tracked in this [issue]​https://github.com/WordPress/Documentation-Issue-Tracker/issues/1150.

[westonruter]

In 58361:

General: Fix array format for allowed HTML passed into wp_kses() for wp_trigger_error().

Kses requires an associative array of allowed HTML.

See #57686. Follow-up to [56707].

Props thelovekesh, westonruter.
Fixes #61318.