Make WordPress Core

Opened 11 years ago

Closed 10 years ago

Last modified 10 years ago

#25834 closed defect (bug) (fixed)

WP_Date_Query not allowed values possible

Reported by: chrico's profile ChriCo Owned by: boonebgorges's profile boonebgorges
Milestone: 4.1 Priority: normal
Severity: normal Version: 3.7
Component: Query Keywords: has-patch needs-docs
Focuses: Cc:

Description

There are several Parameter-Bugs in the new WP_Date_Query-Class.

First of all, there is no correct validation of the following Parameters:

  • month / monthnum
  • week / w
  • second
  • minute

Example:

month > 12, week > 52, hour > 23, Minute > 59, Second > 59

$date_query = new WP_Date_Query( array(
  'week'  => 53,
  'month'  => 13,
  'hour'  => 24,
  'minute' => 60,
  'second'  => 59,
  'dayofyear' => 1000,
  'dayofweek' => 10
));
echo $date_query->get_sql(); 
/*
AND ( 
  ( MONTH( post_date ) = 13 
  AND WEEK( post_date, 1 ) = 53 
  AND DAYOFYEAR( post_date ) = 1000 
  AND DAYOFWEEK( post_date ) = 10 
  AND DATE_FORMAT( post_date, '%H.%i%s' ) = 24.605900 ) 
)
*/

In the date.php-File Codeblock and Documentation are also listed the wrong values:

  • minute (int) - Minute (from 0 to 60).
  • second (int) - Second (0 to 60).

Both of them should be: 0 to 59

Attachments (3)

#25834_created_validation_of_date_query-values_and_unittest.patch (10.3 KB) - added by ChriCo 11 years ago.
added validation and unit-test for validation the date_query-values
#25834_(new)_validation_of_date_query-values_and_unittesting.patch (15.4 KB) - added by ChriCo 11 years ago.
new Version of Date_Query-Validation: * added missing before/after, * moved validation to loop in construct(), * added support for compare (IN, NOT IN, BETWEEN, NOT BETWEEN ) and date-values as array
25834.patch (13.0 KB) - added by boonebgorges 10 years ago.

Download all attachments as: .zip

Change History (44)

#1 @toscho
11 years ago

  • Cc info@… added

#2 follow-up: @SergeyBiryukov
11 years ago

  • Component changed from Date/Time to Query

month > 12, week > 52, hour > 23, Minute > 59, Second > 59

What would be the expected result?

#3 @ocean90
11 years ago

  • Version changed from 3.7.1 to 3.7

and ​Documentation are also listed the wrong values:

I have updated the codex, but have in mind, that you can update he codex by yourself.

#4 in reply to: ↑ 2 ; follow-up: @toscho
11 years ago

Replying to SergeyBiryukov:

month > 12, week > 52, hour > 23, Minute > 59, Second > 59

What would be the expected result?

Sanitize or, better, return a WP_Error with an explanation.

#6 @ChriCo
11 years ago

It is also possible to enter negativ values:

$date_query = new WP_Date_Query( array(
  'week'  => -1,
  'month'  => -1,
  'hour'  => -1,
  'minute' => -1,
  'second'  => -1,
  'dayofyear' => -1,
  'dayofweek' => -1
));
echo $date_query->get_sql(); 
/*
AND ( 
 ( MONTH( post_date ) = -1 
 AND WEEK( post_date, 1 ) = -1 
 AND DAYOFYEAR( post_date ) = -1 
 AND DAYOFWEEK( post_date ) = -1 
 AND DATE_FORMAT( post_date, '%H.%i%s' ) = -1.000000 ) 
)
*/

#7 @emzo
11 years ago

  • Cc wordpress@… added

#8 in reply to: ↑ 4 ; follow-up: @SergeyBiryukov
11 years ago

Replying to toscho:

Sanitize or, better, return a WP_Error with an explanation.

WP_Date_Query::get_sql() is supposed to return a string, so returning a WP_Error object might break some code.

#9 in reply to: ↑ 8 @toscho
11 years ago

Replying to SergeyBiryukov:

WP_Date_Query::get_sql() is supposed to return a string, so returning a WP_Error object might break some code.

Which code? These cases are broken already, we just give no useful response. :)

With an error object we could do something useful, e.g.:

