[wp-trac] [WordPress Trac] #11474: Add validation error reporting system to Settings API
WordPress Trac
wp-trac at lists.automattic.com
Sat Aug 14 16:29:21 UTC 2010
#11474: Add validation error reporting system to Settings API
-------------------------------------+--------------------------------------
Reporter: jeremyclarke | Owner: jeremyclarke
Type: enhancement | Status: reopened
Priority: high | Milestone: 3.0
Component: Administration | Version: 2.9
Severity: normal | Resolution:
Keywords: has-patch needs-testing |
-------------------------------------+--------------------------------------
Changes (by westi):
* version: 3.0.1 => 2.9
Old description:
> == Problem ==
> As discussed in this wp-hackers email thread -
> http://groups.google.com/group/wp-
> hackers/browse_thread/thread/1d70363875bd2e32 - the Settings API (which
> is used to add settings sections and fields to admin settings pages
> elegantly and automatically) has a distinct lack of ability to return
> useful information to users about any validation errors that occurred
> during the validation callback function given in
> {{{register_setting()}}}.
>
> The main problem is that the validation occurs on a seperate pageload
> from the final return to your settings page that the user sees (it
> happens on {{{options.php}}}, the target of forms used in the Settings
> API, when update_option() is called on the registered setting and thus
> its validation filter is run). This means that there is no way for a
> developer using the Settings API to designate error messages for the user
> from within the validation callback, because any GLOBALS, objects or
> other variables set during that function will be destroyed when the user
> is forwarded back to the original page. This forwarding, which causes the
> message 'Settings Saved' to be shown, regardless of how validation went,
> happens in {{{options.php}}} with the following code:
>
> {{{
> $goback = add_query_arg( 'updated', 'true', wp_get_referer() );
> wp_redirect( $goback );
> }}}
>
> == Importance ==
>
> This missing feature is also a huge defect of the Settings API, both
> because it gives no sane way for eager developers to add errors to their
> own settings sections/pages, but also because it fails to inspire them to
> do so with an elegant framework. IMHO error reporting to users should be
> a first-class citizen of the Settings API, and pushing error reporting in
> the faces of developers will cause an overall usability improvement for
> '''all''' WordPress plugins, not just those who's authors were already
> concerned about reporting validation errors.
>
> Put another way: Usability improvements to the Settings API through easy
> error reporting should be treated like security improvements through
> mandatory validation functions: Both are functionalities that
> remind/inspire developers to do what they should be doing anyway, but
> often forget about/don't think to do on their own.
>
> The lack of error reporting also exposes itself within the default admin
> options in WordPress, where no error reporting is done on the core
> options when they are invalid. The admin_email option in Settings >
> General is a perfect example. If you give it an invalid email address (no
> @ sign) it silently removes the entire email address during validation,
> then shows a 'Settings Saved' message on the resulting screen, along with
> an empty admin_email field. This is a very dangerous situation to say the
> least. Like most other core admin options a message about why validation
> failed is vital, but without an API way to show these errors it is
> essentially impossible for core settings to output errors. If my solution
> below is added to core outputting an error for admin_email will involve a
> 1-line patch to its validation.
>
> == Temporary Solution Until 3.0 ==
>
> This should definitely be added by the time 3.0 arrives but that probably
> won't be for a long time from now (dec17-09). Until then it's still very
> important that error messaging be possible while using the Settings API.
>
> With the help of Ade Walker and Otto on wp-hackers I worked out an
> interim solution using a special field inside your option array that you
> check and remove while on your settings page after the processing has
> occurred. It is most assuredly a hack and not ideal, but it is a good
> interim solution for those who want to start using the Settings API but
> don't want to give up messages. If my solution outlined below is accepted
> it should be easy to convert from the hack to the API once its ready.
>
> Code exposing the interim solution is available in this pastie.org paste:
> http://pastie.org/747439
>
> It should also be attached to this ticket as
> settings_validation_errors_interim_hack.txt
>
> == Solution ==
>
> This needs a solution regardless of whether I'm the one to think of it,
> but here is an idea.
>
> === Functions Outline ===
>
> {{{
> // Add errors to a global array while validating options
> add_settings_error( $setting, $name, $message, $type = 'error' );
>
> // Fetch Settings errors that have been defined on the current pageload
> get_settings_errors( $setting = '' );
>
> // Display settings errors saved during the previous pageload
> settings_errors( $setting = '' );
> }}}
>
> === Specifics ===
>
> A function can be added to the API with the express purpose of defining
> errors to show to the user when they are returned to the settings page.
> It should have the ability to handle multiple errors and preferably also
> have the ability to handle failure AND success messages, so that plugins
> can do all their messaging from inside the validation function (which
> inspires further use of validation functions even outside other elements
> of the Settings API involved in pages).
>
> Something like:
>
> {{{
> add_settings_error( $setting, $name, $message, $type = 'error' );
> }}}
> * '''$setting''' The id of the setting the error is related to.
> * '''$name''' A unique identifier for the error message like
> 'admin_email_fail' (used in a css class on the message and in the
> resulting messages array). Might be unnecessary but seems like a good
> idea to include.
> * '''$message''' The actual message text to show
> * '''$type''' One of: error, success, warning etc (ideally it would map
> directly to the CSS classes used on error messages).
>
> Alternately the function could be given a more general name, though I
> think it might be less obvious what its for and 'error' might work better
> despite its mild innacuracy:
>
> {{{
> add_settings_message( $setting, $name, $message, $type = 'error' );
> }}}
>
> Either way this function would save the message/error array to a global
> variable comparable to {{{$wp_settings_fields}}} (which holds registered
> settings fields), maybe {{{$wp_settings_errors}}} (or again,
> {{{$wp_settings_messages}}}).
>
> A complementary function would be used to fetch the errors from the
> global:
>
> {{{
> get_settings_errors( $setting = '' );
> }}}
> * '''$setting''' An optional setting id in case you want to fetch only
> errors/messages related to one setting.
>
> ==== Passing the data back ====
>
> The {{{get_settings_errors()}}} function would then be called in
> {{{options.php}}} just before the redirection occurs after saving
> options, and if errors were present it would use that data for the
> redirect rather than just 'updated', 'true'. Something like:
>
> {{{
> if ($errors = get_settings_errors()) {
> $goback = add_query_arg( 'settings_errors', $errors,
> wp_get_referer() );
> } else {
> $goback = add_query_arg( 'updated', 'true', wp_get_referer() );
> }
> wp_redirect( $goback );
> }}}
>
> Acutally looking at it now it seems like this won't work. We can't really
> pass back an array using add_query_arg(). This implies we might need to
> use some database storage for the settings, maybe {{{set_transient()}}},
> that we could save the $errors to, then show them from on the next
> pageload.
>
> I don't know what would be the best ways to do this, please advise. Is
> there some way we could pass back the array directly and more simply than
> using the database?
>
> ==== Without using Database: message registration (awkward) ====
> One non-database solution would be to specifically register each error
> message with another function like {{{register_settings_error($setting,
> $error_name, $error_text)}}}. Then when redirecting back to the settings
> page we could return a list of comma-separated '''$error_name'''s with
> {{{add_query_arg()}}} that could later be shown with something like
> {{{settings_error($setting, $error_name)}}}.
>
> This strikes me as overkill though, and doubles the number of functions
> people need to learn to use the messaging system. If we can avoid
> 'message registration' then I think we should as developers will only
> have to know {{{add_settings_error()}}} to make use of it. If others
> think message registration is a good idea though then I'm into it, as it
> would make things even cleaner and more organized in exchange for the
> increased complexity.
>
> ==== DB-Based solution: set_transient('settings_errors') ====
>
> The problem with using the database to store the errors sent back from
> options.php is that you could end up with situations where, for whatever
> reason, the messages are not shown to the user right away (e.g. option
> was saved automatically elsewhere in admin or theme), and end up being
> displayed later when the user returns to the settings page but hasn't
> submitted anything.
>
> Thus we'd need to link the saved errors with the subsequent pageload
> explicitly. Maybe 'nonces' could solve it by verifying that the fields
> were just submitted, I'm not hip on nonces (I learned the Settings API to
> avoid them!) so let me know if that would be best.
>
> Based on a quick look at uses of {{{delete_transient()}}} in core,
> transients seem to be commonly used for tasks like this, including
> reporting on updated plugins and core updates across pageloads. A
> transient based solution could go like this, replacing the current logic
> in {{{options.php}}} that I pasted at the top:
>
> {{{
> if ($errors = get_settings_errors()) {
> // Save the $errors into a transient with a short expiration
> set_transient('settings_errors', $errors, 60);
> // Forward back to the settings page with a 'settings_errors'
> flag
> $goback = add_query_arg( 'settings_errors', 'true',
> wp_get_referer() );
> } else {
> $goback = add_query_arg( 'updated', 'true', wp_get_referer() );
> }
> wp_redirect( $goback );
> }}}
>
> If we did it this way then upon return to the original settings page we'd
> have access to {{{$_GET['settings_errors']}}}. If it is 'true', we'd know
> that there were fresh errors to show stored in the 'settings_errors'
> transient. It would also ensure that only that subsequent pageload would
> show those particular errors and the transient would quickly expire and
> become inactive/replaced by other settings manipulations. Just in case we
> would also delete the transient after displaying the errors.
>
> ==== Displaying the errors ====
>
> The last missing piece would be a display function to output the errors
> in standardized WP format (in case it ever changes from the current
> {{{<div class="error">}}} system):
>
> {{{
> settings_errors( $setting = '' );
> }}}
> * '''$setting''' Optionally limit the displayed errors to only those of
> a specific option id (not used in my imagined core system but a useful
> addition for people using the function elsewhere)
>
> I propose this name as comparable to {{{settings_fields($settings)}}},
> which needs to be called inside settings pages that use the Settings API,
> other ideas for the name of this function are welcome.
>
> {{{settings_errors()}}} would run {{{get_transient('settings_errors')}}}
> to fetch any current settings errors, then display them one by one using
> the current html and css for admin errors. It would use the '''$type'''
> argument from '''add_settings_error()''' to style the message
> appropriately. After display it would delete any values in the
> 'settings_errors' transient.
>
> The idea would be to use this during the '''admin_notices''' hook, or
> after the 'updated'='true' checking done in in {{{/wp-admin/options-
> head.php}}}. Something like:
>
> {{{
> if (isset($_GET['settings_errors']))
> settings_errors();
> }}}
>
> This would mean that developers using the API would have their messages
> automatically displayed at the top of their settings page, and would have
> done so with only one function call that was conveniently done in the
> midst of their option validation.
>
> == Conclusion ==
>
> HOO-AH! This is one damn long ticket. Maybe I should have just coded it
> first but IMHO the work is mostly done by now and maybe it was easier
> with writing than coding. Either way, please give me some feedback on
> this and let me know if I'm way off course. A patch will probably be easy
> enough to put together that I'll actually do it, if not hopefully I gave
> enough detail for someone to take care of it before feature freeze on
> 3.0.
New description:
== Problem ==
As discussed in this wp-hackers email thread -
http://groups.google.com/group/wp-
hackers/browse_thread/thread/1d70363875bd2e32 - the Settings API (which is
used to add settings sections and fields to admin settings pages elegantly
and automatically) has a distinct lack of ability to return useful
information to users about any validation errors that occurred during the
validation callback function given in {{{register_setting()}}}.
The main problem is that the validation occurs on a seperate pageload from
the final return to your settings page that the user sees (it happens on
{{{options.php}}}, the target of forms used in the Settings API, when
update_option() is called on the registered setting and thus its
validation filter is run). This means that there is no way for a developer
using the Settings API to designate error messages for the user from
within the validation callback, because any GLOBALS, objects or other
variables set during that function will be destroyed when the user is
forwarded back to the original page. This forwarding, which causes the
message 'Settings Saved' to be shown, regardless of how validation went,
happens in {{{options.php}}} with the following code:
{{{
$goback = add_query_arg( 'updated', 'true', wp_get_referer() );
wp_redirect( $goback );
}}}
== Importance ==
This missing feature is also a huge defect of the Settings API, both
because it gives no sane way for eager developers to add errors to their
own settings sections/pages, but also because it fails to inspire them to
do so with an elegant framework. IMHO error reporting to users should be a
first-class citizen of the Settings API, and pushing error reporting in
the faces of developers will cause an overall usability improvement for
'''all''' WordPress plugins, not just those who's authors were already
concerned about reporting validation errors.
Put another way: Usability improvements to the Settings API through easy
error reporting should be treated like security improvements through
mandatory validation functions: Both are functionalities that
remind/inspire developers to do what they should be doing anyway, but
often forget about/don't think to do on their own.
The lack of error reporting also exposes itself within the default admin
options in WordPress, where no error reporting is done on the core options
when they are invalid. The admin_email option in Settings > General is a
perfect example. If you give it an invalid email address (no @ sign) it
silently removes the entire email address during validation, then shows a
'Settings Saved' message on the resulting screen, along with an empty
admin_email field. This is a very dangerous situation to say the least.
Like most other core admin options a message about why validation failed
is vital, but without an API way to show these errors it is essentially
impossible for core settings to output errors. If my solution below is
added to core outputting an error for admin_email will involve a 1-line
patch to its validation.
== Temporary Solution Until 3.0 ==
This should definitely be added by the time 3.0 arrives but that probably
won't be for a long time from now (dec17-09). Until then it's still very
important that error messaging be possible while using the Settings API.
With the help of Ade Walker and Otto on wp-hackers I worked out an interim
solution using a special field inside your option array that you check and
remove while on your settings page after the processing has occurred. It
is most assuredly a hack and not ideal, but it is a good interim solution
for those who want to start using the Settings API but don't want to give
up messages. If my solution outlined below is accepted it should be easy
to convert from the hack to the API once its ready.
Code exposing the interim solution is available in this pastie.org paste:
http://pastie.org/747439
It should also be attached to this ticket as
settings_validation_errors_interim_hack.txt
== Solution ==
This needs a solution regardless of whether I'm the one to think of it,
but here is an idea.
=== Functions Outline ===
{{{
// Add errors to a global array while validating options
add_settings_error( $setting, $name, $message, $type = 'error' );
// Fetch Settings errors that have been defined on the current pageload
get_settings_errors( $setting = '' );
// Display settings errors saved during the previous pageload
settings_errors( $setting = '' );
}}}
=== Specifics ===
A function can be added to the API with the express purpose of defining
errors to show to the user when they are returned to the settings page. It
should have the ability to handle multiple errors and preferably also have
the ability to handle failure AND success messages, so that plugins can do
all their messaging from inside the validation function (which inspires
further use of validation functions even outside other elements of the
Settings API involved in pages).
Something like:
{{{
add_settings_error( $setting, $name, $message, $type = 'error' );
}}}
* '''$setting''' The id of the setting the error is related to.
* '''$name''' A unique identifier for the error message like
'admin_email_fail' (used in a css class on the message and in the
resulting messages array). Might be unnecessary but seems like a good idea
to include.
* '''$message''' The actual message text to show
* '''$type''' One of: error, success, warning etc (ideally it would map
directly to the CSS classes used on error messages).
Alternately the function could be given a more general name, though I
think it might be less obvious what its for and 'error' might work better
despite its mild innacuracy:
{{{
add_settings_message( $setting, $name, $message, $type = 'error' );
}}}
Either way this function would save the message/error array to a global
variable comparable to {{{$wp_settings_fields}}} (which holds registered
settings fields), maybe {{{$wp_settings_errors}}} (or again,
{{{$wp_settings_messages}}}).
A complementary function would be used to fetch the errors from the
global:
{{{
get_settings_errors( $setting = '' );
}}}
* '''$setting''' An optional setting id in case you want to fetch only
errors/messages related to one setting.
==== Passing the data back ====
The {{{get_settings_errors()}}} function would then be called in
{{{options.php}}} just before the redirection occurs after saving options,
and if errors were present it would use that data for the redirect rather
than just 'updated', 'true'. Something like:
{{{
if ($errors = get_settings_errors()) {
$goback = add_query_arg( 'settings_errors', $errors,
wp_get_referer() );
} else {
$goback = add_query_arg( 'updated', 'true', wp_get_referer() );
}
wp_redirect( $goback );
}}}
Acutally looking at it now it seems like this won't work. We can't really
pass back an array using add_query_arg(). This implies we might need to
use some database storage for the settings, maybe {{{set_transient()}}},
that we could save the $errors to, then show them from on the next
pageload.
I don't know what would be the best ways to do this, please advise. Is
there some way we could pass back the array directly and more simply than
using the database?
==== Without using Database: message registration (awkward) ====
One non-database solution would be to specifically register each error
message with another function like {{{register_settings_error($setting,
$error_name, $error_text)}}}. Then when redirecting back to the settings
page we could return a list of comma-separated '''$error_name'''s with
{{{add_query_arg()}}} that could later be shown with something like
{{{settings_error($setting, $error_name)}}}.
This strikes me as overkill though, and doubles the number of functions
people need to learn to use the messaging system. If we can avoid 'message
registration' then I think we should as developers will only have to know
{{{add_settings_error()}}} to make use of it. If others think message
registration is a good idea though then I'm into it, as it would make
things even cleaner and more organized in exchange for the increased
complexity.
==== DB-Based solution: set_transient('settings_errors') ====
The problem with using the database to store the errors sent back from
options.php is that you could end up with situations where, for whatever
reason, the messages are not shown to the user right away (e.g. option was
saved automatically elsewhere in admin or theme), and end up being
displayed later when the user returns to the settings page but hasn't
submitted anything.
Thus we'd need to link the saved errors with the subsequent pageload
explicitly. Maybe 'nonces' could solve it by verifying that the fields
were just submitted, I'm not hip on nonces (I learned the Settings API to
avoid them!) so let me know if that would be best.
Based on a quick look at uses of {{{delete_transient()}}} in core,
transients seem to be commonly used for tasks like this, including
reporting on updated plugins and core updates across pageloads. A
transient based solution could go like this, replacing the current logic
in {{{options.php}}} that I pasted at the top:
{{{
if ($errors = get_settings_errors()) {
// Save the $errors into a transient with a short expiration
set_transient('settings_errors', $errors, 60);
// Forward back to the settings page with a 'settings_errors' flag
$goback = add_query_arg( 'settings_errors', 'true',
wp_get_referer() );
} else {
$goback = add_query_arg( 'updated', 'true', wp_get_referer() );
}
wp_redirect( $goback );
}}}
If we did it this way then upon return to the original settings page we'd
have access to {{{$_GET['settings_errors']}}}. If it is 'true', we'd know
that there were fresh errors to show stored in the 'settings_errors'
transient. It would also ensure that only that subsequent pageload would
show those particular errors and the transient would quickly expire and
become inactive/replaced by other settings manipulations. Just in case we
would also delete the transient after displaying the errors.
==== Displaying the errors ====
The last missing piece would be a display function to output the errors in
standardized WP format (in case it ever changes from the current {{{<div
class="error">}}} system):
{{{
settings_errors( $setting = '' );
}}}
* '''$setting''' Optionally limit the displayed errors to only those of a
specific option id (not used in my imagined core system but a useful
addition for people using the function elsewhere)
I propose this name as comparable to {{{settings_fields($settings)}}},
which needs to be called inside settings pages that use the Settings API,
other ideas for the name of this function are welcome.
{{{settings_errors()}}} would run {{{get_transient('settings_errors')}}}
to fetch any current settings errors, then display them one by one using
the current html and css for admin errors. It would use the '''$type'''
argument from '''add_settings_error()''' to style the message
appropriately. After display it would delete any values in the
'settings_errors' transient.
The idea would be to use this during the '''admin_notices''' hook, or
after the 'updated'='true' checking done in in {{{/wp-admin/options-
head.php}}}. Something like:
{{{
if (isset($_GET['settings_errors']))
settings_errors();
}}}
This would mean that developers using the API would have their messages
automatically displayed at the top of their settings page, and would have
done so with only one function call that was conveniently done in the
midst of their option validation.
== Conclusion ==
HOO-AH! This is one damn long ticket. Maybe I should have just coded it
first but IMHO the work is mostly done by now and maybe it was easier with
writing than coding. Either way, please give me some feedback on this and
let me know if I'm way off course. A patch will probably be easy enough to
put together that I'll actually do it, if not hopefully I gave enough
detail for someone to take care of it before feature freeze on 3.0.
--
Comment:
Please don't re-open completed enhancements which have already shipped.
Please open a new ticket with your suggestions - you question is probably
best asked in the support forums first
--
Ticket URL: <http://core.trac.wordpress.org/ticket/11474#comment:19>
WordPress Trac <http://core.trac.wordpress.org/>
WordPress blogging software
More information about the wp-trac
mailing list