Create function to output admin notices

[joedolson]

There are at least 100 separate instances of admin notices generated by core. In a quick search for id="message", there are 92 hits, but I know that there are also cases that don't have that ID.

The markup for all of these should be generated by a function; this would make future updates easier, as well as making the markup easier to maintain.

Something like admin_notice( $message, $args );, where args is an array of class & ID attributes.

This introduces a new function, wp_admin_notice() to standardize admin notice output, and improve maintenance.

The function accepts the following parameters:

• string $message - The message for the admin notice.
• array $args - Optional. Arguments for the admin notice. Default empty array.
  • string $type - Optional. The type of the admin notice. e.g. 'success', 'warning', 'info'. Default empty string.
  • bool $dismissible - Optional. Whether the admin notice is dismissible. Default false.
  • string $id - Optional. The ID for the admin notice. Default empty string.
  • string[] $additional_classes - Optional. A string array of class names. Default empty array.
  • bool $paragraph_wrap - Optional. Whether to wrap the message in paragraph tags. Default true.

The function also has:

• Action: wp_admin_notice - Fires before an admin notice is output.
  • Passes (string) $output, (string) $message, (array) $args.
• Filter: wp_admin_notice_args - Filters the arguments for the admin notice.
  • Passes (array) $args, (string) $message.
• Filter: wp_admin_notice_output - Filters the output for the admin notice.
  • Passes (string) $output, (string) $message, (array) $args.

PHPUnit Tests are included and achieve 100% line and branch coverage.

https://core.trac.wordpress.org/ticket/57791

[costdev]

I've submitted ​PR 4119 with a proposed implementation and PHPUnit tests. It's ready for review 🙂

[sakibmd]

I have checked @costdev's PR and it's working great from my side. Thanks, @costdev

[dasnitesh780]

Thanks, @costdev your PR looks good, and thanks @joedolson for figuring it out.

[SergeyBiryukov]

Previously: #40173.

Related: #50486.

[joedolson]

#40173 is interesting; it shows that we already have code that can produce these structures with, but it's only available as part of a larger API for settings. But WordPress uses these notices more broadly. I don't think that closing that previous ticket as wontfix was the right move; instead, we should still create this function, and use it as the rendering portion in settings_errors().

This avoids the code duplication, but still allows us to have a function that renders the standardized structure of an admin notice without being tied to the settings API.

[costdev]

@joedolson This makes sense to me.

Implementing ​PR 4119's function in settings_errors() would replace:

<?php

$output = '';

foreach ( $settings_errors as $key => $details ) {
        if ( 'updated' === $details['type'] ) {
                $details['type'] = 'success';
        }

        if ( in_array( $details['type'], array( 'error', 'success', 'warning', 'info' ), true ) ) {
                $details['type'] = 'notice-' . $details['type'];
        }

        $css_id    = sprintf(
                'setting-error-%s',
                esc_attr( $details['code'] )
        );
        $css_class = sprintf(
                'notice %s settings-error is-dismissible',
                esc_attr( $details['type'] )
        );

        $output .= "<div id='$css_id' class='$css_class'> \n";
        $output .= "<p><strong>{$details['message']}</strong></p>";
        $output .= "</div> \n";
}

echo $output;

with:

<?php

foreach ( $settings_errors as $key => $details ) {
        wp_admin_notice(
                "<strong>{$details['message']}</strong>",
                array(
                        'type'               => $details['type'],
                        'dismissible'        => true,
                        'id'                 => 'setting-error-' . $details['code'],
                        'additional_classes' => array( 'settings-error' ),
                )
        );
}

Escaping is handled in wp_admin_notice().

We could then take the conditions for 'updated' === $details['type'] and the in_array() check into wp_admin_notice() for consistency throughout Core.

If that works for you, I can:

• Add the two conditions to wp_admin_notice().
• (local-only) Implement the above in settings_errors() to check if there's any BC breaks/test failures.
• Adjust wp_admin_notice(), or existing tests, accordingly if any BC breaks are found.

