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

Use `$wpdb->remove_placeholder_escape()` when exporting in WP 4.8.3 and greater #43

Merged
merged 6 commits into from Nov 10, 2017

Conversation

3 participants
@mullnerz
Contributor

mullnerz commented Nov 9, 2017

Fixes #42

@mullnerz

This comment has been minimized.

Show comment
Hide comment
@mullnerz

mullnerz Nov 9, 2017

Contributor

Hello @szabacsik

Contributor

mullnerz commented Nov 9, 2017

Hello @szabacsik

@danielbachhuber

In order to land this pull request, we'll need to do a couple of things:

  1. Add tests that explicitly cover the breaking change.
  2. Internalize the entirety of remove_placeholder_escape() to a private method on the class, and use that instead, because the method isn't available on older WP installs.
$sql = $wpdb->prepare( $sql, array_values( $values ) );
if( method_exists( $wpdb, 'remove_placeholder_escape' ) ) {
// since 4.8.3
$sql = $wpdb->remove_placeholder_escape( $wpdb->prepare( $sql, array_values( $values ) ) );

This comment has been minimized.

@danielbachhuber

danielbachhuber Nov 9, 2017

Member

Is there a reason we can't internalize remove_placeholder_escape and use it with earlier WP versions?

@danielbachhuber

danielbachhuber Nov 9, 2017

Member

Is there a reason we can't internalize remove_placeholder_escape and use it with earlier WP versions?

This comment has been minimized.

@mullnerz

mullnerz Nov 9, 2017

Contributor

You don't need it for earlier WP versions. WP internal behavior changed in 4.8.3 and the method remove_placeholder_escape was provided at the same time to help plugin developers:
https://make.wordpress.org/core/2017/10/31/changed-behaviour-of-esc_sql-in-wordpress-4-8-3/

@mullnerz

mullnerz Nov 9, 2017

Contributor

You don't need it for earlier WP versions. WP internal behavior changed in 4.8.3 and the method remove_placeholder_escape was provided at the same time to help plugin developers:
https://make.wordpress.org/core/2017/10/31/changed-behaviour-of-esc_sql-in-wordpress-4-8-3/

@danielbachhuber danielbachhuber requested a review from gitlost Nov 10, 2017

@danielbachhuber danielbachhuber added this to the 1.0.5 milestone Nov 10, 2017

@danielbachhuber

This comment has been minimized.

Show comment
Hide comment
@danielbachhuber

danielbachhuber Nov 10, 2017

Member

Requesting a second pair of eyes from @gitlost

Member

danielbachhuber commented Nov 10, 2017

Requesting a second pair of eyes from @gitlost

@danielbachhuber danielbachhuber added the bug label Nov 10, 2017

@gitlost

Looks the biz to me - thanks @mullnerz !

@gitlost gitlost merged commit f593506 into wp-cli:master Nov 10, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@danielbachhuber danielbachhuber changed the title from Fix: Changed behaviour of esc_sql() in WordPress 4.8.3 to Use `$wpdb->remove_placeholder_escape()` when exporting in WP 4.8.3 and greater Nov 10, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment