WordPress.org

Make WordPress Core

Opened 4 months ago

Last modified 8 weeks ago

#43208 new enhancement

Separate setting validation from sanitization

Reported by: flixos90 Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version:
Component: Options, Meta APIs Keywords: 2nd-opinion has-patch needs-unit-tests
Focuses: Cc:

Description

As widely known, validation is different from sanitization. A value should first be validated and then be sanitized. Historically, WordPress has been mixing these two responsibilities in the sanitize_option() function, however it is easily possible to add an extra layer on top of that which maintains full backward-compatibility.

Newer parts of core, such as the Customizer and the REST API, have been dealing with this in a better way, keeping the two separate. We can achieve the same for options themselves too.

I suggest introducing a validate_option_{$option} filter that works somewhat similar like the customize_validate_{$setting_id} filter used in the Customizer. It passes an empty WP_Error object that can be added to. In addition to allow separate validation from sanitization, it also makes handling of validation easier, since it can then automatically set the value to the previous value and call add_settings_error(), passing any error messages set, which matches current core behavior.

Attachments (5)

43208.diff (3.8 KB) - added by flixos90 4 months ago.
43208.2.diff (3.8 KB) - added by flixos90 4 months ago.
43208.3.diff (5.8 KB) - added by flixos90 4 months ago.
43208.4.diff (7.3 KB) - added by wakkermedia 4 months ago.
43208.5.diff (8.1 KB) - added by flixos90 8 weeks ago.

Download all attachments as: .zip

Change History (11)

@flixos90
4 months ago

#1 @flixos90
4 months ago

  • Keywords has-patch added; needs-patch removed

43208.diff introduces the validate_option_{$option} filter as a possible implementation. Note that it is only fired if previous core validation didn't already produce an error. This allows for full backward-compatibility, allows performing additional validation even on core values, while preventing plugins from removing errors detected by core.

For easy handling of setting validation, a $validate_callback argument has been introduced for register_setting() which is then hooked into the filter.

#2 @flixos90
4 months ago

An alternative approach: Introduce an entirely new function validate_option() and do all validation including the filter in there. This function would then be called inside of add_option() and update_option() (before sanitize_option()), and if an error is returned, make the function not change the value set and return false. This actually seems like a cleaner approach to me, but also like a bigger change. Let's discuss further.

@flixos90
4 months ago

@flixos90
4 months ago

#3 @flixos90
4 months ago

  • Keywords needs-unit-tests added

43208.2.diff is a cleaner approach to separating validation from sanitization. It introduces a function validate_option( $option, $value ), that consists of a filter to perform setting validation, which was inspired by the existing Customizer logic following a similar purpose. A validate callback can be provided intuitively via register_setting(). Benefits:

  • Validation is not tied into updating the option. This allows for flexible handling: First, check if the value is valid. Then, if invalid, cancel the process, or if valid, proceed with updating the option.
  • No backward-compatibility issues, as it's a new function. Validation of core options that were previously handled in sanitize_option() should remain that way.

Of course, just having the function there won't actually provide a huge benefit. Therefore 43208.3.diff contains implementation of the integration points, which are:

  • Updating an option via a settings page submission (src/wp-admin/options.php).
  • Updating an option from the Customizer (src/wp-includes/class-wp-customize-setting.php).
  • Updating an option via the REST API (src/wp-includes/rest-api/endpoints/class-wp-rest-settings-controller.php).

This makes handling feedback about validation errors simple and straightforward. The REST API previously didn't handle this at all, for settings page submissions it was achieved in a somewhat hacky way by using sanitize_option() and calling add_settings_error() from there. The Customizer was the only location already handling validation properly, however even here there's now the benefit of having a uniform way to register validation for settings. Validation callbacks specific to the Customizer will still fire, so it's fully backward-compatible.

@joehoyle @johnjamesjacoby @SergeyBiryukov Would like to have your feedback on this approach. Any objections/considerations? I'll work on unit tests for this too, if it's a viable approach.

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


4 months ago

@wakkermedia
4 months ago

#5 @WakkerMedia
4 months ago

in 43208.4.diff Added validate_option functionality to class-wp-customize-manager.php on line: 2303 - 2311 Because the customize validator is also used in that file.

@flixos90
8 weeks ago

#6 @flixos90
8 weeks ago

43208.5.diff makes the following changes:

  • Fix minor bug in register_setting() where the count of arguments passed to the filter was wrong prior.
  • Only call validate_option() in Customizer if the customize setting is an actual option, NOT part of a multidimensional array option (a new utility method is_multidimensional() has been introduced for that).
  • Call validate_option() before calling update_option() in the Customizer, and do not save if errors occur.

The latter two changes above ensure that options in the Customizer are validated as expected. We could go even further if we wanted to and even validate multidimensional option settings. That would require us to call validate_option() with the whole root value, with the actual value being part of the array (which could happen with the multidimensional_replace() method). However, since all of this logic needs to happen in the WP_Customize_Manager as well, we'd need to make the get_root_value() and multidimensional_replace() methods public, which would be ugly. On the other hand, the requirement to have duplicate logic here is ugly already anyway. :/

Pinging @johnjamesjacoby for general feedback and @westonruter for the Customizer part specifically.

Note: See TracTickets for help on using tickets.