#34893 closed enhancement (fixed)
Improve Customizer setting validation model
Reported by: | westonruter | Owned by: | westonruter |
---|---|---|---|
Milestone: | 4.6 | Priority: | high |
Severity: | normal | Version: | 3.4 |
Component: | Customize | Keywords: | has-patch has-screenshots has-unit-tests commit |
Focuses: | javascript | Cc: |
Description (last modified by )
Customizer setting values need to be able to be validated and for any validation errors to block the Customizer from saving any setting until all are valid.
Settings in the Customizer rely on sanitization to ensure that only valid values get persisted to the database. The sanitization in the Customizer generally allows values to be passed through to be persisted and does not enforce validation that blocks the saving of the value. This is in large part because there is no standard facility for showing error messages relating to Customizer controls, and there is no standard method to raise validation errors. A Customizer setting can be blocked from being saved by returning null
from the WP_Customize_Setting::sanitize()
method (i.e. generally returned via customize_sanitize_{$setting_id}
). When this is done, however, the modified value completely disappears from the preview with no indication for why the value seems to be reset to the default.
In JavaScript there is the wp.customize.Setting.prototype.validate()
method that can be extended to return null
in the case where the value should be rejected, but again this does not provide a way to display a validation message: the user can be entering data but they just stop seeing the value making changes in the preview without any feedback. Even worse, if the JS validate
method actually manipulates the value to make it valid, there can be strange behavior in the UI as the user provides input, likely having to do with the two-way data binding of wp.customize.Element
instances.
Furthermore, if one setting is blocked from being saved by means of validation in the sanitization method, the other settings in the Customizer state may very well be completely valid, and thus they would save successfully. The result is that some settings would get saved, whereas others would not, and the user wouldn't know which were successful and which failed (again, since there is no standard mechanism for showing validation error message). The Customizer state would only partially get persisted to the database. This isn't good.
We need to improve the Customizer by:
- Validating settings on server before save.
- Displaying validation error messages from server and from JS client.
- Performing transactional/atomic setting saving, rejecting all settings if one is invalid.
Since the WP_Customize_Setting::sanitize()
already partially supports validation by returning null
, we can build on that to also allow it to return a WP_Error
object. We'd want to retain the regular fuzzy sanitization during preview updates, but during the customize_save
action we'd want to impose a strict pass/fail for setting validation.
This is closely related and complimentary to #30937: Add Customizer transactions
For the notification bar to display validation error messages, see #35210.
Feature plugin: https://github.com/xwp/wp-customize-setting-validation
Attachments (23)
Change History (93)
#1
@
9 years ago
- Keywords has-patch needs-testing added; needs-patch removed
- Milestone changed from Future Release to 4.5
- Owner set to westonruter
- Status changed from new to accepted
This ticket was mentioned in Slack in #core-customize by westonruter. View the logs.
9 years ago
#4
@
9 years ago
Instead of overloading the Customizer setting's sanitize_callback
to do both sanitization and validation, for a Core patch the WP_Customizer_Setting
should really also implement a validate_callback
, just as has been done for the REST API (see WP_REST_Request::has_valid_params()
). There can be a default validate_callback
that checks if the sanitize_callback
returns null
and if so, then it can return false
. Otherwise, it can return true
. The REST API also supports the validate_callback
returning a WP_Error
, so our validate_callback
in WP_Customize_Setting
can do the same, something like 34893.0.diff.
Note the addition of a new $strict
argument to the customize_sanitize
filter which allows the filter to do strict/validation checking. I'm not 100% sold on this, but it would be nice if there could be some way to more easily integrate the sanitization/validation logic.
Maybe the REST API folks have some insights here :). The goal here is to keep the Customizer API as close to the REST API, so that Customizer settings can be just added to the REST API and it will just work.
/cc @rmccue @rachelbaker @danielbachhuber @joehoyle
#5
follow-up:
↓ 6
@
9 years ago
I agree on wanting to split the validate / sanitization. Validate can return true
/ false
and optionally a WP_Error to provide extra validation feedback to the user. sanitize
is used to mutate the value to the correct datatype, be it a safe string, bool, int etc.
I'd imagine you could just call the validate_callback
in isolation also, for example, checking an inputs validity on keyup / blur.
@rmccue and myself have discussed for the REST API on Save, whether the validate callback should be called on the value pre or post sanitize. I think there's merit to both, depending on the use case, we never came to a hard decision on that, but core currently calls sanitize before validate. This means the validator function can deal with "clean" data, and is concerned with specific validity rather than type checks. For this way round, the example of posts_per_page is a good one: sanitize_callback
calls absint
on the data passed via POST, and the validate_callback
would then > 0 && < 100
.
#6
in reply to:
↑ 5
@
9 years ago
Replying to joehoyle:
@rmccue and myself have discussed for the REST API on Save, whether the validate callback should be called on the value pre or post sanitize. I think there's merit to both, depending on the use case, we never came to a hard decision on that, but core currently calls sanitize before validate. This means the validator function can deal with "clean" data, and is concerned with specific validity rather than type checks. For this way round, the example of posts_per_page is a good one:
sanitize_callback
callsabsint
on the data passed via POST, and thevalidate_callback
would then> 0 && < 100
.
What about if the original raw unsanitized value is passed into the validator along with the sanitized value? This would allow the validator to have the flexibility to determine whether or not the sanitized value can be considered “good enough” for persisting to the DB, or else it can do a strict check to fail it simply if $sanitized_value !== $unsanitized_value
.
In that regard, the patch for this ticket could be modified to pass in both the $sanitized_value
and the $unsanitized_value
to the customize_validate_{$setting_id}
filter. I've amended my patch to implement that: 34893.1.diff. It also adds:
- Adds a
$strict
second parameter toWP_Customize_Setting::save()
, which is a signal thatWP_Error
can be returned - Validate all settings before the
customize_save
action, and return with a JSON error with an array of all the invalid settings if any are invalid.
For the REST API, however, it doesn't look like the original/raw/unsanitized value would still be available for the validate callback to look at?
#7
@
9 years ago
I'm keeping an eye on this for use in the Fields API, I could see this as super useful there as well, albeit the usage would be different outside of the Customizer -- I'd love to see what it could allow for.
#8
@
9 years ago
Another improvement is needed on the JS side of things. Namely, there is currently a wp.customize.Setting.prototype.validate()
method allows you to sanitize a value and also to block the setting update if you pass back null
to block the Setting
(really, the Value
). So really, the validate
method in JS is serving in the same purpose as the sanitize
method in PHP. They both conflate sanitization and validation.
I suggest that we extend wp.customize.Setting
to introduce a sanitize
method for the purpose of sanitization, and that the existing validate
method be extended to allow for the returning of an Error
object, which would have the same effect for blocking the value but it could also then emit an event on the Setting
object, which could then be picked up by any controls to then display a validation error message.
#9
@
9 years ago
See demo video of the feature plugin: https://www.youtube.com/watch?v=ZNk6FhtS8TM
This ticket was mentioned in Slack in #core-fields by sc0ttkclark. View the logs.
9 years ago
This ticket was mentioned in Slack in #core by westonruter. View the logs.
9 years ago
This ticket was mentioned in Slack in #core by westonruter. View the logs.
9 years ago
This ticket was mentioned in Slack in #core by westonruter. View the logs.
9 years ago
This ticket was mentioned in Slack in #core-customize by celloexpressions. View the logs.
9 years ago
#20
@
9 years ago
- Keywords 4.6-early removed
The error message that appears in the control should use the same interface as the global notifications. For example, they should both have instances of a NotificationCollection
. See https://core.trac.wordpress.org/ticket/35210#comment:60
This ticket was mentioned in Slack in #core by westonruter. View the logs.
9 years ago
#22
@
9 years ago
Please see related post: https://make.wordpress.org/core/2016/05/04/improving-setting-validation-in-the-customizer/
This ticket was mentioned in Slack in #core by westonruter. View the logs.
9 years ago
#24
@
9 years ago
- Keywords needs-unit-tests added
Please see 34893.2.diff.
- Introduces
wp.customize.Notification
which can representWP_Error
instances returned from the server. - Introduces
wp.customize.Control.prototype.notifications
. - Introduces
wp.customize.Setting.prototype.notifications
. - Injects a notification area into existing controls which is populated in response to the control's
notifications
collection changing, where this collection is synced with thenotifications
collections for each of its settings. - Attempting to save settings that are invalid will result in the save being blocked entirely, with the errors being sent in the
customize_save_response
for the controls to display notifications for, and the first such invalid control will be focused. - Introduces
wp.customize.settingConstructor
, allowing custom setting types to be used in the same way that custom controls, panels, and sections can be made. - Introduces
WP_Customize_Setting::validate()
,WP_Customize_Setting::$validate_callback
, and thecustomize_validate_{$setting_id}
filter. - Modifies
WP_Customize_Manager::save()
to check all settings for validity issues prior to calling theirsave
methods. - Introduces
WP_Customize_Setting::json()
for parity with the other Customizer classes. This includes exporting of thetype
.
Needs more unit tests.
#25
@
9 years ago
I’ve updated the sample extension plugin (Customize Validate Entitled Settings) to make use of this patch: https://gist.github.com/westonruter/1016332b18ee7946dec3
#26
@
9 years ago
@westonruter,
Why is the notification area placed at the bottom of the control container when there is no .customize-control-title
, as opposed to the top?
#27
@
9 years ago
@dlh good question. I suppose the reason is that if there is no .customize-control-title
element in a control, then perhaps the control is using a different element to serve as the title and it would make sense to prevent the notification from being injected before any such alternate title? Or if there is a control that just consists of a checkbox and label, without any title, then it wouldn't make sense for the notification appear above the title-less control?
#29
@
9 years ago
@westonruter Those are fair considerations. I'm not sure whether my reasons in favor of displaying the notifications at the top of the container overcome them, but here they are:
- The notifications are not visible when the control is taller than the available space (see screenshot).
- Users might consider it inconsistent for notifications to appear above inputs sometimes and below inputs other times (but that is only speculation).
#30
follow-up:
↓ 31
@
9 years ago
@dlh thanks for demonstrating that. But it could be that the notification could be obscured at the top as well for such a long control, right? In such cases, perhaps it should just be left up to the custom control to just override the getNotificationsContainerElement
to place the container at the appropriate spot?
#31
in reply to:
↑ 30
;
follow-up:
↓ 39
@
9 years ago
But it could be that the notification could be obscured at the top as well for such a long control, right?
True, except (I think) if you are in a different Customizer section when you save. In that case, it looks like you are brought back to top of the control with the notification at the bottom.
In such cases, perhaps it should just be left up to the custom control to just override the getNotificationsContainerElement to place the container at the appropriate spot?
Yeah, that sounds like it could also be a straightforward approach.
#32
follow-up:
↓ 42
@
9 years ago
- Priority changed from normal to high
34893.4.diff cleans up the CSS a bit, see slightly-revised screenshot. One part that needs particular testing - the former width: 98%
for all customizer inputs is revised to 100%
so that they will align with other elements (such as media controls) and notifications.
Other thoughts:
- I would be in favor of injecting at the top of the control when there isn't a title unless the custom control overrides the element.
- Selective refresh seems to revert to the previous setting value when a setting is invalid. Perhaps we should do this but also keep it faded/in an updating state to indicate that there is a problem with the current setting (hence the previous value being used)?
- We will want to move the notification out of the
<label>
element, but let's wait to do that until we do a general cleanup of thelabel
structure in the customizer, since that will require a broader approach. - I think we could probably use the class
.has-notifications
on.customize-control
instead of having both.customize-control
and.customize-control-has-notifications
. - The per-control/setting notifications look pretty good. In terms of an overall notifications area, I think it would be helpful in terms of usability to aggregate all instances of errors into a single place that included autofocusing links to each of the controls with errors. I'll make a mockup of that on #35210.
#33
@
9 years ago
@celloexpressions @valendesigns any feedback on the latest patch? I have the code in a PR for inline commenting as well: https://github.com/xwp/wordpress-develop/pull/136
I want to commit this today so we can move on to transactions.
#34
@
9 years ago
@celloexpressions thanks for the feedback! I missed that you commented just before me :-)
#35
@
9 years ago
@westonruter I just tested the latest patch and here's what I think
- In example plugin, the error being reported is wrong, it should read
You must supply a Navigation Label
instead of thetitle
which begs the question of building a custom notification renderer which could get back the fieldname with WP_Error instance. - CSS Spacing issues are evident almost everywhere with the error notification. I can contribute on that part but probably it has more to do with how notifications are being rendered. Check the images attached below.
- Would it be better if we can designate a certain area for notifications at the top so that folks building custom controls won't mess up the notifications
Check the red line to visually differentiate the odd spacing issues.
#36
follow-up:
↓ 37
@
9 years ago
@mrahmadawais the CSS spacing should be correct for the site title control, but I didn't check the others. The intent is for 8px
above and below the notification, including any margins on adjacent elements.
#37
in reply to:
↑ 36
@
9 years ago
Replying to celloexpressions:
@mrahmadawais the CSS spacing should be correct for the site title control, but I didn't check the others. The intent is for
8px
above and below the notification, including any margins on adjacent elements.
I get that, but it doesn't play well with other conditions, as you can see in the comment above.
@westonruter Here's another suggestion.
- Error notification inside the widgets area is not looking good. The white background of the notification is getting mixed up with the white background of widget accordion. This can be solved by moved the error message to the top of the chain i.e. before the div with class
widget-top
and inside the div with classwidget
. It would look something like this
#38
@
9 years ago
@mrahmadawais:
Would it be better if we can designate a certain area for notifications at the top so that folks building custom controls won't mess up the notifications
For custom controls, developers can take two approaches to displaying notifications:
1) They can include a element with a customize-control-notifications-container
class in the control's content.
2) Or they can override the controlgetNotificationsContainerElement()
method to change how the notification gets injected.
Error notification inside the widgets area is not looking good. The white background of the notification is getting mixed up with the white background of widget accordion. This can be solved by moved the error message to the top of the chain i.e. before the div with class widget-top and inside the div with class widget. It would look something like this
I think the style should be modified rather than moving the notification outside of the control's collapsed contents. If it were outside, this would be confusing when users expect to see a list of controls not a mixture of a list of controls and validation messages.
@celloexpressions I tried re-using the default error
styles in my default div.customize-control-notifications-container
. But I think we should stop trying to do that. Instead of adding error
to that root container element, I think the error
should be added to the li
itself, and so individual notifications could be errors where others could be updates or informational messages.
@
9 years ago
Prepend instead of append notifications container: https://github.com/xwp/wordpress-develop/compare/3f060ed...3af6b10
#39
in reply to:
↑ 31
@
9 years ago
Replying to dlh:
But it could be that the notification could be obscured at the top as well for such a long control, right?
True, except (I think) if you are in a different Customizer section when you save. In that case, it looks like you are brought back to top of the control with the notification at the bottom.
And replying to celloexpressions:
I would be in favor of injecting at the top of the control when there isn't a title unless the custom control overrides the element.
Done in 34893.5.diff
#40
@
9 years ago
@westonruter You are right. Maybe we could just add #eee
borders to the notification. I just submitted a patch and now the notification in the widgets area looks better.
#41
@
9 years ago
@mrahmadawais that does look better, but I notice that the border on top/bottom is causing the border on the left to be cut diagonally. Perhaps a box shadow would have a better result?
Also sorry but I uploaded a new patch 34893.7.diff that didn't include your change. I moved the error
class to be an data-
attribute of the list item itself. This meant that I needed to change the list styles to be compact when multiple are displayed. See data-attributes.png.
@celloexpressions thoughts on this change? I leave the CSS up to you :)
Also, it would be easier to collaborate if you guys could push commits to the existing PR: https://github.com/xwp/wordpress-develop/pull/136 And this would avoid us having to manually reconcile and merge patches. You both have write access to that repo now.
#42
in reply to:
↑ 32
;
follow-up:
↓ 61
@
9 years ago
Replying to celloexpressions:
- I think we could probably use the class
.has-notifications
on.customize-control
instead of having both.customize-control
and.customize-control-has-notifications
.
I've done this in the latest patches.
- Selective refresh seems to revert to the previous setting value when a setting is invalid. Perhaps we should do this but also keep it faded/in an updating state to indicate that there is a problem with the current setting (hence the previous value being used)?
Yeah, the revert to the previous setting is a result of the invalid settings now being (properly) rejected, and so they are treated as if they weren't supplied at all. This would be true in both selective refresh and full-page refresh. At the moment, it would be up to the control to display the notification using JS to give feedback for why an update isn't appearing, as currently PHP-supplied error notifications are currently only being sent back upon attempted Save. However, with transactions (#30937) each change to a setting would get submitted to the server via a PATCH
request, and so this is where I think we'd need to ultimately provide this feedback for why a setting change isn't appearing in the preview. I don't know if we should display any invalidity state inside the preview itself.
This ticket was mentioned in Slack in #core-customize by westonruter. View the logs.
9 years ago
This ticket was mentioned in Slack in #design by westonruter. View the logs.
9 years ago
#46
@
9 years ago
@afercia thoughts on accessibility considerations? It is using aria-live="assertive"
for the notification container.
#47
@
9 years ago
Addressed accessibility feedback from @afercia in 34893.8.diff. Now using wp.a11y.speak()
for notifications added instead of an assertive aria live region for the entire notifications container.
#48
@
9 years ago
34893.8.diff:
sprintf( _n( 'There is %d invalid setting.', 'There are %d invalid settings.', $invalid_count ), $invalid_count );
should be
sprintf( _n( 'There is %s invalid setting.', 'There are %s invalid settings.', $invalid_count ), number_format_i18n( $invalid_count ) );
@
9 years ago
Use number_format_i18n(): https://github.com/xwp/wordpress-develop/pull/136/commits/81549ca484dcf649409d4ee3c1ae7b97497f8f81
@
9 years ago
Re-use existing notice styles and support notice-alt for widget controls https://github.com/xwp/wordpress-develop/pull/136/commits/77241302401ff5f74f5e07e060b18dfe1f15c423
@
9 years ago
Improve error styling for control containers and fix notice spacing for widgets, menus, and multiple vs. single notifications.
#49
@
9 years ago
- Keywords ux-feedback removed
34893.11.diff incorporates design refinements as discussed in #design in slack, and also fixes spacing of notifications for various cases. Indicators are only shown on menu item/widget headings when there is an error inside it (not for warnings/info).
#50
@
9 years ago
34893.12.diff fleshes out php/js-doc; only remove error notifications at save; fix qunit tests by adding a11y dependency. Also remove incremented param count for customize_sanitize_{$setting_id}
filter, since $strict
is no longer being used. (Includes changes from 34893.11.diff.)
#51
@
9 years ago
- Keywords commit has-unit-tests added; needs-testing needs-unit-tests removed
34893.13.diff adds unit tests for changes to WP_Customize_Manager
, with the exception of WP_Customize_Manager::save()
which currently lacks unit tests in core already. (New Ajax tests need to be written for it at some point.)
Suggesting commit
. Any objections?
This ticket was mentioned in Slack in #core by westonruter. View the logs.
9 years ago
#54
@
9 years ago
- Keywords ux-feedback added; commit removed
Removing commit
pending design meeting.
This ticket was mentioned in Slack in #design by hugobaeta. View the logs.
9 years ago
#56
@
9 years ago
Ongoing discussion in #design - haven't found a good way to outline nav menu item controls. Should probably commit 34893.13.diff for now and we'll continue iterating on that.
#57
@
9 years ago
@celloexpressions Besides the design of the collapsed controls for nav menu items and widgets (which is for multi-field controls), I recall there as also a desire from design to have the individual inputs for single-field controls to get a red border around the inputs when invalid: https://wordpress.slack.com/archives/design/p1463679781001325
This ticket was mentioned in Slack in #design by westonruter. View the logs.
9 years ago
#59
@
9 years ago
- Keywords commit added; ux-feedback removed
34893.14.diff includes the outline on text inputs with errors and replaces the left border with an outline on the header element (as an inset box-shadow) for menus and widgets. Should be ready for a first-pass commit. Additional design updates can happen as other approaches are developed.
#61
in reply to:
↑ 42
@
8 years ago
Replying to westonruter:
- Selective refresh seems to revert to the previous setting value when a setting is invalid. Perhaps we should do this but also keep it faded/in an updating state to indicate that there is a problem with the current setting (hence the previous value being used)?
Yeah, the revert to the previous setting is a result of the invalid settings now being (properly) rejected, and so they are treated as if they weren't supplied at all. This would be true in both selective refresh and full-page refresh. At the moment, it would be up to the control to display the notification using JS to give feedback for why an update isn't appearing, as currently PHP-supplied error notifications are currently only being sent back upon attempted Save. However, with transactions (#30937) each change to a setting would get submitted to the server via a
PATCH
request, and so this is where I think we'd need to ultimately provide this feedback for why a setting change isn't appearing in the preview. I don't know if we should display any invalidity state inside the preview itself.
Follow-up on this: #36944
I have implemented this in a feature plugin called “Customize Setting Validation”.
To do server-side validation of a setting, implement a setting sanitizer that returns
null
or aWP_Error
object:By returning
null
, the Customizer will display a generic error message. By returningWP_Error
, the specific message can be provided.The validation error message can also be set programmatically by JS by calling
control.validationMessage.set()
,for example from an extended
control.setting.validate()
method. ThevalidationMessage
is inspired by HTML5.For a demonstration of the functionality made possible with this Customizer setting validation API,
including how to do client-side validation, see the “Customize_Validate_Entitled_Settings” plugin. It will validate that the Site Name (
blogname
), nav menu item titles, and widget titles are all fully populated. The validation is done both on the client and on the server.