Make WordPress Core

Opened 14 years ago

Closed 20 months ago

#12254 closed enhancement (maybelater)

Move show_message() into WP_Error class and add support for various WP_Error features that are missing

Reported by: jeremyclarke's profile jeremyclarke Owned by: valendesigns's profile valendesigns
Milestone: Priority: normal
Severity: normal Version:
Component: General Keywords: needs-patch
Focuses: Cc:

Description

Revisiting an old ticket about the mass upgrader (#11232) after working on my ticket for the Settings API (#11474) makes me think both cases are running into a wider problem related to the way WP_Error messages are handled system-wide. There just isn't a way to show them that fits with the detailed system that exists around logging errors with WP_Error. In both cases having a built-in way to display all messages logged in a WP_Error object would simplify their code and make their use of WP_Error much more logical and less haphazard.

WP_Error is intense

WP_Error objects can have an infinite number of messages logged inside them into $code groups which themselves can even have multiple message values/

 WP_Error::add($code, $message, $data = '')

You can then retrieve error messages for a specific code using WP_Error::get_error_messages()

show_message() is weak

In contrast to this useful maleability is the show_message() function used by various updater scripts to access WP_Error messages:

/**
 * {@internal Missing Short Description}}
 *
 * @since unknown
 *
 * @param unknown_type $message
 */
function show_message($message) {
	if( is_wp_error($message) ){
		if( $message->get_error_data() )
			$message = $message->get_error_message() . ': ' . $message->get_error_data();
		else
			$message = $message->get_error_message();
	}
	echo "<p>$message</p>\n";
}

Living in misc.php, this is clearly not the most loved function in WordPress, but beyond its lack of documentation and arguments it also actually doesn't make sense when combined with the nature of WP_Error, and I'd argue that it should be a first-class citizen of the WP_Error API.

Deal with the input as WP_Error class by default

For one thing its argument is named $message, which is a misnomer since it also supports complex WP_Error objects. I think it should be called $error and it should attempt to handle the error object in as much detail as possible. If this function is given a string instead of an error_object it should just show the text with the default formatting.

Move it into the class defintion

The function logic should also be moved to be inside WP_Error as ::show_message() as well as maybe adding a ::show_messages() to differentiate between forcing the first message in the object and showing any relevant messages. The original function can be kept as a shell for the class method.

Support $code and $data better

The updated function should allow $code values to be specified to show only messages related to a specific error $code (from WP_Error::add()) as well as some kind of option specifying whether the contents of the $data value for the results should be displayed.

Add support for 'types' of errors for display purposes

To really have valuable visual cues linked with error display the system needs to be adapted to handle error types like 'error', 'updated' and something green like 'success'. Having this be part of hte API would increase the likelihood of people using the system because it will make it easier to quickly add visual errors to a plugin. IMHO the easiest way to achieve this would be to link 'types' with CSS classes expected to exist in wp-admin. 'updated' and 'error' classes already exist, yellow and red respectively, and can be a starting point. This way marking an error as 'error' or 'updated' automatically controls the color it will be when show_messages() is run.

This would probably require modifying the WP_Error::wp_error() and WP_Error::add() methods to accept a 4th argument, but would be a great step forward.

Use nice formatting

I think the formatting should be the same as the 'settings updated' message from after you save a settings page. It is malleable and stands out in the admin:

<div class="updated fade"><p><strong> MESSAGE </strong></p></div>

Lets make showing errors easy

Even though these functionalities aren't needed for the current uses of show_message() in the upgrader process I think they will inform and improve those systems when the changes are taken into account. WP_Error is fairly powerful but not used enough because it is incomplete and awkward. Improving show_message() to fill this hole will mature the API and hopefully get plugin authors using it in more detail. It would definitely make integrating WP_Error into the Settings API much easier.

Thoughts on this before I make a patch?

I'll try to work on this soon but am interested in feedback. Anyone had this idea before and ran into a wall? Something else you think should be included?

Change History (20)

#1 @voyagerfan5761
14 years ago

  • Cc WordPress@… added

#2 @nacin
14 years ago

I'd decouple this from show_message(). Its only purpose is to echo out messages for the upgraders.

The only issue is that it's missing code to then flush the message while the upgrade process continues (which is the whole point of having it), but that is to be handled elsewhere.

#3 @jeremyclarke
14 years ago

@nacin: I don't think looking at show_message() as defined entirely by its current hacked-together use is productive or necessary. IMHO it was added as a convenience and without consideration of its position in the larger WP_Error ecosystem. I'm trying to fill in the gaps and figure out what it SHOULD be doing rather than what it does now (which is get in the way of fixing the updates system because it has no actual display configurability). It's title implies a generic utility rather than a specific one. If it is truly only for use in updaters it should be changed to show_updater_message() or something like that, though IMHO even such a function should call the new, more powerful show_message() function to generate output.

Message types and colors: In IRC today jane wells and I discussed the current colors/message types situation and how it could be improved. We discussed how currently there are only two kinds of messages:

  • updated - yellow
  • error - red


This fails to account for a lot of different kinds of problems that need to be shown in the UI. For example, how to state that the current situation is unnacceptable even when there was no submission (e.g. something missing).

The ideas we had for types fall into these rough groups, with their most logical color choices:

  • updated/confirmed - GREEN (currently yellow) - When a submission has gone well and changes were saved.
  • error - RED - When a submission somehow failed and reason for failure.
  • notice/warning - YELLOW - A message for the user about the current state of affairs regardless of whether a form was just submitted.

This would allow almost all messages to be presented usefully and semantically to users using show_message() along with the appropriate message type. On some pageloads there could be two related messages, one about the submission result and one about the current status, using updated/error for the submission result and notice/warning for the status message. (i.e. "ERROR: invalid email submitted. WARNING: No email currently saved")

Potential color inconsistency with plugins: PeteMall worried that changing the meaning of yellow from 'upated/confirmed' to 'warning/notice' would cause plugin conflicts, but IMHO this is a fairly safe compromise to make. The difference is subtle and no users will be freaked out by a yellow plugin. This is especially unthreatening compared with the lack of a consistent message/error taxonomy to guide UI development in the rest of WP core.

#4 @nacin
14 years ago

  • Milestone changed from 3.0 to Future Release

#5 follow-up: @jeremyclarke
14 years ago

  • Milestone changed from Future Release to 3.1

Setting the milestone to 3.1. I don't think now is the right time to get into it because of all the other new features, but will try to tackle it once 3.0 drops.

#6 @ntm
14 years ago

This enhancement is very important e.g. for plugin developers. There are a lot of threads in the WP.org forum which are related to the WP message "The plugin generated ... characters of unexpected output during activation. If you notice “headers already sent” messages, problems with syndication feeds or other issues, try deactivating or removing this plugin." and often these threads stay unresolved.

It is good to know that there is something wrong e.g. during the plugin activation but it would definitely be better know what is wrong.

#8 in reply to: ↑ 5 @mikeschinkel
14 years ago

Replying to jeremyclarke:

Setting the milestone to 3.1. I don't think now is the right time to get into it because of all the other new features, but will try to tackle it once 3.0 drops.

A BIG +1 on this. What you suggest is really needed. Right now my error messages are all implemented in craptastic ways because I didn't want to build my own error framework. It definitely would be better if the platform integrated this for me (and everyone else.)

#9 @mikeschinkel
14 years ago

  • Cc mikeschinkel@… added

#10 @nacin
13 years ago

  • Milestone changed from Awaiting Triage to Future Release

#11 @anonymized_6443559
13 years ago

  • Cc Ryan_B added

#12 follow-up: @johnbillion
13 years ago

  • Cc johnbillion@… added

#13 in reply to: ↑ 12 @simonsharks
13 years ago

  • Cc simon@… added

#14 @chriscct7
9 years ago

  • Focuses accessibility added
  • Keywords needs-patch dev-feedback added

#15 @joedolson
9 years ago

  • Focuses accessibility removed

#16 @chriscct7
9 years ago

I don't know why I tagged that. Sorry about that.

#17 @joedolson
9 years ago

Yeah, I figured it was just an error. :)

