WordPress.org

Make WordPress Core

Opened 15 months ago

Closed 14 months ago

Last modified 5 months ago

#36944 closed defect (bug) (fixed)

Display server-sent Customizer setting validation errors before save is attempted

Reported by: westonruter Owned by: westonruter
Milestone: 4.6 Priority: normal
Severity: normal Version: 4.6
Component: Customize Keywords: has-patch has-unit-tests commit
Focuses: Cc:

Description

Following up on #34893.

At the moment, the setting validation errors that are returned by WP_Customize_Setting::validate() are only displayed when a save is attempted. This is the only situation where these errors are sent back from the server, as part of the customize_save_response. However, now that there is strict validation available in the Customizer, if a validation error does happen when editing a setting, then the behavior is for the Customizer preview to act as if the supplied setting does not exist at all. This is a bad user experience because then their changes in the preview just disappear. In this case, I should think it is unlikely that the user would try to save their changes since they don't appear properly in the preview, and thus the validation errors would never get displayed to the user.

With transactions, this issue would be addressed by continually sending the setting values to the server to amend the transaction, and in the response the validation errors could be included, as noted in #34893 (comment 42):

  • 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.

In the mean time, without transactions in core, there needs to be an alternate mechanism for giving early feedback on validation errors. I suggest that we can send back the setting validation states whenever the preview is refreshed or whenever a selecting refresh occurs.

Change History (22)

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


15 months ago

#2 @westonruter
15 months ago

  • Keywords needs-unit-tests needs-patch added
  • Owner set to westonruter
  • Status changed from new to accepted

In 36944.0.wip.diff:

  • Send back all validities for all settings, not just the error notifications for invalid settings.
  • Explicitly link a control notification to its corresponding setting.
  • Keep track of which notifications are from the server versus which are created in JS.
  • Prevent submitting settings if there are validity errors that are not from the server.
  • Skip removing all notification errors at save, only removing errors if they are absent from the subsequent save response.
  • Add wp.customize.findControlsForSettings( settingIds ) helper.

What is next is to make sure the setting validity check are done during full refresh and partial refresh requests, and sent back to the Customizer as well.

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


15 months ago

This ticket was mentioned in Slack in #core-customize by westonruter. View the logs.


15 months ago

#5 follow-up: @westonruter
14 months ago

  • Keywords has-patch added; needs-patch removed

In 36944.1.diff:

  • Export setting validities with full refreshes and selective refreshes, giving near immediate feedback for validation errors.
  • Introduce WP_Customize_Manager::prepare_setting_validity_for_js().

The UX should be so much better to show/hide notifications for server-side setting validation handling.

@fjarrett @mrahmadawais @aristath @natewr would one or more of you please test?

#6 @westonruter
14 months ago

  • Keywords has-unit-tests added; needs-unit-tests removed

#7 in reply to: ↑ 5 ; follow-up: @mrahmadawais
14 months ago

Replying to westonruter:

The UX should be so much better to show/hide notifications for server-side setting validation handling.

@fjarrett @mrahmadawais @aristath @natewr would one or more of you please test?

Sure.

So, to test it, I need to apply this patch to the core, what else? Should I install the validation gist plugin as an example?

#8 in reply to: ↑ 7 ; follow-up: @westonruter
14 months ago

Replying to mrahmadawais:

So, to test it, I need to apply this patch to the core, what else? Should I install the validation gist plugin as an example?

Exactly. Apply the patch and add the Customize Validate Entitled Settings demo plugin. You should see then that validation errors happen in conjunction with selective refreshes and full refreshes.

#9 follow-up: @NateWr
14 months ago

I had a quick test with the demo plugin. I really like it. A couple of thoughts:

  1. It can be a little bit disorienting the way that the preview falls back to the currently saved value. For instance, I title a widget Hello and then save it. Then I type Hello World and see it in the preview. Then I add a ! and the preview goes back to showing Hello. I don't think it's a critical problem, especially since the error message appears there. But can still feel a bit odd. If it could show the last valid value that'd be great.
  1. Skimming the code in the diff and the demo plugin, it looks like JS Setting objects now have a Notification object attached, and these are passed to a control, which has a renderNotifications method. So to render a validation error in a custom control we just need to do some work in the renderNotifications method, correct? That seems pretty straightforward if correct, and if I understand the code correctly it seems like errors will be announced (wp.ally.speak) prior to this, meaning we don't need to reproduce that. Very nice.
  1. I'm wondering how I would go about extending a Notification object's data param. For my use case, I have a kind of "uber" Control. It saves to a single Setting, but in the process it compiles a bunch of different input fields into a single serialized string. So for me, the setting id won't be sufficient for determining where to display the error message. I would need additional params attached to the Notification. It seems to include a data param which looks like it is where extra data could be attached, but I'm not seeing anything in api._handleSettingValidities that would allow me to attach extra params. Can you point me to what I'm missing?

#10 in reply to: ↑ 9 @westonruter
14 months ago

Replying to NateWr:

I had a quick test with the demo plugin. I really like it. A couple of thoughts:

Thanks for the great feedback!

  1. It can be a little bit disorienting the way that the preview falls back to the currently saved value. For instance, I title a widget Hello and then save it. Then I type Hello World and see it in the preview. Then I add a ! and the preview goes back to showing Hello. I don't think it's a critical problem, especially since the error message appears there. But can still feel a bit odd. If it could show the last valid value that'd be great.

