Skip to content

HTML API: Refactor wp_get_admin_notice() #13273

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
dmsnell wants to merge 4 commits into from
+37 −30
Closed

HTML API: Refactor wp_get_admin_notice() #13273

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d15c355
HTML API: Refactor `wp_get_admin_notice()` with the HTML API.
dmsnell Aug 26, 2026
7c17d6e
Update tests for HTML compliance.
dmsnell Aug 26, 2026
2a58ee4
Update wpAdminNotice tests for spec compliance.
dmsnell Aug 26, 2026
77b40f2
Update wpGetAdminNotice tests for spec compliance
dmsnell Aug 26, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 29 additions & 22 deletions src/wp-includes/functions.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9233,23 +9233,26 @@ function wp_get_admin_notice( $message, $args = array() ) {
* @param array $args The arguments for the admin notice.
* @param string $message The message for the admin notice.
*/
$args = apply_filters( 'wp_admin_notice_args', $args, $message );
$id = '';
$classes = 'notice';
$attributes = '';
$args = apply_filters( 'wp_admin_notice_args', $args, $message );

$wrap_with_p = false !== $args['paragraph_wrap'];
$wrap_opener = $wrap_with_p ? '<p>' : '';
$wrap_closer = $wrap_with_p ? '</p>' : '';
$html_builder = new WP_HTML_Tag_Processor( "<div class=\"notice\">{$wrap_opener}" );
$html_builder->next_token();

if ( is_string( $args['id'] ) ) {
$trimmed_id = trim( $args['id'] );

if ( '' !== $trimmed_id ) {
$id = 'id="' . $trimmed_id . '" ';
$html_builder->set_attribute( 'id', $trimmed_id );
}
}

if ( is_string( $args['type'] ) ) {
$type = trim( $args['type'] );

if ( str_contains( $type, ' ' ) ) {
if ( strlen( $type ) !== strcspn( $type, " \f\t\r\n" ) ) {

@aaronjorbin aaronjorbin Aug 27, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aside: This feels like it could be a good helper function. grep -rnE --include='*.php' "str_contains\(.*,[[:space:]]*' '[[:space:]]*\)" . found 5 additional places where we do str_contains on an empty string.

@dmsnell dmsnell Aug 27, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it’s an abstraction that has been sitting in the wing, so to speak. one thing we discovered was that we have to be attentive to where we apply the \f form feed character. in HTML-sourced strings it’s part of whitespace, but in CSS content it’s not.

this means it’s different for class names going into HTML vs. class names going into a block attribute as JSON.

wp_contains_html_whitespace() and wp_contains_css_whitespace() could do it, but then we also have wp_contains_linear_whitespace() from RFC specs…and I just don’t know if the abstractions are actually worth it, which is why I haven’t pushed any.

this is good thought; my comment here is mostly to share the context of what has kept it from being born yet.

@dmsnell dmsnell Aug 27, 2026 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aaronjorbin some context we have from the HTML API development, which I mostly maintain as practice these days, is that abstracting low-level text operations like this is a hefty operation which showed up surprisingly well in performance benchmarking for the HTML API. it’s not likely to be in the hot-path here, so the risk is lower, but there’s an outsized impact on abstracting these calls compared to languages whose compilers inline them, or languages with cheap function calls (PHP’s remain fairly expensive, for reasons I don’t understand)

_doing_it_wrong(
__FUNCTION__,
sprintf(
Expand All @@ -9262,36 +9265,40 @@ function wp_get_admin_notice( $message, $args = array() ) {
}

if ( '' !== $type ) {
$classes .= ' notice-' . $type;
$html_builder->add_class( "notice-{$type}" );
}
}

if ( true === $args['dismissible'] ) {
$classes .= ' is-dismissible';
$html_builder->add_class( 'is-dismissible' );
}

if ( is_array( $args['additional_classes'] ) && ! empty( $args['additional_classes'] ) ) {
$classes .= ' ' . implode( ' ', $args['additional_classes'] );
foreach ( $args['additional_classes'] as $class_name ) {
$html_builder->add_class( $class_name );

@aaronjorbin aaronjorbin Aug 27, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume that add_class won't have an issue if an empty string is passed to it? Before it would just mean extra white space.

@dmsnell dmsnell Aug 27, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah that’s a good point to consider inside the HTML API. we could call _doing_it_wrong() like we do with set_attribute(), but the operation is benign.

in fact, if no existing classes are on the item, it won’t even add the class attribute when an empty class name is passed.

php > require __DIR__ . '/src/wp-load.php';
php > $p = new WP_HTML_Tag_Processor( '<div>' );
php > $p->next_token();
php > var_dump( $p->add_class( '' ) );
bool(true)
php > var_dump( $p->get_updated_html() );
string(5) "<div>"
php > var_dump( $p->add_class( 'bar' ) );
bool(true)
php > var_dump( $p->get_updated_html() );
string(17) "<div class="bar">"
php > var_dump( $p->add_class( null ) );
bool(true)
php > var_dump( $p->get_updated_html() );
string(18) "<div class="bar ">"
php > var_dump( $p->add_class( [] ) );
PHP Warning:  Uncaught TypeError: Cannot access offset of type array on array in /Users/dmsnell/code/WordPress-develop/src/wp-includes/html-api/class-wp-html-tag-processor.php:4824
Stack trace:
#0 php shell code(1): WP_HTML_Tag_Processor->add_class(Array)
#1 {main}
  thrown in /Users/dmsnell/code/WordPress-develop/src/wp-includes/html-api/class-wp-html-tag-processor.php on line 4824

Warning: Uncaught TypeError: Cannot access offset of type array on array in /Users/dmsnell/code/WordPress-develop/src/wp-includes/html-api/class-wp-html-tag-processor.php:4824
Stack trace:
#0 php shell code(1): WP_HTML_Tag_Processor->add_class(Array)
#1 {main}
  thrown in /Users/dmsnell/code/WordPress-develop/src/wp-includes/html-api/class-wp-html-tag-processor.php on line 4824

some of these deep internals do pass non-strings at times when we expect them. we might add hardening, though I think that in this case we have a similar behavior as the legacy sprintf() had

}
}

if ( is_array( $args['attributes'] ) && ! empty( $args['attributes'] ) ) {
$attributes = '';
foreach ( $args['attributes'] as $attr => $val ) {
if ( is_bool( $val ) ) {
$attributes .= $val ? ' ' . $attr : '';
} elseif ( is_int( $attr ) ) {
$attributes .= ' ' . esc_attr( trim( $val ) );
} elseif ( $val ) {
$attributes .= ' ' . $attr . '="' . esc_attr( trim( $val ) ) . '"';
foreach ( $args['attributes'] as $name => $value ) {
if ( is_int( $name ) ) {
/*
* Boolean attributes may have been appended as numeric list items,
* for example, with `$args['attributes'][] = 'disabled'`. They should
* be recorded with the value serving as their name.
*/
$html_builder->set_attribute( $value, true );
} elseif ( true === $value ) {
$html_builder->set_attribute( $name, true );
} elseif ( false !== $value ) {
$html_builder->set_attribute( $name, trim( (string) $value ) );
}
}
}

if ( false !== $args['paragraph_wrap'] ) {
$message = "<p>$message</p>";
}

$markup = sprintf( '<div %1$sclass="%2$s"%3$s>%4$s</div>', $id, $classes, $attributes, $message );
$markup = $html_builder->get_updated_html();
$markup .= $message;
$markup .= "{$wrap_closer}</div>";

/**
* Filters the markup for an admin notice.
Expand Down
8 changes: 4 additions & 4 deletions tests/phpunit/tests/functions/wpAdminNotice.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ public function test_should_output_admin_notice( $message, $args, $expected ) {
wp_admin_notice( $message, $args );
$actual = ob_get_clean();

$this->assertSame( $expected, $actual );
$this->assertEqualHTML( $expected, $actual );
}

/**
Expand Down Expand Up @@ -154,21 +154,21 @@ public function data_should_output_admin_notice() {
'args' => array(
'type' => '"><script>alert("Howdy,admin!");</script>',
),
'expected' => '<div class="notice notice-">alert("Howdy,admin!");"&gt;<p>A notice with an unsafe type.</p></div>',
'expected' => '<div class="notice notice-&quot;><script>alert(&quot;Howdy,admin!&quot;);</script>"><p>A notice with an unsafe type.</p></div>',
),
'an unsafe ID' => array(
'message' => 'A notice with an unsafe ID.',
'args' => array(
'id' => '"><script>alert( "Howdy, admin!" );</script> <div class="notice',
),
'expected' => '<div id="">alert( "Howdy, admin!" ); <div class="notice"><p>A notice with an unsafe ID.</p></div>',
'expected' => '<div id="&quot;><script>alert( &quot;Howdy, admin!&quot; );</script> <div class=&quot;notice" class="notice"><p>A notice with an unsafe ID.</p></div>',
),
'unsafe additional classes' => array(
'message' => 'A notice with unsafe additional classes.',
'args' => array(
'additional_classes' => array( '"><script>alert( "Howdy, admin!" );</script> <div class="notice' ),
),
'expected' => '<div class="notice ">alert( "Howdy, admin!" ); <div class="notice"><p>A notice with unsafe additional classes.</p></div>',
'expected' => '<div class="notice &quot;><script>alert( &quot;Howdy, admin!&quot; );</script> <div class=&quot;notice"><p>A notice with unsafe additional classes.</p></div>',
),
'a type that is not a string' => array(
'message' => 'A notice with a type that is not a string.',
Expand Down
8 changes: 4 additions & 4 deletions tests/phpunit/tests/functions/wpGetAdminNotice.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ class Tests_Functions_WpGetAdminNotice extends WP_UnitTestCase {
* @param string $expected The expected admin notice markup.
*/
public function test_should_return_admin_notice( $message, $args, $expected ) {
$this->assertSame( $expected, wp_get_admin_notice( $message, $args ) );
$this->assertEqualHTML( $expected, wp_get_admin_notice( $message, $args ) );
}

/**
Expand Down Expand Up @@ -150,21 +150,21 @@ public function data_should_return_admin_notice() {
'args' => array(
'type' => '"><script>alert("Howdy,admin!");</script>',
),
'expected' => '<div class="notice notice-"><script>alert("Howdy,admin!");</script>"><p>A notice with an unsafe type.</p></div>',
'expected' => '<div class="notice notice-&quot;><script>alert(&quot;Howdy,admin!&quot;);</script>"><p>A notice with an unsafe type.</p></div>',
),
'an unsafe ID' => array(
'message' => 'A notice with an unsafe ID.',
'args' => array(
'id' => '"><script>alert( "Howdy, admin!" );</script> <div class="notice',
),
'expected' => '<div id=""><script>alert( "Howdy, admin!" );</script> <div class="notice" class="notice"><p>A notice with an unsafe ID.</p></div>',
'expected' => '<div id="&quot;><script>alert( &quot;Howdy, admin!&quot; );</script> <div class=&quot;notice" class="notice"><p>A notice with an unsafe ID.</p></div>',
),
'unsafe additional classes' => array(
'message' => 'A notice with unsafe additional classes.',
'args' => array(
'additional_classes' => array( '"><script>alert( "Howdy, admin!" );</script> <div class="notice' ),
),
'expected' => '<div class="notice "><script>alert( "Howdy, admin!" );</script> <div class="notice"><p>A notice with unsafe additional classes.</p></div>',
'expected' => '<div class="notice &quot;><script>alert( &quot;Howdy, admin!&quot; );</script> <div class=&quot;notice"><p>A notice with unsafe additional classes.</p></div>',
),
'a type that is not a string' => array(
'message' => 'A notice with a type that is not a string.',
Expand Down
Loading