Opened 10 years ago
Closed 10 years ago
#31067 closed defect (bug) (fixed)
WP_Date_Query - broken hour-validation
Reported by: |
|
Owned by: |
|
---|---|---|---|
Milestone: | 4.1.1 | Priority: | normal |
Severity: | normal | Version: | 4.1 |
Component: | Query | Keywords: | has-patch commit fixed-major |
Focuses: | Cc: |
Description
It seems, there is still an error in the implementation of #25834:
$min_max_checks['hour'] = array( 'min' => 1, 'max' => 23 );
This means, that 0:00 o'clock (midnight) is invalid. Shouldn't the 'min' for 'hour'-validation 0?
Attachments (2)
Change History (14)
@
10 years ago
Updating test_validate_date_values_hour()
unit test to support 0
as a valid hour value. Removing the string from invalid value checks, as a string will evaluate as 0
, which is now actually a valid value.
#6
@
10 years ago
ChriCo - Thanks for the report. And tyxla - thanks for the test update, though in this case we can't just remove the string check; the fact that it's failing suggests a further bug. We can work around the 0
/string issue by checking is_numeric()
separately. Fix coming up.
#7
@
10 years ago
- Owner set to boonebgorges
- Resolution set to fixed
- Status changed from new to closed
In 31251:
#8
@
10 years ago
- Keywords commit fixed-major added
- Resolution fixed deleted
- Status changed from closed to reopened
Reopening for 4.1.1.
#9
@
10 years ago
Why adding an is_numeric-Check here, when already one is there ?
Maybe we should also consider, if its a good way to move some checks out of the class and to the formattings.php. Something like this:
/** * Verifies that the given value is a valid hour. * * @since 4.1.1 * * @param int|string $value * @return bool true|false */ function is_hour( $value ) { if ( ! is_numeric( $value ) ) { return false; } if ( $value < 0 || $value > 23 ) { return false; } return true; }
same for: year, day, dayofweek, dayofyear, month, minute, second and week.
Week and dayofyear needs a second param "year" for a strict check.
#10
@
10 years ago
Why adding an is_numeric-Check here, when already one is there ?
The one you linked to only checks the 'year'. Perhaps the 'year' one is no longer needed, but this needs testing.
Maybe we should also consider, if its a good way to move some checks out of the class and to the formattings.php.
It seems a little bit verbose to have separate functions for all of this - it's not easy to see how they'd be used anywhere else in WordPress itself - but it might be worth considering having a single date validation function. If you think this is worth pursuing, please feel free to open an enhancement ticket.
#11
@
10 years ago
The one you linked to only checks the 'year'. Perhaps the 'year' one is no longer needed, but this needs testing.
Yep you're right, so the year can be removed in my second link.
It seems a little bit verbose to have separate functions for all of this - it's not easy to see how they'd be used anywhere else in WordPress itself - but it might be worth considering having a single date validation function. If you think this is worth pursuing, please feel free to open an enhancement ticket.
Yes and no. This "function" was a simple example for extracting the validation to a reusable component which can be tested with Unit Tests. The best thing would be a Validation-/Sanitizing-API which can be used in other situations too.
The hour, day, month, ... is nothing else than a "is_between"-check:
function is_between( $value, $args ) { $default_args = array( 'min' => 0, 'max' => PHP_INT_MAX, 'inclusive => TRUE ); $args = wp_parse_args( $args, $default_args ); if ( (bool) $args[ 'inclusive' ] ) { if ( $args[ 'min' ] > $value || $value > $args[ 'max' ] ) { return false; } } else { if ( $args[ 'min' ] >= $value || $value >= $args[ 'max' ] ) { return false; } } return true; }
The current "validation"-implementation in WP_Date_Query is way to complex and are just special validations for date-queries.
SergeyBiryukov
You're right. Go ahead and do so. :-)