WordPress.org

Make WordPress Core

Opened 7 years ago

Last modified 4 months ago

#11515 new enhancement

Admin needs standardized way of handling messages (notices) displayed to the user

Reported by: filosofo Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version: 3.0
Component: Administration Keywords: error-message admin-messages
Focuses: Cc:

Description

If you try to upload a media item under Media > Add new without an uploads directory, you get the following vague error message:

Error saving media attachment.

If you try instead to upload from the post edit page, you get a much more helpful message:

Unable to create directory /path/wp-content/uploads. Is its parent directory writable by the server?

In each case, the root error is the same, but the second error message points the way to a solution. Even if the user doesn't know herself what that message means, it's a message that provides the necessary information to someone else who does and is trying to help the user. The first message is completely useless, as it states only what we already know: something went wrong.

The reason Media > Add new doesn't offer a helpful message is that the error is generated on one page request, and then the user is redirected to another page.

We need a standard, cross-page-load way of conveying messages in admin.

I've thought of a few possible ways of doing this:

  • Define and use a standardized set of error codes and associated error messages. This is similar to what happens currently on many pages: the unhelpful "Error saving media attachment." appears when the message argument is set to "3." What I'm suggesting would use a common set of message codes across the admin and be much more detailed. So the above situation would instead produce a message like "Unable to create the uploads directory."
  • Save error messages to a cookie. Unlike the previous method, this would allow messages to be made particular to their event.
  • Have some kind of user messaging stack. New messages would be pushed into a user's stack (stored in usermeta) and popped off after a certain time, or when read, etc. This has the advantage of lasting across sessions and browsers and being usable for other applications, such as PMs between users.

What do you think?

Change History (27)

#1 @nacin
7 years ago

I've been thinking about a way to standardize and improve handling of both error and updated messages for a while. I think it would help to develop some new API, instead of maintaining them on a page-by-page basis with ?message=# and various other solutions.

Mental note, error messages should consistently be red (.error), not yellow (.updated). We can also strip .fade from core, as the effect has been removed.

#2 @scribu
7 years ago

  • Type changed from defect (bug) to enhancement

+1 on the innitiative. Here's my 2c on the 3 proposals

1. A global array of error messages. 

Advantages: 
  - easy to implement, as it's the closest to the current practice

Drawbacks:
  - has to be loaded on each admin page
  - hard to maintain, once the list gets big

2. Cookie based messages

Advantages:
  - persistent enough

Drawbacks:
  - potential security issues? I have no ideea

3. Database stored messages

Advantages:
  - persistent

Drawbacks:
  - database clutter

Also, for number 3, having it also used for private messaging is not feasible, IMO.

So I would go with cookies on this.

#3 @hakre
7 years ago

I would like to see improvements here as well but I would first start to actually analyze why the situation is the situation we have. The overall concept is that errors are displayed on reference (you can provoke them with a get request w/o actually having an error) and the injection of the reference differs. Sometimes it's a numerical value only, sometimes it's a string value, sometimes those are multiple values which work as parameters in a error message.

I guess this was the most simple concept available and I ask myself: Isn't it the best concept? Why should we change something? If a message is not fitting well in the reporters sense, that certain location in the code can be improved. We do not need a new overall system here. Additionally the error messages aren't linked into a error message repository / online help (e.g. within code).

A first step would be to create a spreadsheet containing all backend errors with all it's parameters (if avail) and sources of error. With that it will be fairly much more easy to decide on new suggestions like the one by scribu.

#4 @hakre
7 years ago

There is code for handling error messages more structured in wp-admin/includes/class-wp-upgrader.php lines 848ff. Compare to function show_message() (file wp-admin/includes/misc.php line 261).

Exemplary code for another current method can be found in wp-admin/categories.php lines 128ff.

A combination of multiple methods can exemplary be found in wp-admin/users.php.

#5 @hakre
7 years ago

Related: #11517

#6 @hakre
7 years ago

Realted: #11207

#7 @hakre
7 years ago

I suggest that this tickets title should take into account that it is not only related to error but also to info messages. In Zend Framework those are called FlashMessenger or so if I remember correctly. Dunno wether this is a common wording or not. Maybe the implementation there is of interest.

Related: #11515

#8 @hakre
7 years ago

Correction: Related: #11402

#9 @ptahdunbar
7 years ago

  • Cc ptahdunbar added
  • Summary changed from Admin needs standardized way of handling error messages to Admin needs standardized way of handling messages (notices) displayed to the user

#10 @ptahdunbar
7 years ago

I've been dealing with this issue too when creating my own plugin messages. I've used a combination of what the WP_Upgrader class does + the $_GETmessage? global..

Here's an outline:

  • function that registers all message codes with their values
  • during form processing, add any feedback messages (like errors or success) to a $message array.
  • explode that array by commas to it can be sent back on wp_redirect once form processing is complete
  • a template function that parses the $message var and echos the actual message to the user.

One thing with preregistering messages is that you can only have generic messages. For instance, creating a new category would be "Category added." instead of a more personalized "The News category has been added". So variables can't be accessed when using the WP_Upgrader messaging approach. I'd really like to figure out a way to have a standardized way of displaying messages to the user with the ability to personalize the messages based on the results of form processing.

#11 @nacin
7 years ago

  • Milestone changed from 3.0 to Future Release

#12 @edward mindreantre
7 years ago

  • Cc edward mindreantre added

Excuse me for being a retard, but can't one use the $session array to store stuff?

We need a message api.

addErrorMessage(message);
addInfoMessage('The plugin has been activated.');
addErrorMessage(('Your post is too long!', PLUGINNAME));

if (!hasErrorMessages())