#18 @valendesigns
9 years ago

Hey Guys,

I would really like to take over responsibility for this issue and create a patch, and unit tests if necessary. Although, I don't have experience yet with unit test, I do want to learn and help make the current test we have better. This seems like as good of a place to start as any. However, before I begin... @nacin, do you have any other concerns or requirements for this ticket besides your previous comments? I'd like to make sure we're on the same page before I begin planing and coding this enhancement.

Cheers,
Derek

#19 @wonderboymusic
9 years ago

  • Keywords dev-feedback removed
  • Owner changed from jeremyclarke to valendesigns
  • Status changed from new to assigned

Take a swing, valendesigns

#20 @valendesigns
9 years ago

I forgot all about this one.

#21 @desrosj
20 months ago

  • Resolution set to maybelater
  • Status changed from assigned to closed

I'm going to close this one out as a maybelater.

The only place in core show_message() is used today is the upgrade process, and I think that WP_Error may still be "incomplete and awkward", but it has been the widely accepted standard for how to handle errors and error messages across Core and the general ecosystem for some time now.

Personally, I like that WP_Error does not have any presentation related aspects and feel that it should stay that way. [49115] introduced some improvements that made it easier to log more data about a certain error code, and to work with multiple WP_Error objects/merge multiple into one.

Discussion can always continue on a closed ticket, and this can always be reopened if factors change.

Note: See TracTickets for help on using tickets.