Opened 7 months ago
Last modified 3 days ago
#57791 reopened task (blessed)
Create function to output admin notices
Reported by: |
|
Owned by: |
|
---|---|---|---|
Milestone: | 6.4 | Priority: | normal |
Severity: | normal | Version: | |
Component: | Administration | Keywords: | has-patch has-unit-tests dev-feedback |
Focuses: | administration | Cc: |
Description
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.
Attachments (2)
Change History (96)
This ticket was mentioned in PR #4119 on WordPress/wordpress-develop by @costdev.
7 months ago
#2
- Keywords has-patch has-unit-tests added
#3
@
7 months ago
I've submitted PR 4119 with a proposed implementation and PHPUnit tests. It's ready for review 🙂
#4
@
7 months ago
I have checked @costdev's PR and it's working great from my side. Thanks, @costdev
#7
@
7 months ago
#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.
#8
@
7 months ago
@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.
#9
@
7 months ago
- Keywords dev-feedback added
@joedolson When you have time, let me know if the above works for you and I'll update the PR 🙂
#10
@
7 months ago
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. :)
#11
@
7 months ago
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.
#12
@
7 months ago
@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.
This ticket was mentioned in Slack in #accessibility by joedolson. View the logs.
6 months ago
#14
@
4 months ago
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.
#15
@
3 months ago
That makes sense to me; it ensures that both are fired, and it makes logical sense with the nature of action and filter behavior.
#17
@
3 months ago
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?
#18
@
3 months ago
- Keywords early added
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 ticket was mentioned in PR #4895 on WordPress/wordpress-develop by @costdev.
2 months ago
#20
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.
#21
@
6 weeks ago
- Keywords early removed
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.
This ticket was mentioned in Slack in #core by joedolson. View the logs.
6 weeks ago
#24
@
6 weeks ago
- Resolution fixed deleted
- Status changed from closed to reopened
- Type changed from enhancement to task (blessed)
Reopening and turning into a task to start implementing new functions across core.
@joedolson commented on PR #4895:
6 weeks ago
#27
@joedolson commented on PR #4119:
6 weeks ago
#28
#29
@
6 weeks ago
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.
#30
@
6 weeks ago
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.
#31
@
6 weeks ago
There's the WP Notify feature project for that: https://github.com/WordPress/wp-feature-notifications
This ticket was mentioned in Slack in #accessibility by joedolson. View the logs.
6 weeks ago
This ticket was mentioned in PR #5141 on WordPress/wordpress-develop by @joedolson.
4 weeks ago
#33
Trac ticket: https://core.trac.wordpress.org/ticket/57791
This ticket was mentioned in PR #5142 on WordPress/wordpress-develop by @joedolson.
4 weeks ago
#35
PR for finishing applying wp_admin_notice
functions across the admin. In progress.
Trac ticket: https://core.trac.wordpress.org/ticket/57791
This ticket was mentioned in PR #5149 on WordPress/wordpress-develop by @costdev.
4 weeks ago
#36
The wp_get_admin_notice()
and wp_admin_notice()
functions were introduced in [56408].
This change implements these functions in wp-admin/includes/
files.
This ticket was mentioned in PR #5150 on WordPress/wordpress-develop by @costdev.
4 weeks ago
#37
The wp_admin_notice()
function was introduced in [56408].
This change implements this function in wp-includes/
files.
This ticket was mentioned in PR #5179 on WordPress/wordpress-develop by @joedolson.
3 weeks ago
#38
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
This ticket was mentioned in Slack in #feature-notifications by bacoords. View the logs.
3 weeks ago
@joedolson commented on PR #5142:
2 weeks ago
#41
Committed in r56570.
@joedolson commented on PR #5149:
2 weeks ago
#43
In r56571
@joedolson commented on PR #5150:
2 weeks ago
#45
In r56572
@joedolson commented on PR #5179:
2 weeks ago
#47
In r56573
#48
@
2 weeks ago
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.
This ticket was mentioned in PR #5205 on WordPress/wordpress-develop by @joedolson.
2 weeks ago
#49
Contains changes to .error
containers in /wp-admin/
Trac ticket: https://core.trac.wordpress.org/ticket/57791
#50
in reply to:
↑ 46
@
2 weeks ago
https://core.trac.wordpress.org/changeset/56573/trunk/src/wp-admin/edit-comments.php
Is this correct? => wp_admin()
#53
@
2 weeks ago
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!
#54
@
2 weeks ago
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
.
This ticket was mentioned in Slack in #accessibility by joedolson. View the logs.
2 weeks ago
This ticket was mentioned in PR #5223 on WordPress/wordpress-develop by @costdev.
2 weeks ago
#59
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.
This ticket was mentioned in PR #5224 on WordPress/wordpress-develop by @costdev.
2 weeks ago
#60
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.
This ticket was mentioned in PR #5225 on WordPress/wordpress-develop by @joedolson.
2 weeks ago
#61
- 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
@mukesh27 commented on PR #5224:
2 weeks ago
#64
@joedolson commented on PR #5205:
2 weeks ago
#67
In r56600
This ticket was mentioned in PR #5230 on WordPress/wordpress-develop by @joedolson.
2 weeks ago
#68
Add wp_admin_notice()
in wp-admin/network
Trac ticket: https://core.trac.wordpress.org/ticket/57791
@joedolson commented on PR #5224:
2 weeks ago
#69
In r56599
This ticket was mentioned in PR #5232 on WordPress/wordpress-develop by @joedolson.
2 weeks ago
#70
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 commented on PR #5230:
13 days ago
#72
In r56601
This ticket was mentioned in PR #5233 on WordPress/wordpress-develop by @joedolson.
13 days ago
#73
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
@joedolson commented on PR #5233:
13 days ago
#74
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 commented on PR #5233:
13 days ago
#76
in r56602
@joedolson commented on PR #5232:
13 days ago
#78
In r56603.
This ticket was mentioned in PR #5236 on WordPress/wordpress-develop by @mukesh27.
13 days ago
#79
Trac ticket: https://core.trac.wordpress.org/ticket/57791
This ticket was mentioned in PR #5277 on WordPress/wordpress-develop by @kebbet.
9 days ago
#83
Follow up to [56570].
@mukesh27 commented on PR #5277:
9 days ago
#84
@kebbet, do we have any documentation regarding or any guidelines for this?
9 days ago
#85
@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
@mukesh27 commented on PR #5277:
9 days ago
#86
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.
9 days ago
#87
This is the ticket för 6.4 -> https://core.trac.wordpress.org/ticket/58976
#88
@
8 days ago
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
@joedolson commented on PR #5277:
8 days ago
#89
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 commented on PR #5277:
8 days ago
#91
In r56662
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:
wp_admin_notice
- Fires before an admin notice is output.(string) $output
,(string) $message
,(array) $args
.wp_admin_notice_args
- Filters the arguments for the admin notice.(array) $args
,(string) $message
.wp_admin_notice_output
- Filters the output for the admin notice.(string) $output
,(string) $message
,(array) $args
.PHPUnit Tests are included and achieve 100% line and branch coverage.
https://core.trac.wordpress.org/ticket/57791