Make WordPress Core

Opened 14 months ago

Closed 7 months ago

Last modified 3 months ago

#57791 closed task (blessed) (fixed)

Create function to output admin notices

Reported by: joedolson's profile joedolson Owned by: joedolson's profile joedolson
Milestone: 6.4 Priority: normal
Severity: normal Version:
Component: Administration Keywords: has-patch has-unit-tests add-to-field-guide has-dev-note
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)

57791-nav-menus.diff (2.1 KB) - added by joedolson 8 months ago.
Patch to extend function usage to nav-menus.php
57791-wp-admin-upload-dupe.diff (639 bytes) - added by david.binda 7 months ago.

Download all attachments as: .zip

Change History (110)

#1 @joedolson
14 months ago

  • Component changed from General to Administration

This ticket was mentioned in PR #4119 on WordPress/wordpress-develop by @costdev.


14 months ago
#2

  • Keywords has-patch has-unit-tests added

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

#3 @costdev
14 months ago

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

#4 @sakibmd
14 months ago

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

#5 @dasnitesh780
14 months ago

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

#6 @SergeyBiryukov
14 months ago

Previously: #40173.

Related: #50486.

#7 @joedolson
14 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 @costdev
14 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().

However, this means that separate admin notices are output, so we might want to consider either a (bool) $output = true parameter for wp_admin_notice(), or put the main logic in a separate wp_get_admin_notice() function.

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 a third parameter, or a wp_get_admin_notice() function (whichever you prefer).
  • 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.
Version 4, edited 14 months ago by costdev (previous) (next) (diff)

#9 @costdev
14 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 @joedolson
14 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 @sabernhardt
14 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 @costdev
14 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.


13 months ago

#14 @costdev
11 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 @joedolson
10 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.

#16 @joedolson
10 months ago

  • Milestone changed from Awaiting Review to 6.4

#17 @costdev
10 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 @joedolson
10 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.

#19 @joedolson
10 months ago

  • Owner set to joedolson
  • Status changed from new to accepted

This ticket was mentioned in PR #4895 on WordPress/wordpress-develop by @costdev.


9 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 @hellofromTonya
8 months 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.


8 months ago

#23 @joedolson
8 months ago

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

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.

#24 @joedolson
8 months 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.

#25 @joedolson
8 months ago

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.

#26 @joedolson
8 months ago

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
8 months ago

Patch to extend function usage to nav-menus.php

#29 @samiamnot
8 months 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 @joedolson
8 months 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 @johnbillion
8 months 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.


8 months ago

#34 @joedolson
8 months ago

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.

This ticket was mentioned in PR #5142 on WordPress/wordpress-develop by @joedolson.


8 months 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.


8 months 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.


8 months 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.


8 months 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.


8 months ago

#40 @joedolson
7 months ago

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.

@joedolson commented on PR #5142:


7 months ago
#41

Committed in r56570.

#42 @joedolson
7 months ago

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.

#44 @joedolson
7 months ago

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.

#46 follow-up: @joedolson
7 months ago

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.

#48 @joedolson
7 months 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.


7 months ago
#49

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

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

#51 @costdev
7 months ago

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.

#52 @costdev
7 months ago

Nicely spotted @takayukister! Fixed in r56576.

#53 @joedolson
7 months 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 @TobiasBg
7 months 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.

#55 @joedolson
7 months ago

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

#56 @joedolson
7 months ago

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.

#57 @joedolson
7 months ago

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.

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


7 months ago

This ticket was mentioned in PR #5223 on WordPress/wordpress-develop by @costdev.


7 months 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.


7 months 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.


7 months 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.

https://i0.wp.com/github.com/WordPress/wordpress-develop/assets/1686451/21502391-5e81-4152-b12f-21d982c02a6c

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

#62 @costdev
7 months ago

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.

@costdev commented on PR #5223:


7 months ago
#63

Committed in r56597.

@mukesh27 commented on PR #5224:


7 months ago
#64

https://i0.wp.com/github.com/WordPress/wordpress-develop/assets/10103365/48d079a6-4989-4b12-a8b1-95e0be6eb463

#65 @costdev
7 months ago

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.

#66 @joedolson
7 months ago

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.

This ticket was mentioned in PR #5230 on WordPress/wordpress-develop by @joedolson.


7 months ago
#68

Add wp_admin_notice() in wp-admin/network

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

This ticket was mentioned in PR #5232 on WordPress/wordpress-develop by @joedolson.


7 months 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

#71 @joedolson
7 months ago

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.

This ticket was mentioned in PR #5233 on WordPress/wordpress-develop by @joedolson.


7 months 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:


7 months 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.

#75 @joedolson
7 months ago

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.

#77 @joedolson
7 months ago

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.

@joedolson commented on PR #5232:


7 months ago
#78

In r56603.

#80 @costdev
7 months ago

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.

@costdev commented on PR #5236:


7 months ago
#81

Thanks for the PR! Committed in changeset r56608.

#82 @kebbet
7 months ago

[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].

@mukesh27 commented on PR #5277:


7 months ago
#84

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

@kebbet commented on PR #5277:


7 months 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:


7 months 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.

@kebbet commented on PR #5277:


7 months ago
#87

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

#88 @david.binda
7 months 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:


