WordPress.org

Make WordPress Core

Opened 23 months ago

Last modified 16 months 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:
PR Number:

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 (6)

43208.diff (3.8 KB) - added by flixos90 23 months ago.
43208.2.diff (3.8 KB) - added by flixos90 22 months ago.
43208.3.diff (5.8 KB) - added by flixos90 22 months ago.
43208.4.diff (7.3 KB) - added by wakkermedia 22 months ago.
43208.5.diff (8.1 KB) - added by flixos90 21 months ago.
43208.6.diff (8.1 KB) - added by flixos90 16 months ago.

Download all attachments as: .zip

Change History (13)

@flixos90
23 months ago

#1 @flixos90
23 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
23 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
22 months ago

@flixos90
22 months ago

#3 @flixos90
22 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.


22 months ago

#5 @WakkerMedia
22 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
21 months ago

#6 @flixos90
21 months 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.

@flixos90
16 months ago

#7 @flixos90
16 months ago

43208.6.diff ensures the patch applies cleanly again. It also changes the validate_option_{$option} filter into an action, in accordance which how a similar new hook was handled in #40364. An action is the proper way to amend a passed-in object. They are passed by reference, so filtering them through can only have negative side-effects with invalid filter values.

I also wrote a small test plugin for that, which registers a few different options with validation adds an interface to them via an options page, a Customizer section, and the REST API settings controller. You can easily test the new functionality using it.

<?php
/*
Plugin Name: Option Validation Tests
Plugin URI: https://make.wordpress.org/
Description: Tests for option validation.
Version: 1.0
Author: WordPress Core Contributors
Author URI: https://make.wordpress.org/
*/

function test_register_settings() {
        register_setting(
                'option_validation_tests',
                'ovt_integer',
                array(
                        'type'              => 'integer',
                        'description'       => __( 'An integer option.' ),
                        'sanitize_callback' => 'absint',
                        'validate_callback' => function( $errors, $value ) {
                                test_validate_setting( $errors, $value, array( 'min' => 3 ) );
                        },
                        'show_in_rest'      => true,
                )
        );

        register_setting(
                'option_validation_tests',
                'ovt_email',
                array(
                        'type'              => 'string',
                        'description'       => __( 'An email option.' ),
                        'sanitize_callback' => 'sanitize_email',
                        'validate_callback' => function( $errors, $value ) {
                                test_validate_setting( $errors, $value, array( 'email' => true ) );
                        },
                        'show_in_rest'      => true,
                )
        );

        register_setting(
                'option_validation_tests',
                'ovt_bool',
                array(
                        'type'              => 'boolean',
                        'description'       => __( 'A boolean option.' ),
                        'sanitize_callback' => 'boolval',
                        'validate_callback' => function( $errors, $value ) {
                                test_validate_setting( $errors, $value, array( 'checked' => true ) );
                        },
                        'show_in_rest'      => true,
                )
        );

        register_setting(
                'option_validation_tests',
                'ovt_enum',
                array(
                        'type'              => 'string',
                        'description'       => __( 'An enum option.' ),
                        'sanitize_callback' => 'strip_tags',
                        'validate_callback' => function( $errors, $value ) {
                                test_validate_setting( $errors, $value, array( 'enum' => array( 'value1', 'value2', 'value3' ) ) );
                        },
                        'show_in_rest'      => true,
                )
        );
}
add_action( 'init', 'test_register_settings' );

function test_validate_setting( $errors, $value, $args = array() ) {
        if ( isset( $args['enum'] ) && ! in_array( $value, $args['enum'], true ) ) {
                $errors->add( 'value_not_in_enum', sprintf( __( 'The value %s is not one of the valid choices.' ), esc_html( $value ) ) );
        }

        if ( isset( $args['min'] ) && (float) $value < (float) $args['min'] ) {
                $errors->add( 'value_too_small', sprintf( __( 'The value %1$s is smaller than the allowed minimum %2$s.' ), number_format_i18n( (float) $value ), number_format_i18n( (float) $args['min'] ) ) );
        }

        if ( isset( $args['max'] ) && (float) $value > (float) $args['max'] ) {
                $errors->add( 'value_too_great', sprintf( __( 'The value %1$s is greater than the allowed maximum %2$s.' ), number_format_i18n( (float) $value ), number_format_i18n( (float) $args['max'] ) ) );
        }

        if ( isset( $args['email'] ) && $args['email'] && ! is_email( $value ) ) {
                $errors->add( 'value_not_an_email', sprintf( __( 'The value %s is not a valid email address.' ), esc_html( $value ) ) );
        }

        if ( isset( $args['checked'] ) && $args['checked'] && ! $value ) {
                $errors->add( 'value_unchecked', __( 'You must check the checkbox.' ) );
        }
}