echo 'doing stuff';

else

echo displayMessages();

Something like that, eh?

#13 @williamsba1
7 years ago

  • Cc brad@… added

Running into this same issue on #11863. Need an easier way to display a notice when a permalink is changed because of a conflict.

#14 @koopersmith
7 years ago

  • Cc koopersmith added

#15 @scribu
7 years ago

  • Milestone changed from Future Release to 3.1

It seems the Settings API already has this. Check out add_settings_error().

It should be used on all settings pages.

#16 @nacin
6 years ago

  • Milestone changed from Awaiting Triage to Future Release

#17 @nvwd
4 years ago

  • Cc nowell@… added

#18 follow-ups: @obenland
4 years ago

I thought about this a little bit and prepared a pretty big patch, deprecating *_settings_error(), coming up with a way for a global approach to this, and updating all existing notices to follow the new API (which makes the patch fairly big).


Given the size of the patch, I thought I'd test the waters with small gist, outlining the essentials of what I had in mind:
https://gist.github.com/obenland/5173811


  • I chose the name "admin notice" because it is already fairly established and can be used globally in the admin, outside of just settings pages.
  • do_admin_notices() would run on every admin page load by default. That means that admin notices should not be added unless they are indeed supposed to be displayed. The global gets unset so it doesn't get saved on shutdown.
  • On shutdown, all "registered" (added) admin notices would be saved (either to the database or to a cookie), specific to the current user.
  • On init of the next page load, we check if any notices were saved, and if so, add them back to the global array of notices.
  • We would deprecate add_settings_error(), get_settings_errors(), and settings_errors().

Example for a use case where the admin notices would get saved:

<?php

/**
 * How it's currently done
 */
// See: http://core.trac.wordpress.org/browser/trunk/wp-admin/comment.php#L96
if ( !current_user_can( 'edit_comment', $comment->comment_ID ) ) {
        wp_redirect( admin_url('edit-comments.php?error=2') );
        die();
}

// Error handling in edit-comment.php
if ( isset( $_REQUEST['error'] ) ) {
        $error = (int) $_REQUEST['error'];
        $error_msg = '';
        switch ( $error ) {
                case 1 :
                        $error_msg = __( 'Oops, no comment with this ID.' );
                        break;
                case 2 :
                        $error_msg = __( 'You are not allowed to edit comments on this post.' );
                        break;
        }
        if ( $error_msg )
                add_admin_notice( 'moderated', $error_msg );
}


/**
 * How it could be done:
 */
// No error handling in edit-comments.php necessary.
if ( !current_user_can( 'edit_comment', $comment->comment_ID ) ) {
        add_admin_notice( 'edit-comments', __( 'You are not allowed to edit comments on this post.' ) );
        wp_redirect( admin_url( 'edit-comments.php' ) );
        die();
}

Last edited 3 years ago by obenland (previous) (diff)

#20 @iandunn
3 years ago

+1

It sucks for plugins to have to jump through hoops to make notices persist through the post/redirect/get process.

In some of my personal projects I've been doing essentially the same thing as Konstantin's patch. One minor tweak that I like is to group the types of messages into the same div, so that they stack cleaner. It looks better than having 2 separate divs when you end up with 2 errors on the same page.

One problem I ran into in my version was that duplicate messages could be added, so I had my enqueue() method only add new ones if an identical one didn't already exist. It's been awhile since I wrote that, but IIRC, the duplicate messages were often added by hook callbacks that were fired multiple times.

Making the notice expire after 30 seconds makes me feel a bit uneasy, although I can't think of a great use case for extending it. Perhaps if there's a redirect and the user's connection craps out, and it takes them a few minutes (or hours) to get back online; it'd be good if the notice was still waiting for them when they got back. Probably a pretty rare edge case, but it'd be easy to extend it and I don't think it'd hurt anything.

Last edited 3 years ago by iandunn (previous) (diff)

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


2 years ago

#22 in reply to: ↑ 18 @jdgrimes
22 months ago

Replying to obenland:

Given the size of the patch, I thought I'd test the waters with small gist, outlining the essentials of what I had in mind:
https://gist.github.com/obenland/5173811

I just want to note that there is a bug in the gist. The transient should be deleted only after the messages are displayed, not when they are retrieved. (See my revision.) Otherwise the list of messages might get retrieved and never displayed. For example, if there was an Ajax request, the messages would be deleted, but not dispalyed.

#23 follow-up: @obenland
22 months ago

Good point, but we could check for a valid request and only grab them then. On AJAX requests it would need to pull the messages anyway.

#24 in reply to: ↑ 23 @jdgrimes
22 months ago

Replying to obenland:

Good point, but we could check for a valid request and only grab them then. On AJAX requests it would need to pull the messages anyway.

Right, but I still think the messages shouldn't be deleted at that point. It's just unnecessarily more fragile that way.

#25 follow-up: @obenland
22 months ago

What would you suggest?

#26 in reply to: ↑ 25 @jdgrimes
22 months ago

Replying to obenland:

What would you suggest?

Oh, only deleting the transient after the messages have actually been displayed. (Sorry I wasn't clear.)

#27 in reply to: ↑ 18 @gschoppe
4 months ago

Replying to obenland:
I think using transients is the wrong way to go, because if multiple users are sharing an account, they can have race conditions that cause one user to receive another's notices.

I built out a persistent notice class based on using sessions to store the notices. I think it is a much more flexible option, moving forward. It's built as a drop-in solution, so that it can be included by themes and plugins now, with an eye toward future integration with core.

https://gist.github.com/gschoppe/dd7d12577974166529ea0139f1f53939

Note: See TracTickets for help on using tickets.