7 months 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.

#90 @joedolson
7 months ago

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.

#92 @joedolson
7 months ago

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.

#93 @joedolson
7 months ago

Thanks for catching those, @davidbinda and @kebbet!

This ticket was mentioned in Slack in #core-media by joedolson. View the logs.


7 months ago

#95 @joedolson
7 months ago

In 56800:

Administration: Add missing space in theme activation notices.

Restore missing space in two admin notices during theme activation.

Props shailu25, sergeybiryukov, mukesh27, hellofromtonya.
Fixes #59501. See #57791.

#96 @TobiasBg
7 months ago

With an incompatible PHP version (just set a plugin's "Requires PHP" header to e.g. 8.4), I'm now getting

Uncaught ArgumentCountError: 2 arguments are required, 1 given in /var/www/html/wordpress/wp-admin/includes/class-wp-plugins-list-table.php:1282

This is caused by the removal of the comma at the end of line 1285 in wp-admin/includes/class-wp-plugins-list-table.php, which was done in https://core.trac.wordpress.org/changeset/56599#file4, when only the HTML code should have been removed. printf() has only one argument then.

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


7 months ago

#98 @marybaum
7 months ago

This appears to be a monster of a ticket that is mostly done, with parts long since committed. @beapi and I wonder what yall would think of closing this, which is largely done but for the small tasks remaining?

He is opening specific tickets for those.

#99 @marybaum
7 months ago

  • Milestone changed from 6.4 to 6.5

Moving to 6.5 per @joedolson, who very kindly dropped by our little scrub to answer the question:

"The simple answer is that I don't think it should be finished in 6.4; there's still more remaining besides the current open PR, and what's left is mostly the weird outliers.

3:57
E.g., notices that are created in JS, notices that are parts of JS templates, notices that have do_action() calls inside the markup... :grimacing:
3:58
So I think those changes are more fragile, and better left until 6.5, and done during alpha so we can discover what weird things might happen as we change them."

#100 @costdev
7 months ago

In 56824:

Plugins: Fix broken sprintf() call in plugins list table.

In [56599], a sprintf() call was modified which resulted in an insufficient number of arguments.
This caused a Fatal Error when an incompatible plugin notice was displayed.

This fixes the sprintf() call.

Follow-up to [56599].

Props petitphp, TobiasBg, sabernhardt, mukesh27.
Fixes #59590. See #57791.

#101 @costdev
7 months ago

  • Keywords needs-dev-note add-to-field-guide added; dev-feedback removed
  • Milestone changed from 6.5 to 6.4
  • Resolution set to fixed
  • Status changed from reopened to closed

@marybaum Moving to 6.5

To make sure this ticket closes in the milestone in which its initial commits landed, let's close this one off for 6.4 and handle the remaining work in one or more 6.5 ticket(s).

Housekeeping: Removing dev-feedback.

#102 @webcommsat
6 months ago

Hello following up where this is for dev notes. @costdev and @joedolson do one of you already have a dev note in draft we can add to the documentation tracker. There has been amazing work on this ticket.

Using much of the wording from @joedolson above, my understanding is:
This change is taking all of the existing WordPress admin notifications (e.g. "Your post is now published", "You have successfully deleted a comment", and similar), 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 do.

There is an ongoing task to implement new function across core.

This was originally triggered by the number of separate instances of admin notices generated by core.

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.

#103 @costdev
6 months ago

Hey @webcommsat! I was thinking that the dev note would introduce extenders to the new wp_admin_notice() and wp_get_admin_notice() functions so that they too can benefit from the reduce the maintenance burden of maintaining HTML for their admin notices, and that the dev note might get a callout in the Field Guide?

This conservative search shows that a significant number of plugins can benefit from these two functions right away.

I don't have the dev note done yet but can work on it and have it ready next week. Does that work for you and the team?

#104 @webcommsat
6 months ago

Thanks @costdev for the quick reply. That sounds ideal in terms of focus. We really need the dev note and to have been reviewed on Monday to be able to feature it in the Field Guide. Is that possible please?

The dev note listing on the [documentation tracker]https://github.com/WordPress/Documentation-Issue-Tracker/issues/1161.

For info, I have added it to the callout list for the Field Guide - [tracking issue]https://github.com/WordPress/Documentation-Issue-Tracker/issues/1160.

Can I also suggest that it needs a mention in the email to plugin authors which is currently being drafted in the release-marcomms channel. @meher for reference

#105 @costdev
6 months ago

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

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


6 months ago

#107 @afercia
6 months ago

In [56603]

@joedolson Quick question, when you have a chance: why the test multiple attributes with "role", invalid, data-*, numeric, and boolean tests for a disabled attribute on a div element? That would be invalid HTML anyways. Instead of being allowed and tested for as 'expected', it should be prevented I guess?

#108 @joedolson
3 months ago

It is invalid HTML, but that function doesn't have any mechanism for testing whether an attribute is valid on a div, so the expected result is that it would be added.

This is testing the difference between how wp_get_admin_notice() handles this and how wp_admin_notice() handles it. wp_admin_notice() is run through wp_kses_post(), so invalid attributes are stripped then, but that's not true for wp_get_admin_notice().

Note: See TracTickets for help on using tickets.