Make WordPress Core

Opened 6 years ago

Closed 5 years ago

Last modified 5 years ago

#47580 closed defect (bug) (fixed)

Prevent output of invalid HTML in `_default_wp_die_handler()`

Reported by: jeremyfelt's profile jeremyfelt Owned by: audrasjb's profile audrasjb
Milestone: 5.3 Priority: normal
Severity: normal Version:
Component: Administration Keywords: has-patch has-dev-note
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 (2)

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

Download all attachments as: .zip

Change History (23)

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


6 years ago

#2 @afercia
6 years 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
6 years 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
6 years 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
6 years ago

  • Milestone changed from Future Release to 5.3

@dinhtungdu
6 years ago

Use div for wrapper instead of p

#6 @dinhtungdu
6 years 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.


6 years ago

#8 @audrasjb
6 years ago

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

#9 @audrasjb
6 years 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 :)

#10 @audrasjb
5 years ago

  • Keywords needs-dev-note added

#11 follow-up: @audrasjb
5 years ago

  • Keywords needs-refresh added

@dinhtungdu thanks for the patch, but after a full review, I think we should separate p and .wp-die-message in two CSS rules (even if they have the same CSS declarations) in src/wp-admin/css/common.css.

@dinhtungdu
5 years ago

#12 @dinhtungdu
5 years ago

  • Keywords dev-feedback added; needs-refresh removed

Thank @audrasjb, I updated the patch with your comment :)

#13 @audrasjb
5 years ago

  • Keywords dev-feedback removed

Thanks! 47580.2.diff is looking great to me 👍

This ticket was mentioned in Slack in #core by pento. View the logs.


5 years ago

#15 @kirasong
5 years ago

  • Resolution set to fixed
  • Status changed from accepted to closed

In 45909:

Administration: Output valid HTML when wp_die() is called.

To better support HTML and string calls to wp_die() without
outputting invalid HTML, wraps error messages in <div> rather than <p>.

Adds .wp-die-message CSS class for styling.

Props dinhtungdu, jeremyfelt, audrasjb, SergeyBiryukov, afercia, audrasjb, noisysocks.
Fixes #47580.

#16 in reply to: ↑ 11 ; follow-up: @SergeyBiryukov
5 years ago

Replying to audrasjb:

a full review, I think we should separate p and .wp-die-message in two CSS rules (even if they have the same CSS declarations) in src/wp-admin/css/common.css.

Just curious, what was the reason for separating the rules? It seems that would make it harder to keep them consistent in the future.

#17 in reply to: ↑ 16 ; follow-up: @dinhtungdu
5 years ago

Replying to SergeyBiryukov:

Replying to audrasjb:

a full review, I think we should separate p and .wp-die-message in two CSS rules (even if they have the same CSS declarations) in src/wp-admin/css/common.css.

Just curious, what was the reason for separating the rules? It seems that would make it harder to keep them consistent in the future.

IMO, we don't need to keep them consistent in the future, they are two separated elements. We have their CSS rules identical for now because I want to keep the style as it was. In the future, it's more likely that we'll change the style for .wp-die-message.

#18 in reply to: ↑ 17 ; follow-up: @SergeyBiryukov
5 years ago

Replying to dinhtungdu:

IMO, we don't need to keep them consistent in the future, they are two separated elements. We have their CSS rules identical for now because I want to keep the style as it was. In the future, it's more likely that we'll change the style for .wp-die-message.

It just seems inconsistent to me that they're combined in _default_wp_die_handler() styles, but separated in common.css. I would think it's better to reduce duplication for now and separate them in the future if needed.

#19 in reply to: ↑ 18 @dinhtungdu
5 years ago

Replying to SergeyBiryukov:

It just seems inconsistent to me that they're combined in _default_wp_die_handler() styles, but separated in common.css. I would think it's better to reduce duplication for now and separate them in the future if needed.

Yeah, it makes sense now :). What do you think @audrasjb ?

#20 @audrasjb
5 years ago

  • Keywords has-dev-note added; needs-dev-note removed

#21 @SergeyBiryukov
5 years ago

In 48082:

Administration: Combine the styles for <p> and .wp-die-message, instead of duplicating.

This brings some consistency with _default_wp_die_handler(), where they are already combined.

Follow-up to [45909].

See #47580.

Note: See TracTickets for help on using tickets.