Make WordPress Core

Opened 8 years ago

Closed 7 years ago

Last modified 4 years ago

#37638 closed enhancement (fixed)

Allow plugins to do comprehensive late validation of settings

Reported by: westonruter's profile westonruter Owned by: westonruter's profile westonruter
Milestone: 4.7 Priority: normal
Severity: normal Version: 4.6
Component: Customize Keywords: has-patch needs-testing has-dev-note
Focuses: Cc:

Description

With #34893 there was introduced a new pass/fail validation model for Customizer settings. The constructor for WP_Customize_Setting will add a customize_validate_{$setting->id} filter if there is a supplied validate_callback param. Then inside of WP_Customize_Setting::validate() it applies_filters on this hook.

There is currently a scenario where a plugin could create a subclass of WP_Customize_Setting and with a custom validate method that neglects to apply_filters. For example:

<?php
class Required_Setting extends WP_Customize_Setting {
        function validate( $value ) {
                if ( empty( $value ) ) {
                        return new WP_Error( 'empty_value', __( 'You must supply a value' ) );
                }
                return true;
        }
}

The simplest fix here for a plugin would be to explicitly call parent::validate( $value ) like so:

<?php
class Required_Setting extends WP_Customize_Setting {
        function validate( $value ) {
                if ( empty( $value ) ) {
                        return new WP_Error( 'empty_value', __( 'You must supply a value' ) );
                }
                return parent::validate( $value ); // <= apply_filters on "customize_validate_{$this->id}"
        }
}

Nevertheless, since it is not mandated that a subclass's override method call its parent class's overridden method, there should be some way to guarantee that these filters get applied to account for this edge case.

There are two options I see:

1) We could just (re-)apply the filter in WP_Customize_Manager::validate_setting_values():

--- src/wp-includes/class-wp-customize-manager.php
+++ src/wp-includes/class-wp-customize-manager.php
@@ -1013,6 +1013,10 @@ final class WP_Customize_Manager {
                        }
                        $validity = $setting->validate( $unsanitized_value );
+                       if ( ! is_wp_error( $validity ) ) {
+                               /** This filter is documented in wp-includes/class-wp-customize-setting.php */
+                               $validity = apply_filters( "customize_validate_{$setting->id}", $validity, $unsanitized_value, $setting );
+                       }
                        if ( ! is_wp_error( $validity ) ) {
                                $value = $setting->sanitize( $unsanitized_value );
                                if ( is_null( $value ) ) {
                                        $validity = false;

2) We could introduce a new filter in WP_Customize_Manager::validate_setting_values() that applies on the array of setting validities:

  • src/wp-includes/class-wp-customize-manager.php

    final class WP_Customize_Manager { 
    10251025                        }
    10261026                        $validities[ $setting_id ] = $validity;
    10271027                }
     1028
     1029                /**
     1030                 * Filters the result of validating the setting values after each setting's validate method is called.
     1031                 *
     1032                 * @see WP_Customize_Setting::validate()
     1033                 *
     1034                 * @param array                $validities     Validities for settings, mapping setting ID to validity.
     1035                 * @param array                $setting_values Unsanitized setting values.
     1036                 * @param WP_Customize_Manager $wp_customize   Manager.
     1037                 */
     1038                $validities = apply_filters( 'customize_setting_validities', $validities, $setting_values, $this );
     1039
    10281040                return $validities;
    10291041        }

Attachments (2)

37638.option1.diff (754 bytes) - added by westonruter 8 years ago.
37638.option2.diff (906 bytes) - added by westonruter 8 years ago.

Download all attachments as: .zip

Change History (14)

#1 @chandrapatel
8 years ago

Hello @westonruter

I am seeing the problem with Option-1 solution is possible same filter apply twice from different files if developer didn't create subclass of WP_Customize_Setting.

In Option-2 solution, customize_setting_validities is new filter which also allow developer to validate setting.

So developer either do validation by adding filter on customize_validate_{$setting->id} or customize_setting_validities.

Another solution is add customize_validate_{$setting->id} filter inside WP_Customize_Manager::validate_setting_values() and remove from WP_Customize_Setting::validate(), so developer can use only one filter to validate setting and no need to introduce of new filter.

#2 follow-up: @westonruter
8 years ago

@chandrapatel I don't really see applying filters a second time to be a big problem. There is a problem with your alternate solution to move the apply_filters call: this needs to be done inside the validate method because if filters are used for validation, then if a plugin calls the validate() method somewhere else then it won't then work. It is intended that the validate() should be able to be called from other places than just validate_setting_values().

#3 in reply to: ↑ 2 @chandrapatel
8 years ago

Replying to westonruter:

There is a problem with your alternate solution to move the apply_filters call: this needs to be done inside the validate method because if filters are used for validation, then if a plugin calls the validate() method somewhere else then it won't then work. It is intended that the validate() should be able to be called from other places than just validate_setting_values().

Oh yes, I got your point.

I don't really see applying filters a second time to be a big problem.

Then Option-1 solution is better instead of introducing new filter. And also developer no need to rely on two different filters.

#4 @jorbin
8 years ago

  • Keywords needs-dev-note added

It's unlikely that running filters multiple times here will result in a problem, but it's certainly plausible. I would love to see this land early and get called out for testing. Likely can be during the normal customizor post, but I'm also happy to add a note to this week in core when this lands.

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


7 years ago

#6 @desrosj
7 years ago

  • Keywords needs-testing added

As @jorbin stated, there is a small potential for an issue running filters multiple times. This patch needs testing to make sure there are no problems.

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


7 years ago

#8 @celloexpressions
7 years ago

  • Owner set to westonruter
  • Status changed from new to assigned

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


7 years ago

#10 @westonruter
7 years ago

  • Keywords commit added

I'll go with 37638.option1.diff since it doesn't introduce a new filter and any handlers are designed to be run multiple times.

#11 @westonruter
7 years ago

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

In 38765:

Customize: Ensure customize_validate_{$setting->id} filters apply on input post values for WP_Customize_Setting subclasses that neglect to apply the filter themselves.

Fixes #37638.

#12 @desrosj
4 years ago

  • Keywords has-dev-note added; needs-dev-note commit removed
Note: See TracTickets for help on using tickets.