$date_query = new WP_Date_Query( array( /* data */ ) );

if ( is_wp_error( $date_query ) )
    // stop here, log the error message

#10 @wonderboymusic
11 years ago

  • Keywords needs-patch needs-unit-tests added
  • Milestone changed from Awaiting Review to 3.9

This ticket was mentioned in IRC in #wordpress-dev by cs_shadow. View the logs.


11 years ago

#12 @Viper007Bond
11 years ago

I'm not convinced that this change would be a good one.

What should happen when you pass an out of range date value to a WP_Query call?

Should the invalid parameter be discarded and the query run without it? I strongly think it shouldn't as it would make debugging a nightmare. It should fail to return any posts as it does now.

Or perhaps the whole WP_Query call is aborted, but that would stop the SQL filters from running where potentially it could be fixed or filtered or whatever.

I'm recommending wontfix here. We didn't validate the data parameters before I wrote WP_Date_Query either, nor do we currently validate many other parameters -- we only sanitize and escape them.

#13 follow-up: @ChriCo
11 years ago

What should happen when you pass an out of range date value to a WP_Query call?

As Toscho (https://core.trac.wordpress.org/ticket/25834#comment:9) already said, returning or giving at least an Error-Message (could also trigger_error() to show a warning when WP_DEBUG is true).

I'm a little bit disappointed of your recommendation. It's harder to debug an unexpected result and not validated parameters, than debugging a meaningful error-message thrown by WP_Error or trigger_error().

Basic validation could be something like:

// checking the days of year
if( array_key_exists( 'dayofyear', $date_query ) ){
	$year               = array_key_exists( 'year', $date_query ) ? $date_query[ 'year' ] : date( 'Y' );
	$max_days_in_year   = date( "z", mktime( 0, 0, 0, 12, 31, $year ) ) + 1;
	$filter_args = array(
		'options' => array(
			'min_range' => 1,
			'max_range' => $max_days_in_year
		)
	);
	$valid = filter_var(
		$date_query[ 'dayofyear' ],
		FILTER_VALIDATE_INT,
		$filter_args
	);
}

// checking day of week
if( array_key_exists( 'dayofweek', $date_query ) ){
	$filter_args = array(
		'options' => array(
			'min_range' => 1,
			'max_range' => 6
		)
	);
	$valid = filter_var(
		$date_query[ 'dayofweek' ],
		FILTER_VALIDATE_INT,
		$filter_args
	);
}

And when switching min-Support of PHP to 5.3 you can also change the whole logic to:

function check_date_value_by_format( $date_value, $format ) {
	$date_time = DateTime::createFromFormat( $format, $date_value);
	$is_valid = $date_time && $date_time->format( $format ) == $date_value;
	if( !$is_valid && WP_DEBUG ) {
		$msg = sprintf( 
			'The entered date_value <code>%s</code> is not valid. Excepted <code>%s</code> by validating with format <code>%s</code>.'
			$date_value,
			$date_time->( $format ),
			$format
		);
		trigger_error( $msg, E_USER_ERROR );
	}
	return $is_valid;
}

// week
$format = 'w';
check_date_value_by_format( 53, $format ); // invalid
check_date_value_by_format( 52, $format ); // valid

// second
$format = 's';
check_date_value_by_format( 60, $format ); // invalid
check_date_value_by_format( 59, $format ); // valid

// and so on..

The only problem i see is, validating the day-month-year-combinations (maybe with the PHP check_date()-Function). There you have following scenarios:

  1. validating day + month + year ( month: min 1 to max 12 && day: min (of month of year) + max (of month of year) ).
  2. validating day + month ( month: min 1 to max 12 && day: min (of month) + max (of month) )

But what happens on 2. when we have following values:

$day = 29;
$month = 2

In which year? Or should we just check through cal_days_in_month(); of a leap-year to verify a valid max ( 29 or 30 or 31 )?

Last edited 11 years ago by ChriCo (previous) (diff)

@ChriCo
11 years ago

added validation and unit-test for validation the date_query-values

#14 @ChriCo
11 years ago

i've attached a .diff-File for validating the date_query-values and some unit-tests.

I agree that it would be not the best solution to return an Error-Object or break the Query, but it would be nice to trigger an error (on WP_DEBUG === true) when some parameters are invalid.

The Method validate_date_values( Array $query= array() ); is a public Method, So it could be resued in other WordPress-Functions (wp_insert_post(), wp_insert_commert(), .. ) when Date-Inputs should be validated.

Edit: uploaded a new version of the validation and added some missing stuff.

Edit2: sry its monday morning and i still had no coffee..pls ignore the phpunit.xml.dist, wp-config.php, wp-config-test.php..

Last edited 11 years ago by ChriCo (previous) (diff)

@ChriCo
11 years ago

new Version of Date_Query-Validation: * added missing before/after, * moved validation to loop in construct(), * added support for compare (IN, NOT IN, BETWEEN, NOT BETWEEN ) and date-values as array

#15 in reply to: ↑ 13 @Viper007Bond
11 years ago

Replying to ChriCo:

What should happen when you pass an out of range date value to a WP_Query call?

As Toscho (https://core.trac.wordpress.org/ticket/25834#comment:9) already said, returning or giving at least an Error-Message (could also trigger_error() to show a warning when WP_DEBUG is true).

I don't know of any cases right now where WP_Query validates input values and returns an error if you supply an out of bounds value. For example roll back to version 3.6 and do WP_Query( array( 'day' => 50 ) ). Please correct me if I am mistaken though!

Changing the behavior of WP_Query to bail on an out of bounds value would break many things and I think is a very, very bad idea.

In my opinion if WP_DEBUG is not enabled, then we should do absolutely nothing and run the query with the values supplied, as we do today.

However I can certainly see the advantage of throwing a non-blocking error if and only if WP_DEBUG is enabled and I would be in favor of that. It shouldn't stop the query from being run or anything else, just throw a PHP error. This could help in figuring out why a query is returning no results.

#16 @ChriCo
11 years ago

I don't know of any cases right now where WP_Query validates input values and returns an error if you supply an out of bounds value.

Never said something like that. I think you misunderstood me.

Please read my patch.

I only run the validate_date_values()-Method on construct() to check the given query-values and return true or false (i do not break the loop on error!).
When WP_DEBUG is defined and true than the trigger_error will print out a E_USER_NOTICE. I don't manipulate the given values (not like the validate_column()-Method).
I've moved the WP_DEBUG-check to the validate_date_values()-Method, to make it reusable for other functions (wp_insert_post, wp_insert_commert(), ... ).

#17 @Viper007Bond
11 years ago

Okay, then I can see triggering a notice if a value is wrong. That would indeed be helpful to developers.

I have some issues with the actual patch though. For example validate_date_values() shouldn't run unless WP_DEBUG is enabled. There's no reason to validate the values in a production environment, especially if no notice is going to be triggered. There's also some various coding standards issues and stuff like that.

I'll throw together an updated patch.

#18 @wonderboymusic
11 years ago

  • Keywords 4.0-early added
  • Milestone changed from 3.9 to Future Release

Let's regroup on these early 4dotOh

#19 @wonderboymusic
11 years ago

This is too late in the cycle, we need to spend some more time in Unit Tests and make sure enough people properly understand how all of this stuff works before tossing it all in. I personally moved a lot of the Query tickets into 3.9 and want to see resolutions as much as anyone else...

#20 @wonderboymusic
11 years ago

  • Milestone changed from Future Release to 4.0

#21 @DrewAPicture
11 years ago

  • Keywords 4.1-early added; 4.0-early removed
  • Milestone changed from 4.0 to Future Release

No activity in two months. Punting.

#22 @ChriCo
10 years ago

don't really know where the problem is. The solution was posted as diff 7 months ago. Maybe it will come with WordPress 5.0 ?

#23 @helen
10 years ago

Sounded like Viper007Bond (as the originating author of WP_Date_Query) had some thoughts about the patch which could use some iterating on.

@boonebgorges
10 years ago

#24 @boonebgorges
10 years ago

  • Keywords has-patch added; needs-patch needs-unit-tests 4.1-early removed
  • Milestone changed from Future Release to 4.1
  • Owner set to boonebgorges
  • Status changed from new to accepted

I am a fan of this approach and a fan of the patch. Silently sanitizing is a bad idea because it can return improper results (if I request stuff from the 13th month, the correct behavior is to give back nothing). Throwing a WP_Error is, as noted above, not backward compatible, and may cause fatal errors on existing installations. So I like the idea of throwing debug notices.

25834.patch is a cleaned up version of ChriCo's second patch. I've made the following changes:

  • Instead of checking WP_DEBUG and calling trigger_notice(), use the _doing_it_wrong() wrapper. It serves the same purpose, and is in keeping with behavior elsewhere in WP. It also plays more nicely with our unit tests (@expectedIncorrectUsage annotation instead of suppressing errors with @).
  • Moved unit tests to the appropriate file.
  • Changed the behavior of 'before' and 'after' validation so that correct errors are generated when these are used in combination with each other and with other function params (plus a new unit test to demonstrate the combinations).
  • Coding standards, function naming, inline docs, etc.

I think this is a very nice improvement. Posting the patch here because (a) Viper007Bond said he had some ideas for improving and I want to make sure he gets a chance to look at it, and (b) I may wait until after #29822 has been settled, because the validation is going to have to work a bit differently with nested params.

#25 @DrewAPicture
10 years ago

  • Keywords needs-docs added

Couple of docs notes on 25834.patch:

  • We'll need translator comments for the specifiers in the $min_max_msg string
  • A comment or two in or around the foreach ( $min_max_checks as $key => $check ) { block would be helpful
  • The return description on validate_date_values() is incomplete
  • Probably also worth mentioning in the long description for WP_Date_Query::__construct() that values will be sanitized and what will happen if invalid values are passed. Doesn't necessarily need to be super granular but we do need to specify the expected result either way

#26 @ChriCo
10 years ago

@boonebgorges THX! :-)

#27 @boonebgorges
10 years ago

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

In 29925:

Throw notices when invalid date values are passed to WP_Date_Query.

_doing_it_wrong() notices are now generated when passing out-of-range values
(month=13) or invalid dates (2014-02-29).

Includes unit tests.

Props ChriCo.
Fixes #25834.

#28 @jorbin
10 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

A number of unit tests are broken from this change. see: https://travis-ci.org/aaronjorbin/develop.wordpress/builds/38195399

1) Tests_Canonical::test with data set #47 ('/2012/11/51/', '/2012/11/')
Unexpected incorrect usage notice for WP_Date_Query
/home/travis/build/aaronjorbin/develop.wordpress/tests/phpunit/includes/testcase.php:223
/home/travis/build/aaronjorbin/develop.wordpress/tests/phpunit/includes/testcase.php:59
/home/travis/build/aaronjorbin/develop.wordpress/tests/phpunit/tests/canonical.php:88
/home/travis/.phpenv/versions/5.2.17/bin/phpunit:46
2) Tests_Canonical_HTTPS::test with data set #47 ('/2012/11/51/', '/2012/11/')
Unexpected incorrect usage notice for WP_Date_Query
/home/travis/build/aaronjorbin/develop.wordpress/tests/phpunit/includes/testcase.php:223
/home/travis/build/aaronjorbin/develop.wordpress/tests/phpunit/includes/testcase.php:59
/home/travis/build/aaronjorbin/develop.wordpress/tests/phpunit/tests/canonical.php:88
/home/travis/.phpenv/versions/5.2.17/bin/phpunit:46
3) Tests_Meta_Query::test_invalid_query_clauses
Failed asserting that Array (

0 => 'foo'
'relation' => 'OR'

) is identical to Array ().
/home/travis/build/aaronjorbin/develop.wordpress/tests/phpunit/tests/meta/query.php:43
/home/travis/.phpenv/versions/5.2.17/bin/phpunit:46
4) Tests_Query_Conditionals::test_bad_dates
Unexpected incorrect usage notice for WP_Date_Query
/home/travis/build/aaronjorbin/develop.wordpress/tests/phpunit/includes/testcase.php:223
/home/travis/build/aaronjorbin/develop.wordpress/tests/phpunit/includes/testcase.php:59
/home/travis/build/aaronjorbin/develop.wordpress/tests/phpunit/tests/query/conditionals.php:37
/home/travis/.phpenv/versions/5.2.17/bin/phpunit:46
5) Tests_Query_Results::test_query_is_date
Unexpected incorrect usage notice for WP_Date_Query
/home/travis/build/aaronjorbin/develop.wordpress/tests/phpunit/includes/testcase.php:223
/home/travis/build/aaronjorbin/develop.wordpress/tests/phpunit/includes/testcase.php:59
/home/travis/.phpenv/versions/5.2.17/bin/phpunit:46
6) Tests_Query_VerbosePageRules::test_bad_dates
Unexpected incorrect usage notice for WP_Date_Query
/home/travis/build/aaronjorbin/develop.wordpress/tests/phpunit/includes/testcase.php:223
/home/travis/build/aaronjorbin/develop.wordpress/tests/phpunit/includes/testcase.php:59
/home/travis/build/aaronjorbin/develop.wordpress/tests/phpunit/tests/query/conditionals.php:37
/home/travis/.phpenv/versions/5.2.17/bin/phpunit:46

#29 @boonebgorges
10 years ago

Yup, just noticed this myself. Working on it. Thanks, jorbin.

#30 @boonebgorges
10 years ago

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

In 29932:

Add expectedIncorrectUsage flags for unit tests that generate invalid dates.

Since [29925], passing an invalid date to WP_Date_Query will generate a
_doing_it_wrong() notice. The current changeset adds the
@expectedIncorrectUsage flag to those existing unit tests that generate
invalid dates, such as those that test canonical redirect and is_404()
conditionals.

Fixes #25834.

#31 @dd32
10 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

[29925] introduced a bunch of strings which needlessly use HTML within the translation strings, and in a few places %d is used when I think %s should probably be favoured instead.

For example:

$min_max_msg = __( 'Invalid value <code>%1$s</code> for <strong>%2$s</strong>. Excepted value should between <code>%3$d</code> and </code>%4$d</code>.' ); 
....
$error = sprintf( 
    $min_max_msg, 
    esc_html( $date_query[ $key ] ), 
    $key, 
    $check['min'], 
    $check['max'] 
); 

should

  • include __() in the sprintf() call since it's only used once (and even if it was used multiple times, it's far easier to read in-line)
  • Use '<code>' . esc_html( $date_query[ $key ] ) . '</code>' in the replacement instead of including HTML in the translation
  • use esc_html() over all of the placeholders, even if they're believed to be safe
  • use %s for all placeholders (with numbered args) instead of %d just to show any invalid data thats passed through to the function (rather than them being forced to ints) - If it's not possible for it to be a non-int at this point, that's fine, but I can't tell while within this function if the data has been validated/reformed at a higher layer
  • Typo: s/Excepted/Expected/ ?
