[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