That's a good idea. And actually, this improvement you describe could be something we get for free with transactions (#30937). The problem with implementing it right now (before transactions) is that the setting values entered do not persist anywhere except in the life of the request. But with transactions, each update would get written to the transaction, with any invalid values being rejected from being written to the transaction (I presume). So yeah, in your example “Hello World” would get written to the transaction but then “Hello World!” would get rejected, so the preview would remain with “Hello World”.

  1. Skimming the code in the diff and the demo plugin, it looks like JS Setting objects now have a Notification object attached, and these are passed to a control, which has a renderNotifications method. So to render a validation error in a custom control we just need to do some work in the renderNotifications method, correct? That seems pretty straightforward if correct, and if I understand the code correctly it seems like errors will be announced (wp.ally.speak) prior to this, meaning we don't need to reproduce that. Very nice.

Yes, a control can override how notifications are displayed by overriding the renderNotifications in the wp.customize.Control subclass.

  1. I'm wondering how I would go about extending a Notification object's data param. For my use case, I have a kind of "uber" Control. It saves to a single Setting, but in the process it compiles a bunch of different input fields into a single serialized string. So for me, the setting id won't be sufficient for determining where to display the error message. I would need additional params attached to the Notification. It seems to include a data param which looks like it is where extra data could be attached, but I'm not seeing anything in api._handleSettingValidities that would allow me to attach extra params. Can you point me to what I'm missing?

This sounds very similar to what is needed for Customize Posts as well. For this plugin, a post setting contains the title, content, excerpt, and all the other core fields. When an invalid title is supplied, the entire setting is marked as invalid. However, there needs to be a way to indicate which specific control should receive the notification even though they all share the same post setting. The short answer is that the validate method for the setting needs to do something like this:

return new WP_Error( 'invalid_title', __( 'Invalid title' ), array( 'post_field' => 'post_title' ) )

And then this post_field data will be included in the Notification object's data property, which you can use to determine whether or not the error should be displayed for a given control. In the case of Customize Posts, it should only get rendered on the Title control. I'm going to work on this for Customize Posts and I'll share with you the PR so you can review and adapt for your as well.

Otherwise, a +1 from you for commit?

#11 @westonruter
14 months ago

In 36944.3.diff:

  • Introduce Setting#findControls to split out logic from wp.customize.findControlsForSettings, allowing individual settings to override the logic for how associated controls are found.

@NateWr this change should help you give some control (pun intended) over how controls are listed for a given setting with associated notifications. You can override a setting's findControls method to handle a case when a single setting is associated with multiple controls, or when a single control has multiple fields that map to a setting with an object value (e.g. nav menu items or widget). I added support for handling notifications for a setting associated with multiple controls in the Customize Posts plugin: https://github.com/xwp/wp-customize-posts/pull/180

#12 @NateWr
14 months ago

Cheers @westonruter. I had a quick look over the PR and I think it makes sense. I see now how you set the setting_property param from the server side, so that's useful to know.

Yes, a +1 from me for commit, but I'm not exactly anyone to check with on that sort of thing. :)

#13 in reply to: ↑ 8 @mrahmadawais
14 months ago

Replying to westonruter:

So, I went ahead and tested the patch. Works perfect. The UX is much better now. I had a similar concern as of what @NateWr had on extending a Notification object's data param to display the notification for the right Setting ID (which may be attached to another Setting ID triggering the validation error) but you have already answered and addressed it in the PR 180.

+1 for the commit for sure.

#14 @westonruter
14 months ago

  • Keywords commit added

@NateWr @mrahmadawais I just updated the “Customize Validate Entitled Settings” demo plugin to also implement this routing of the error notifications to the title field: https://gist.github.com/westonruter/1016332b18ee7946dec3/revisions#diff-c7d2182050c602e788c279958c8c0006"

Thanks both for reviewing!

#15 @westonruter
14 months ago

  • Resolution set to fixed
  • Status changed from accepted to closed

In 37700:

Customize: Update server-sent setting validation notifications as changes are entered.

Send back setting validities with full refreshes and selective refreshes so that invalid settings can have notifications displayed immediately before attempting save, and so that these notifications can be cleared as soon as the input is corrected.

  • Splits out JS logic for listing controls into separate methods wp.customize.Setting.prototype.findControls() and wp.customize.findControlsForSettings().
  • Adds a setting property to the data on notifications added to controls that are synced from their settings.
  • Adds selective-refresh-setting-validities message sent from preview to pane.
  • Changes WP_Customize_Manager::validate_setting_values() to return when settings are valid as well as invalid.
  • Adds WP_Customize_Manager::prepare_setting_validity_for_js().
  • Add setting validities to data exported to JS in Customizer Preview and in selective refresh responses.

Fixes #36944.

#16 @westonruter
14 months ago

In 37867:

Customize: Always define functions reflowPaneContents, findControlsForSettings, and _handleSettingValidities on wp.customize.

Moves definitions of functions outside of a jQuery.ready() callback, as these functions needn't be deferred to DOM ready. This change also ensures that the functions are available if customize-controls is enqueued outside of the Customizer context, as the ready callback short-circuits if _wpCustomizeSettings is not defined. The findControlsForSettings and _handleSettingValidities functions were misplaced in r37700.

See #34893.
See #36944.
See #29071.

#17 @westonruter
5 months ago

In 40319:

Customize: Prevent client-side validation from being cleared when no corresponding server-side validation is present.

See #36944.
Fixes #39770.

#18 @swissspidy
5 months ago

In 40345:

Customize: Prevent client-side validation from being cleared when no corresponding server-side validation is present.

See #36944.
Fixes #39770.

Merges [40319] to the 4.7 branch.

Note: See TracTickets for help on using tickets.