function test_add_settings_fields() {
        add_settings_section( 'general', __( 'General' ), null, 'option_validation_tests' );

        add_settings_section( 'other', __( 'Other' ), null, 'option_validation_tests' );

        add_settings_field(
                'ovt_integer',
                __( 'Integer Option' ),
                function() {
                        ?>
                        <input type="number" name="ovt_integer" value="<?php echo esc_attr( get_option( 'ovt_integer' ) ); ?>" />
                        <p class="description"><?php esc_html_e( 'Enter a value greater than or equal to 3.' ); ?></p>
                        <?php
                },
                'option_validation_tests',
                'general',
                array( 'label_for' => 'ovt_integer' )
        );

        add_settings_field(
                'ovt_email',
                __( 'Email Option' ),
                function() {
                        ?>
                        <input type="email" name="ovt_email" value="<?php echo esc_attr( get_option( 'ovt_email' ) ); ?>" />
                        <p class="description"><?php esc_html_e( 'Enter an email address.' ); ?></p>
                        <?php
                },
                'option_validation_tests',
                'general',
                array( 'label_for' => 'ovt_email' )
        );

        add_settings_field(
                'ovt_bool',
                __( 'Boolean Option' ),
                function() {
                        ?>
                        <input type="checkbox" name="ovt_bool" <?php checked( get_option( 'ovt_bool' ) ); ?> />
                        <p class="description"><?php esc_html_e( 'Check this checkbox.' ); ?></p>
                        <?php
                },
                'option_validation_tests',
                'general',
                array( 'label_for' => 'ovt_bool' )
        );

        add_settings_field(
                'ovt_enum',
                __( 'Enum Option' ),
                function() {
                        ?>
                        <input type="text" name="ovt_enum" value="<?php echo esc_attr( get_option( 'ovt_enum' ) ); ?>" />
                        <p class="description"><?php esc_html_e( 'Enter either value1, value2, or value3.' ); ?></p>
                        <?php
                },
                'option_validation_tests',
                'other',
                array( 'label_for' => 'ovt_enum' )
        );
}
add_action( 'admin_init', 'test_add_settings_fields' );

function test_register_settings_page() {
        add_options_page( __( 'Option Validation Test' ), __( 'Option Validation Test' ), 'manage_options', 'option_validation_tests', 'test_render_settings_page' );
}
add_action( 'admin_menu', 'test_register_settings_page' );

function test_render_settings_page() {
        ?>
        <div class="wrap">
                <h1><?php _e( 'Option Validation Test' ); ?></h1>

                <form action="options.php" method="post" novalidate="novalidate">
                        <?php settings_fields( 'option_validation_tests' ); ?>
                        <?php do_settings_sections( 'option_validation_tests' ); ?>
                        <?php submit_button(); ?>
                </form>
        </div>
        <?php
}

function test_customize_register( $wp_customize ) {
        $wp_customize->add_setting( 'ovt_integer', array( 'type' => 'option' ) );
        $wp_customize->add_setting( 'ovt_email', array( 'type' => 'option' ) );
        $wp_customize->add_setting( 'ovt_bool', array( 'type' => 'option' ) );
        $wp_customize->add_setting( 'ovt_enum', array( 'type' => 'option' ) );

        $wp_customize->add_section(
                'option_validation_tests',
                array(
                        'title'      => __( 'Option Validation Test' ),
                        'capability' => 'manage_options',
                )
        );

        $wp_customize->add_control(
                'ovt_integer',
                array(
                        'section'     => 'option_validation_tests',
                        'type'        => 'number',
                        'label'       => __( 'Integer Option' ),
                        'description' => __( 'Enter a value greater than or equal to 3.' ),
                )
        );

        $wp_customize->add_control(
                'ovt_email',
                array(
                        'section'     => 'option_validation_tests',
                        'type'        => 'email',
                        'label'       => __( 'Email Option' ),
                        'description' => __( 'Enter an email address.' ),
                )
        );

        $wp_customize->add_control(
                'ovt_bool',
                array(
                        'section'     => 'option_validation_tests',
                        'type'        => 'checkbox',
                        'label'       => __( 'Boolean Option' ),
                        'description' => __( 'Check this checkbox.' ),
                )
        );

        $wp_customize->add_control(
                'ovt_enum',
                array(
                        'section'     => 'option_validation_tests',
                        'type'        => 'text',
                        'label'       => __( 'Enum Option' ),
                        'description' => __( 'Enter either value1, value2, or value3.' ),
                )
        );
}
add_action( 'customize_register', 'test_customize_register' );

Still looking for feedback on this, I consider it a valuable enhancement that could make plugin authors' lives a lot easier.

It also opens up possibilities for further improvements: For example, $wp_customize->add_setting() calls could automatically happen for options that are registered because providing those manually in such a case is often redundant. If someone registers a control with an option identifier for an option that is registered and no $wp_customize->add_setting() call is manually made, those could be "auto-filled".

Last edited 16 months ago by westonruter (previous) (diff)
Note: See TracTickets for help on using tickets.