Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WP-r44392: Customize: Safeguard a check on the `customize_validate_{$… #398

Merged
merged 1 commit into from Mar 26, 2019

Conversation

Projects
None yet
3 participants
@Mte90
Copy link
Collaborator

commented Mar 11, 2019

…setting_id}filter value to ensure it is aWP_Error`.

While the filter is documented to only support a WP_Error, it has been a common practice to return true in a validation function if no errors have occurred. This was already caught when the same filter was executed in WP_Customize_Setting, it was however missing in WP_Customize_Manager::validate_setting_values().

WP:Props flixos90.

Merges https://core.trac.wordpress.org/changeset/43578 to the 5.0 branch.
Fixes https://core.trac.wordpress.org/ticket/44809.


Merges https://core.trac.wordpress.org/changeset/44392 / WordPress/wordpress-develop@45a8f1d to ClassicPress.

WP-r44392: Customize: Safeguard a check on the `customize_validate_{$…
…setting_id}` filter value to ensure it is a `WP_Error`.

While the filter is documented to only support a `WP_Error`, it has been a common practice to return true in a validation function if no errors have occurred. This was already caught when the same filter was executed in `WP_Customize_Setting`, it was however missing in `WP_Customize_Manager::validate_setting_values()`.

WP:Props flixos90.

Merges https://core.trac.wordpress.org/changeset/43578 to the 5.0 branch.
Fixes https://core.trac.wordpress.org/ticket/44809.

----
Merges https://core.trac.wordpress.org/changeset/44392 / WordPress/wordpress-develop@45a8f1d to ClassicPress.
@nylen

This comment has been minimized.

Copy link
Member

commented Mar 12, 2019

Have you seen this bug on a live site?

How can we test the bug and the fix?

@Mte90

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 13, 2019

No I didn't saw this problem on a site but looking of the ticket text can be a fatal error because there is no consistency between docs and the code of the filter https://core.trac.wordpress.org/ticket/44809
Because in case of error the filter require a WP_Error but there isn't a check for it in the code and devs return true instead of an object.
I don't think that for an edge case we want fatal error on our CP instances :-)

We can create a unit test for this but it is only for consistency with wp and also with the documentation this little fix.

@nylen

This comment has been minimized.

Copy link
Member

commented Mar 14, 2019

OK - I think this particular fix is pretty harmless, but generally we need to focus on things that we know we want, and be clear about why we want them. If no one is seeing this error, and we don't know how to test and reproduce it, then why should we fix it?

For one example, the WP notice to warn the user about an outdated PHP version would be a better place to focus any backporting efforts right now, since it's something we know we want.

@Mte90

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 14, 2019

I agree about prioritize the backporting but I was just trying to keep the compatibility and consistency for the future between WP/CP.
If we can approve and merge the 5.0.3 patch we can move on onther version and slowly work on the more hardest changeset to integrate.
Just that we are not facing this issue doesn't means that we don't want a bugfix, yes the problem of testing is a big problem and probably we need a plan.

Like for every changeset from WP if there aren't tests we want them as mandatory so we can improve our codebase slowly.

@nylen

This comment has been minimized.

Copy link
Member

commented Mar 14, 2019

I was just trying to keep the compatibility and consistency for the future between WP/CP.

I agree this is a good idea in theory, however...

If we can approve and merge the 5.0.3 patch we can move on onther version and slowly work on the more hardest changeset to integrate.

Generally we can't merge untested code. For each change we need someone to understand and explain what it is and how it works.

Then, in order to review and approve a change, I need to know how to test it. Automated tests are best, but we can live with manual testing also. If we don't have any testing at all, then we are going to break something, and it's better to just leave the code alone.

@Mte90

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 16, 2019

This filter is used by 2 plugins: https://wpdirectory.net/search/01D63DJGBP2N5ZVZB0R02C0B3A
And 0 themes in the WP repository

I am testing it with but I am not getting any error on saving:

add_filter( 'customize_validate_page_on_front', function() {
return true;
}, 10, 2 );
@Mte90

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 18, 2019

I am investigating this problem and I can see only the problem of that there is no check about the return.
The code at https://github.com/ClassicPress/ClassicPress/blob/develop/src/wp-includes/class-wp-customize-manager.php#L2279 is not doing a check about what is passing to the filter.

@nylen

This comment has been minimized.

Copy link
Member

commented Mar 26, 2019

OK, I understand this now. Thanks for the additional info.

https://core.trac.wordpress.org/changeset/42761#file12

Before this change in WP, there is no error:

$validity = true; // if filter has 'return true;'
if ( ! empty( $validity->errors ) ) {
    // this will never be executed
}

After this change in WP, there is an error:

$validity = true; // if filter has 'return true;'
if ( ! $validity->has_errors() ) { // ERROR!
    // ...
}

So, this was never a bug that would have affected ClassicPress, unless we also merge this other changeset.

This is a good example of why it's important to be careful even with backporting changes from WP. If we had merged the has_errors() function instead of this change, then we would have broken something without knowing about it.

@nylen

nylen approved these changes Mar 26, 2019

Copy link
Member

left a comment

This has no effect on ClassicPress but it may guard against a bug in the future (if we change this from ->errors to ->has_error() like WP did).

@nylen nylen merged commit 282a59f into ClassicPress:develop Mar 26, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.