WordPress.org

Make WordPress Core

Opened 2 years ago

Last modified 14 months ago

#19691 new enhancement

Cannot modify admin messages for /wp-admin/edit.php

Reported by: mikeschinkel Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version: 3.3
Component: Administration Keywords: dev-feedback has-patch 2nd-opinion
Focuses: Cc:

Description

The admin console messages output on line 264 of WordPress 3.3's file /wp-admin/edit.php are not filterable. This causes problems when added row actions need to HTTP GET from to WordPress to modify a post and then display an appropriate message complete with a revert link (like the "Trash" link does.)

An example use-case could be for a custom post type used for both quotes and invoices where a row action might be "Convert Quote to Invoice" where you'd want a message and link displayed at the top of the admin after similar to this:

  • Quote #{$post_id} converted to Invoice. Revert

Currently the only way to accomplish this is to pick hooks before and after the messages are output and use PHP's output buffering; clearly not a "best practice" approach.

In order to address this I'm proposing an 'admin_messages' filter hook to run just before the messages are output:

$messages = apply_filters( 'admin_messages', $messages );

However, since messages are output in numerous locations in the WordPress admin it seemed best to add the hook in every location where messages are output, which is what my patch does. Thus a hook can look at $pagenow or get_current_screen() to decide it is needs to do anything.

Also while searching for places in the admin code that echo messages I found $messages are sometimes an array of HTML where the entire array is echoed and other times the $messages are an array with an index passed via $_GET and only one message will be displayed. For those cases I created another hook 'admin_message' (note that this hook name is singular):

$message = apply_filters( 'admin_message', $message, $messages, $_GET['message'] );

I really only found a specific need for /wp-admin/edit.php today, but it seemed that it would be better for consistency if all messages were made hook filterable. That's why I created a larger patch when all my use-case needs is one new line.

Looking forward to your feedback.

Attachments (2)

admin_messages_hook.diff (5.7 KB) - added by mikeschinkel 2 years ago.
Adds 'admin_messages' and 'admin_message' hooks.
admin_messages_hook-2.diff (6.6 KB) - added by mikeschinkel 2 years ago.
Adds 'admin_messages' hook.

Download all attachments as: .zip

Change History (9)

mikeschinkel2 years ago

Adds 'admin_messages' and 'admin_message' hooks.

comment:1 naomicbush2 years ago

  • Cc naomicbush added

comment:2 follow-up: mbijon2 years ago

  • Cc mike@… added

+1 to add admin_message as-is.

For admin_messages, I'm not sure that requiring plugin devs to test $page now or get_current_screen is a good idea. That's a very different requirement from most filters. Could we have a different hook per-location?


And, if we keep admin_messages like it is now, could it be renamed to more explicitly than just by pluralizing, maybe 'before_all_admin_messages' or 'before_multiple_admin_messages' ?

comment:3 in reply to: ↑ 2 mikeschinkel2 years ago

Replying to mbijon:

For admin_messages, I'm not sure that requiring plugin devs to test $page now or get_current_screen is a good idea. That's a very different requirement from most filters. Could we have a different hook per-location?

While I don't have strong feelings about using my patch as-is, I will disagree that requiring a $pagenow test is different from most filters. I just searched our main plugin code base and we have over 25 places where I had to test $pagenow in order to get the correct behavior.

That said, I don't prefer having to check $pagenow because it requires exception-based programming which is prone to side-effects and errors. One solution might be to use "{$pagenow}-admin_messages" but I'd hate to loose the generic "admin_messages" in case the use-case is not $pagenow specific.

And, if we keep admin_messages like it is now, could it be renamed to more explicitly than just by pluralizing, maybe 'before_all_admin_messages' or 'before_multiple_admin_messages' ?

Good point about making them easier to differentiate, although those hooks names are a bit long, no? How about:

  • admin_message_list and admin_message

Or

  • admin_messages and indexed_admin_message

Or

  • ...?

comment:4 follow-up: mbijon2 years ago

I actually like "{$pagenow}-admin_messages". That's better than making a unique hook per-location.

You're right about checking $pagenow being more common lately too. The generic name is probably simplest given how many hooks I can barely remember at times. Maybe we keep the generic one, but rename to 'before_admin_messages'.

comment:5 in reply to: ↑ 4 mikeschinkel2 years ago

  • Cc mikeschinkel@… added

Replying to mbijon:

I can barely remember at times. Maybe we keep the generic one, but rename to 'before_admin_messages'.

Hmm. Doesn't labeling a hook with 'before_*' imply that there is also an 'after_*'? Having that "before" indicator without an "after" would confuse me more than not.

When I made the patch I was only trying to tackle the plural use-case and threw in the singular use-case because I found it while searching code. Your comments have made me question the difference between the two use-cases (plural and singular) and the best I could come up with is that there is only an implementation difference, not a conceptual one.

So I've updated the patch to always use 'admin_messages', even for the singular case because it's easily conceivable someone might want to add a message to an area that currently only has single messages. Thus the patch converts all single messages to an array and passes thru the 'admin_messages' filter. This created one situation where we ideally need an 'error_messages' filter too, which the patch has too.

In reviewing all the message handling code in WordPress core's admin area it's painfully obvious that it should all be consolidated into a singular message handling function, but I won't tackle that without the core devs blessing first.

mikeschinkel2 years ago

Adds 'admin_messages' hook.

comment:6 mbijon2 years ago

I was considering suggesting an after filter too. I couldn't think of a good use-case though.

Getting rid of admin_message and your good points about too-long names makes admin_messages sound better.

+1 on this version

comment:7 Offereins14 months ago

+1

Can this come to 3.6? This is catching dust way too long.

Version 0, edited 14 months ago by Offereins (next)
Note: See TracTickets for help on using tickets.