#25834 closed defect (bug) (fixed)
WP_Date_Query not allowed values possible
Reported by: |
|
Owned by: |
|
---|---|---|---|
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)
Change History (44)
#3
@
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:
↓ 8
@
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
@
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 ) ) */
#8
in reply to:
↑ 4
;
follow-up:
↓ 9
@
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
@
11 years ago
Replying to SergeyBiryukov:
WP_Date_Query::get_sql()
is supposed to return a string, so returning aWP_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
@
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
@
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:
↓ 15
@
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:
- validating day + month + year ( month: min 1 to max 12 && day: min (of month of year) + max (of month of year) ).
- 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 )?
#14
@
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..
@
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
@
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
@
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
@
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
@
11 years ago
- Keywords 4.0-early added
- Milestone changed from 3.9 to Future Release
Let's regroup on these early 4dotOh
#19
@
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...
#21
@
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
@
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
@
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.
#24
@
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 callingtrigger_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
@
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
#28
@
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
#31
@
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 thesprintf()
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/
?
#32
@
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:
↓ 34
@
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
@
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.
#39
@
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?
What would be the expected result?