Last edited 10 years ago by dd32 (previous) (diff)

#32 @boonebgorges
10 years ago

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

[30300] missed the ticket. Thanks for the feedback, dd32 :)

#33 follow-up: @Offereins
10 years ago

'Invalid value %1$s for %2$s. Expected value should between %3$s and %4$s.'

Should read

'Invalid value %1$s for %2$s. Expected value should be between %3$s and %4$s.'

Right?

#34 in reply to: ↑ 33 @boonebgorges
10 years ago

Replying to Offereins:

'Invalid value %1$s for %2$s. Expected value should between %3$s and %4$s.'

Should read

'Invalid value %1$s for %2$s. Expected value should be between %3$s and %4$s.'

Right?

Right. Stupid language. Thanks, Offereins.

#35 @boonebgorges
10 years ago

In 30301:

Correct grammar error in WP_Date_Query error message.

Props Offereins.
See #25834.

#36 @ramiy
10 years ago

Related: #30612

#37 @boonebgorges
10 years ago

In 30750:

Use wp_checkdate() when checking date validity in WP_Date_Query.

See #25834.

#38 @boonebgorges
10 years ago

In 30751:

Remove <code> tags from translatable string in WP_Date_Query.

This was missed in [30300]. See #25834.

Props ramiy.
Fixes #30612.

#39 @ChriCo
10 years ago

It seems, there is still an error in this implementation:

 $min_max_checks['hour'] = array( 
 	'min' => 1, 
 	'max' => 23 
); 

this means, that 0:00 o'clock (midnight) is invalid. Don't be afraid of ghosts and witches. :)

Shouldn't the 'min' for 'hour'-validation 0?

#40 @SergeyBiryukov
10 years ago

This ticket was closed on a completed milestone, please create a new one if there are any remaining issues.

Note: See TracTickets for help on using tickets.