[costdev]

@joedolson When you have time, let me know if the above works for you and I'll update the PR 🙂

[joedolson]

The logic in your update doesn't totally match the original, since in the original the foreach loops over all messages and then echos the whole output, vs. outputting each message as it comes.

That said, I can't see any particular reason that would matter.

I also think that as part of adding this function, we should provide a returning and an echoing option, for future flexibility. This function would be quite useful for plugin authors, but I know that I wouldn't be able to use it for all notices if it only echoes. That probably means wp_get_admin_notice() would contain the logic you've provided and wp_admin_notice() would just call the getter.

But I definitely like getting rid of a bunch of string appending. :)

[sabernhardt]

The required fields indicator functions only return the content (ticket:54394#comment:37), so anyone who wants to display it could add echo instead of using another parameter. Would the same be appropriate here, or would this function specifically need a $display parameter?

Also related: #44082 addresses the paragraph and strong tags.

[costdev]

@joedolson The logic in your update doesn't totally match the original, since in the original the foreach loops over all messages and then echos the whole output, vs. outputting each message as it comes.
That said, I can't see any particular reason that would matter.

I noticed the above and also came to the same conclusion. 🙂

A quick check on the wp_get_admin_notice() vs wp_admin_notice() approach. The PR introduces a new action: wp_admin_notice.

With the return/output approach, this would either:

• Be fired when creating an admin notice, which may not be desired.
• Be fired only when wp_admin_notice() is called, which wouldn't catch all admin notices.

Example:
With the PR as it stands, implementing wp_admin_notice() in settings_errors() would fire the wp_admin_notice action X times. However, if settings_errors() instead used a wp_get_admin_notice() function, concatenating the results, then outputting, it wouldn't fire the wp_admin_notice action at all.

[costdev]

Since the new action is wp_admin_notice, it makes sense it only appears in that function, and a wp_get_admin_notice() function would contain filters to modify the $args and $markup for the notice.

@joedolson @sabernhardt With that in mind, what do you think of this?

<?php

function wp_admin_notice( $message, $args ) {
    do_action( 'wp_admin_notice', $message, $args );

    echo wp_get_admin_notice( $message, $args );
}

function wp_get_admin_notice( $message, $args ) {
    $defaults = array( '...' );
    $args     = wp_parse_args( $args, $defaults );
    $args     = apply_filters( 'wp_admin_notice_args', $args, $message );

    // Create $markup.

    return apply_filters( 'wp_admin_notice_markup', $markup, $message, $args );
}

In this case, settings_errors() would still use wp_admin_notice() so that the wp_admin_notice action is triggered for each notice.

[joedolson]

That makes sense to me; it ensures that both are fired, and it makes logical sense with the nature of action and filter behavior.

[costdev]

Thanks for the feedback @joedolson!

I've updated the PR to implement the above. Tests are included - some base tests for both, then separate tests for the filters/action, and examples are included in the description of ​PR 4119.

Do you think you'll get time to take a look at this ahead of Beta 1 next week and see if it's committable during the 6.3 cycle?

[joedolson]

I moved it to 6.4 a couple days ago; I think it's pretty ready, but it's going to take a while to get it put into use. I think this would be great to do early in the 6.4 cycle, but I don't think it's valuable to commit this code unless we're also ready to make all the changes to use it across core.

This implements the new wp_admin_notice() and wp_get_admin_notice() functions in multisite administration pages.

This PR is blocked and CI failures are expected until #4119 is committed.

[hellofromTonya]

I've removed the early keyword. Why?

​The handbook explains the keyword as:

This keyword should only be applied by committers. The keyword signals that the ticket is a priority, and should be handled early in the next release cycle.

Though it would be great to prepare the work for commit as early as possible, commits are constrained to only happen during the early part of 6.4 Alpha. Rather, commits can happen up to 6.4 Beta 1.

To aid in 6.4 tracking and scrubs, I've removed the early keyword.

[joedolson]

In 56408:

Administration: Add function to standardize admin notices.

Add functions wp_get_admin_notice() and wp_admin_notice() to create and output admin notices & tests for usage. New functions accept a message and array of optional arguments. This commit does not implement the functions. Include new filters: wp_admin_notice_args, wp_admin_notice_markup and action: wp_admin_notice.

Props joedolson, costdev, sakibmd, dasnitesh780, sabernhardt.
Fixes #57791.

[joedolson]

Reopening and turning into a task to start implementing new functions across core.

[joedolson]

In 56409:

Administration: Apply admin notice functions in multisite.

Use wp_get_admin_notice and wp_admin_notice to handle multisite settings notices.

Props costdev.
See #57791.

[joedolson]

In 56410:

Administration: Invalid argument passed in additional_classes.

Fix additional_classes argument passed as a string instead of an array. Follow up to [56409].

Props joedolson.
See #57791.

[joedolson]

Patch to extend function usage to nav-menus.php

In https://core.trac.wordpress.org/changeset/56409

In https://core.trac.wordpress.org/changeset/56408

[samiamnot]

Is there any thought to give the site owner some control of what and how various plugins can spam notifications? Right now that area is a mess. This brings a standardized way, but would ideally put the site owner in control of what they want to allow posted. Yes, I can uninstall a plugin that I feel abuses things, but giving site owners control will likely keep plugins in line and reduce the "spam" of alerts. Can you imagine a weekly admin notice from many plugins to upgrade to paid versions? Let the site owner mute etc.

[joedolson]

Hi, @samiamnot! I think you may have misunderstood what this change is doing; this is taking all of the existing WordPress admin notifications (e.g. "Your post is now published" "You have successfully deleted a comment", that sort of thing) and using a function to generate the markup for those functions instead of having each one printed individually to the screen.

It has no impact at all on what plugin authors can and can't do.

[johnbillion]

There's the WP Notify feature project for that: ​https://github.com/WordPress/wp-feature-notifications

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

[joedolson]

In 56518:

Administration: Use admin notice functions in nav menu admin.

Use wp_get_admin_notice and wp_admin_notice to handle settings notices on the nav menu admin screens.

Props joedolson.
See #57791.

PR for finishing applying wp_admin_notice functions across the admin. In progress.

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

The wp_get_admin_notice() and wp_admin_notice() functions were introduced in [56408].

This change implements these functions in wp-admin/includes/ files.

The wp_admin_notice() function was introduced in [56408].

This change implements this function in wp-includes/ files.

Contains changes to notices using the .updated pattern (And some .error)

Note: will probably update all of these again, to replace the .updated class with .notice-success and .error with .notice-error.

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

[joedolson]

In 56570:

Administration: Use wp_admin_notice() in /wp-admin/.

Add usages of wp_admin_notice() and wp_get_admin_notice() on .notice-[type] in the root level of /wp-admin/. Ongoing task to implement new function across core.

Props costdev, joedolson.
See #57791.

Committed in r56570.

[joedolson]

In 56571:

Administration: Use wp_admin_notice() in /wp-admin/includes.

Add usages of wp_admin_notice() and wp_get_admin_notice() on .notice-[type] in the root level of /wp-admin/includes. Ongoing task to implement new function across core.

Props costdev, joedolson.
See #57791.

In r56571

[joedolson]

In 56572:

Administration: Use wp_admin_notice() in /wp-includes/.

Add usages of wp_admin_notice() and wp_get_admin_notice() on .notice-[type] in the root level of /wp-includes/. Ongoing task to implement new function across core.

Props costdev, joedolson.
See #57791.

In r56572

[joedolson]

In 56573:

Administration: Use wp_admin_notice() for .updated.

Add usages of wp_admin_notice() and wp_get_admin_notice() on .updated in the root level of /wp-admin/. Ongoing task to implement new function across core.

Props costdev, joedolson.
See #57791.

In r56573

[joedolson]

Remaining todos:

• Instances of .error and .info classes
• Review of wp-includes context for uses of .error, .updated, .info` classes
• Testing .error, .updated, and .info classes to see if they can be replaced with .notice- class equivalents.

Contains changes to .error containers in /wp-admin/

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

[takayukister]

https://core.trac.wordpress.org/changeset/56573/trunk/src/wp-admin/edit-comments.php

Is this correct? => wp_admin()

[costdev]

In 56576:

Administration: Fix erroneous call to wp_admin().

In [56573], a typo caused wp_admin() to be called rather than wp_admin_notice().

This fixes the typo to correctly call wp_admin_notice().

Follow-up to [56573].

Props takayukister.
See #57791.

[costdev]

Nicely spotted @takayukister! Fixed in r56576.

[joedolson]

Must admit it was a bizarre experience to make that change in trunk, then realize I needed to revert and refresh, and it was still there when I ran svn up... ;)

Thanks, @takayukister!

[TobiasBg]

I'm now seeing

Warning: Undefined variable $updated_args in /var/www/html/wordpress/wp-admin/plugins.php on line 702

warnings on /wp-admin/plugins.php.

Seems to be caused by [56573], where the definition of the $updated_args array in that file is inside an else clause but then the variable is used in subsequent elseif clauses as well. This probably just needs to be moved before the if.

[joedolson]

Thanks for catching that, @TobiasBg! Will get that fixed shortly.

[joedolson]

In 56589:

Administration: Fix undeclared variable.

Moved $updated_notice_args to be defined before the conditional in which it's used.

Props TobiasBg.
See #57791.

[joedolson]

In 56590:

Administration: additional_classes is not a function.

Correct usage of additional_classes array parameter as if it were a function. Clearly, I'm getting tired and missing things. So many little changes. Follow up to [56573].

Props joedolson.
See #57791.

Adds further usages of wp_admin_notices() in the root level of /wp-includes/ on .error and .notice-info.

Ongoing task to implement new function across core.

Adds further usages of wp_admin_notices() in the root level of /wp-admin/includes/ on
.notice-error, .notice-warning, .error, and .updated.

Ongoing task to implement new function across core.

• Uses echo wp_get_admin_notice() because wp_kses_post removes template conditionals
• Adds function to output data because there are two usages in theme-install.php and the only difference is the notice class.
• Function will be easier to clone for use in other similar contexts.

Tested and works. Currently loses the padding in the error messages due to missing p wrapper; todo.

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

[costdev]

In 56597:

Administration: Increase wp_admin_notice() usage in /wp-includes/.

Adds further usages of wp_admin_notice() in the root level of /wp-includes/ on .error and .notice-info.

Ongoing task to implement new function across core.

Follow-up to [56408], [56409], [56410], [56518], [56570], [56571], [56572], [56573], [56576], [56589], [56590].

Props joedolson, costdev.
See #57791.

Committed in r56597.

[costdev]

In 56599:

Administration: Use wp_admin_notice() more in /wp-admin/includes/.

Adds further usages of wp_admin_notice() in /wp-admin/includes/ on .notice-error, .notice-warning, .error, and .updated.

Ongoing task to implement new function across core.

Follow-up to [56408], [56409], [56410], [56518], [56570], [56571], [56572], [56573], [56576], [56589], [56590], [56597].

Props joedolson, mukesh27, costdev.
See #57791.

[joedolson]

In 56600:

Administration: Use wp_admin_notice() more in wp-admin/.

Add additional usage of wp_admin_notice() in wp-admin/ on .error and miscellaneous usages previously overlooked.

Follow up to [56408], [56409], [56410], [56518], [56570], [56571], [56572], [56573], [56576], [56589], [56590], [56597], [56599].

Props costdev, joedolson.
See #57791.

In r56600

Add wp_admin_notice() in wp-admin/network

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

In r56599

Adds support for additional attributes to the wp_get_admin_notice() function & tests to review.

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

[joedolson]

In 56601:

Administration: Use wp_admin_notice() in wp-admin/network/.

Add additional usage of wp_admin_notice() in wp-admin/network/ where previously overlooked.

Follow up to [56408], [56409], [56410], [56518], [56570], [56571], [56572], [56573], [56576], [56589], [56590], [56597], [56599], [56600].

Props costdev, joedolson.
See #57791.

In r56601

Moves tabindex from the PHP output of notices to the JS handler for that notice. Allows us to avoid adding tabindex to the globally allowed attributes in KSES.

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

Both changes are tested and work the same as the original code as best as I can tell. These are the only instances I found where tabindex was set directly in the source.

[joedolson]

In 56602:

Administration: Move tabindex="-1" from notice to JS.

In a handful of admin notices, a tabindex attribute is set so that JS can move focus to the notice div. Rather than adding tabindex to globally accepted attributes for wp_kses_post(), move the assignment of tabindex into the JS handlers that display those notices. The attribute is only relevant if JS is running, so there is no reason to add it in the original HTML notice.

Follow up to [56408], [56409], [56410], [56518], [56570], [56571], [56572], [56573], [56576], [56589], [56590], [56597], [56599], [56600], [56601].

Props costdev, joedolson.
See #57791.

in r56602

[joedolson]

In 56603:

Administration: Add support for attributes in wp_admin_notice().

Allow admin notices to be created with additional attributes. Test attributes include hidden, data-*, and role="*" values, which are all in use in various admin notices across core.

This commit adds aria-live and hidden to the KSES global attributes array to support core usages.

Follow up to [56408], [56409], [56410], [56518], [56570], [56571], [56572], [56573], [56576], [56589], [56590], [56597], [56599], [56600], [56601], [56602].

Props costdev, joedolson.
See #57791.

In r56603.

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

[costdev]

In 56608:

Docs: Document aria-live and hidden in safecss_filter_attr().

In [56603], support was added for aria-live and hidden attributes in safecss_filter_attr().

This adds a @since annotation to document this change.

Follow-up to [56603].

Props mukesh27.
See #57791.

Thanks for the PR! Committed in changeset r56608.

[kebbet]

[56570] introduced the parameter $echo for the function wp_update_php_annotation. That naming is no longer used in core, should be $display. See this old commit for example: [54956].

Follow up to [56570].

https://core.trac.wordpress.org/ticket/57791

@kebbet, do we have any documentation regarding or any guidelines for this?

@kebbet, do we have any documentation regarding or any guidelines for this?

Not really atm, just looking at previous efforts, like https://core.trac.wordpress.org/changeset/54956 which was commited to his ticket https://core.trac.wordpress.org/ticket/56788

Thanks That changes was committed by @SergeyBiryukov so let's ping him for the clarification. Also i ping @costdev if hi knows anything about for it.

This is the ticket för 6.4 -> https://core.trac.wordpress.org/ticket/58976

[david.binda]

I have noticed that a success message on wp-admin/upload.php is displayed twice after r56573 . There seems to be duplicate call to wp_admin_notice. See https://core.trac.wordpress.org/browser/trunk/src/wp-admin/upload.php?rev=56590#L222 and https://core.trac.wordpress.org/browser/trunk/src/wp-admin/upload.php?rev=56590#L233

I'm attaching a patch for removing the duplicate call: https://core.trac.wordpress.org/attachment/ticket/57791/57791-wp-admin-upload-dupe.diff

I think that @SergeyBiryukov's commit message is pretty clear here; we're avoiding using PHP keywords in variable names. So I don't think we need any additional confirmation; this is good to go.

[joedolson]

In 56662:

Code Modernization: Rename reserved keyword used as variable.

Change the $echo parameter added to wp_update_php_annnotation() to $display to avoid using reserved PHP keywords as parameters. Follow up to [56570].

Props kebbet, mukesh27.
See #57791.

In r56662

[joedolson]

In 56663:

Administration: Remove duplicate wp_admin_notice() call.

Remove duplicate success message displayed in wp-admin/upload.php. Follow up to [56573].

Props davidbinda.
See #57791.

[joedolson]

Thanks for catching those, @davidbinda and @kebbet!