Skip to content

Quote SQL values in --regex code#59

Merged
gitlost merged 8 commits intomasterfrom
58-fix-regex-table-quoting
Jan 24, 2018
Merged

Quote SQL values in --regex code#59
gitlost merged 8 commits intomasterfrom
58-fix-regex-table-quoting

Conversation

@schlessera
Copy link
Member

Adds esc_sql_value() method that tries to make an educated guess about whether the value should be quoted or not.

Uses a list of reserved keywords from the Drupal project found here: https://www.drupal.org/docs/develop/coding-standards/list-of-sql-reserved-words

Fixes #58

Adds `esc_sql_value()` method that tries to make an educated guess about whether the value should be quoted or not.

Uses a list of reserved keywords from the Drupal project found here: https://www.drupal.org/docs/develop/coding-standards/list-of-sql-reserved-words

Fixes #58
@schlessera schlessera added this to the 1.1.5 milestone Jan 23, 2018
@schlessera schlessera requested a review from a team January 23, 2018 20:18
@gitlost
Copy link
Contributor

gitlost commented Jan 23, 2018

Sorry I'm not getting this, all that needs to be done is to quote the esc_sql(). It's just looking for the value in the primary index and didn't allow for string primary keys (my mistake).

Edit: actually it would be best if it only quoted if not a number to avoid MySQL's implicit type conversion.

@schlessera
Copy link
Member Author

Yes, you are right. I built a general value quoting mechanism, but we're only dealing with a primary key here.

I'll remove all unneeded processing for now, as it is complexity we don't need to deal with yet.

@gitlost
Copy link
Contributor

gitlost commented Jan 24, 2018

Okay the tests are failing due to wp-cli/wp-cli#4624. I'll do a PR very shortly to adjust them.

`updated_at` timestamp NOT NULL DEFAULT CURRENT_TIMESTAMP ON UPDATE CURRENT_TIMESTAMP,
PRIMARY KEY (`name`)
) ENGINE=InnoDB;
INSERT INTO `wp_123_test` VALUES ('test_val','off','2016-11-15 14:41:33','2016-11-15 21:41:33');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need further test keys here, eg

INSERT INTO `wp_123_test` VALUES ('123.','off','2016-11-15 14:41:33','2016-11-15 21:41:33');
INSERT INTO `wp_123_test` VALUES ('quote\'quote','off','2016-11-15 14:41:33','2016-11-15 21:41:33');
INSERT INTO `wp_123_test` VALUES ('0','off','2016-11-15 14:41:33','2016-11-15 21:41:33');
INSERT INTO `wp_123_test` VALUES ('','off','2016-11-15 14:41:33','2016-11-15 21:41:33');

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added.

private static function esc_sql_value( $values ) {
$quote = function ( $v ) {
// Don't quote numeric values to MySQL's implicit type conversion.
if ( is_numeric( $v ) ) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer to only not quote decimal integer values rather than general numerics as PHP's definition may or may not match MySQL's definition. Highly unlikely and haven't been able to make it fail (!) but maybe just:

if ( '' !== $v && strlen( $v ) === strspn( $v, '0123456789' ) ) {

Tables with anything other than ints or strings as primary keys would be very rare anyway.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree, but used a different check.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very modern!

}

// Put any string values between single quotes.
return "'" . str_replace( "'", "''", esc_sql( $v ) ) . "'";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The str_replace() should not be there.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed.

*/
private static function esc_sql_value( $values ) {
$quote = function ( $v ) {
// Don't quote numeric values to MySQL's implicit type conversion.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo missing "avoid".

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gitlost gitlost merged commit b447337 into master Jan 24, 2018
@gitlost gitlost deleted the 58-fix-regex-table-quoting branch January 24, 2018 15:27
danielbachhuber pushed a commit that referenced this pull request Nov 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants