WordPress.org

Make WordPress Core

Opened 2 months ago

Last modified 2 days ago

#47580 accepted defect (bug)

Prevent output of invalid HTML in `_default_wp_die_handler()`

Reported by: jeremyfelt Owned by: audrasjb
Milestone: 5.3 Priority: normal
Severity: normal Version:
Component: Administration Keywords: has-patch
Focuses: accessibility Cc:

Description

The default handler for wp_die() wraps (by default) error messages with a paragraph tag.

For a large number of wp_die() calls in WordPress, a plain text string is passed and the HTML displayed is valid: <p>This is an error message.</p>.

Also for a large number of wp_die() calls in WordPress, an HTML string is passed and the HTML displayed is invalid: <p><h1>You need a higher level of permission.</h1><p>Sorry, you are not allowed to manage terms in this taxonomy.</p></p>

It seems that both use cases are valid and should be supported. wp_die() already has $args that are passed to the handlers. It seems like we could add key there for whether the handler should wrap in its own HTML or display the message as is.

Supporting a new argument would make it possible to have a slightly more complex error message while also maintaining the simple output of general error messages.

Related #47413, which addresses the issue when filling out a comment.

Attachments (1)

47580.diff (1.6 KB) - added by dinhtungdu 5 days ago.
Use div for wrapper instead of p

Download all attachments as: .zip

Change History (10)

This ticket was mentioned in Slack in #accessibility by afercia. View the logs.


3 weeks ago

#2 @afercia
3 weeks ago

  • Component changed from General to Administration
  • Milestone changed from Awaiting Review to Future Release

Discussed during today's accessibility bug-scrub and agreed ensuring valid HTML output is always valuable. Moving to Future Release.

#3 in reply to: ↑ description @SergeyBiryukov
3 weeks ago

Replying to jeremyfelt:

wp_die() already has $args that are passed to the handlers. It seems like we could add key there for whether the handler should wrap in its own HTML or display the message as is.

A key in $args might be cleaner, but also seems a bit like an overkill. Maybe a simple strpos( $message, '<p>' ) check before adding the <p> tag would suffice here?

Another option suggested by @audrasjb is to use <div> instead of <p> to allow for nested elements.

#4 @afercia
2 weeks ago

One more example of invalid HTML:

https://core.trac.wordpress.org/browser/trunk/src/index.php?rev=45649&marks=33-55#L32

The passed string avoids to set the initial opening tag <p>, which is smart, but then forgets to not pass the last closing tag </p>. Actual result in the HTML: </p></p>.

#5 @SergeyBiryukov
2 weeks ago

  • Milestone changed from Future Release to 5.3

@dinhtungdu
5 days ago

Use div for wrapper instead of p

#6 @dinhtungdu
5 days ago

  • Keywords has-patch dev-feedback added; needs-patch removed

After considering checking for tags in the $message and replacing p by div, I think it's better to use div for the die message wrapper.

We can check if $message contains <p>, but it means that we need to check for other tags as well. h1 for example, can't be a child element of p. A full covered check can make the current function more complex.

On the other hand, replacing p by div doesn't increase the complexity of _default_wp_die_handler.

What I did:

This ticket was mentioned in Slack in #accessibility by audrasjb. View the logs.


3 days ago

#8 @audrasjb
3 days ago

  • Keywords needs-testing added
  • Owner set to audrasjb
  • Status changed from new to accepted

#9 @audrasjb
2 days ago

  • Keywords dev-feedback needs-testing removed

Good to go on my side. As said during the meeting, I like the <div> approach because it‘s more secure concerning backward compatibility :)

Note: See TracTickets for help on using tickets.