Make WordPress Core

Opened 9 years ago

Closed 9 years ago

#31067 closed defect (bug) (fixed)

WP_Date_Query - broken hour-validation

Reported by: chrico's profile ChriCo Owned by: boonebgorges's profile boonebgorges
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)

31067.patch (434 bytes) - added by ChriCo 9 years ago.
31067.2.patch (751 bytes) - added by tyxla 9 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.

Download all attachments as: .zip

Change History (14)

@ChriCo
9 years ago

#1 @SergeyBiryukov
9 years ago

  • Milestone changed from Awaiting Review to 4.2

#2 @SergeyBiryukov
9 years ago

  • Milestone changed from 4.2 to 4.1.1

#3 @SergeyBiryukov
9 years ago

  • Keywords has-patch needs-unit-tests added

#4 @ChriCo
9 years ago

SergeyBiryukov
You're right. Go ahead and do so. :-)

@tyxla
9 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.

#5 @tyxla
9 years ago

  • Keywords needs-unit-tests removed

#6 @boonebgorges
9 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 @boonebgorges
9 years ago

  • Owner set to boonebgorges
  • Resolution set to fixed
  • Status changed from new to closed

In 31251:

WP_Date_Query date validation should not fail for hour = 0.

Props ChriCo, tyxla.
Fixes #31067 for trunk.

#8 @boonebgorges
9 years ago

  • Keywords commit fixed-major added
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening for 4.1.1.

#9 @ChriCo
9 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 @boonebgorges
9 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 @ChriCo
9 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.

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

#12 @dd32
9 years ago

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

In 31377:

WP_Date_Query date validation should not fail for hour = 0.

Props ChriCo, tyxla.
Merges [31251] to the 4.1 branch.
Fixes #31067.

Note: See TracTickets for help on using tickets.