Make WordPress Core

Opened 10 years ago

Closed 10 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 10 years ago.
31067.2.patch (751 bytes) - added by tyxla 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.

Download all attachments as: .zip

Change History (14)

@ChriCo
10 years ago

#1 @SergeyBiryukov
10 years ago

  • Milestone changed from Awaiting Review to 4.2

#2 @SergeyBiryukov
10 years ago

  • Milestone changed from 4.2 to 4.1.1

#3 @SergeyBiryukov
10 years ago

  • Keywords has-patch needs-unit-tests added

#4 @ChriCo
10 years ago

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

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

#5 @tyxla
10 years ago

  • Keywords needs-unit-tests removed

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

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

Reopening for 4.1.1.

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

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

#12 @dd32
10 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.