WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 4 years ago

Last modified 4 years ago

#28063 closed enhancement (fixed)

'dayofweek' deviates from ISO-8601

Reported by: mboynes Owned by: boonebgorges
Milestone: 4.1 Priority: normal
Severity: normal Version: 3.7
Component: Query Keywords: needs-patch
Focuses: Cc:

Description

In WP_Date_Query, specifying 'dayofweek' uses MySQL's DAYOFWEEK function, which adheres to the ODBC standard where Sunday=1 and Saturday=7. There are two problems here:

  1. This deviates from the much-more-widely-accepted ISO-8601 standard, where Monday=1 and Sunday=7.
  2. PHP's date() function does not natively support this format, so it leaves it up to the developer to do the work. Since none of this is documented in the source or codex, it also means that developers have to figure it out for themselves. A developer might use date()'s 'N' or 'w' format, not even considering that WordPress uses yet a third format.

I propose that we change this to use the ISO-8601 numeric representation of the day of the week.

Of course, the most significant consideration in such a change is that this would be a "breaking" change. Queries using 'dayofweek' would start returning results which were off by a day. I'm attaching a patch (dayofweek.diff) which makes this breaking change, if we want to go that route. Another possible route would be to deprecate 'dayofweek' (but continue supporting it) and add 'weekday'. This would allow Core to embrace the ISO standard without a breaking change. I'm attaching a separate patch (weekday.diff) which does this.

Attachments (3)

28063-dayofweek.diff (2.1 KB) - added by mboynes 5 years ago.
Converting 'dayofweek' to use ISO-8601 standard
28063-weekday.diff (2.4 KB) - added by mboynes 5 years ago.
Adding an additional argument to WP_Date_Query for 'weekday', an ISO-8601-compliant way to query day of week
28063-weekday-2.diff (2.6 KB) - added by mboynes 5 years ago.
Renaming PHPUnit test for 'weekday' implementation

Download all attachments as: .zip

Change History (10)

@mboynes
5 years ago

Converting 'dayofweek' to use ISO-8601 standard

@mboynes
5 years ago

Adding an additional argument to WP_Date_Query for 'weekday', an ISO-8601-compliant way to query day of week

@mboynes
5 years ago

Renaming PHPUnit test for 'weekday' implementation

#1 @mboynes
5 years ago

  • Component changed from Date/Time to Query

#2 follow-up: @boonebgorges
4 years ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to Future Release
  • Version set to 3.7

Thanks for the patches, and ugh. :)

You are correct that we can't change the behavior of the existing parameter. 'dayofweek' has to stay the way it is.

I lean toward adding a new parameter, and at the same time not deprecating 'dayofweek'. I don't think there's anything wrong with having formal support for both the MySQL standard and the ISO standard. So I definitely would not remove the documentation or unit test for 'dayofweek' as you've done in your patches. If anything, I would expand the relevant documentation to make it clearer what the param does. We should do this regardless of whether we add another param.

I'm not thrilled with the choice of 'weekday' for the name of the ISO-compliant param. One, it's got a connotation of M-F. Two, as noted in your patch, MySQL's WEEKDAY sets Monday to index 0 and Tuesday to 1. (Like I said, ugh.) How about something like 'dayofweek_iso'?

#3 in reply to: ↑ 2 @mboynes
4 years ago

Replying to boonebgorges: Hey Boone, thanks for jumping on this!

I lean toward adding a new parameter, and at the same time not deprecating 'dayofweek'. I don't think there's anything wrong with having formal support for both the MySQL standard and the ISO standard. So I definitely would not remove the documentation or unit test for 'dayofweek' as you've done in your patches. If anything, I would expand the relevant documentation to make it clearer what the param does. We should do this regardless of whether we add another param.

You make some great points here and I think this is worth a discussion. I would offer that "having formal support for both the MySQL standard and the ISO standard" is making an option and not a decision. As WordPress grows, I think it's important to consider these MySQL-centric features, as they make it more difficult to leverage new technologies (for instance, I stumbled across this while writing an Elasticsearch replacement for WP_Query). In short, I think that deprecating dayofweek is making a decision in favor of an international standard.

I'm not thrilled with the choice of 'weekday' for the name of the ISO-compliant param. One, it's got a connotation of M-F. Two, as noted in your patch, MySQL's WEEKDAY sets Monday to index 0 and Tuesday to 1. (Like I said, ugh.) How about something like 'dayofweek_iso'?

I completely agree, weekday is not a good choice for the reasons you name, thanks for catching that. Naming it dayofweek_iso could work, depending on the result of the above discussion. dayofweek_iso makes perfect sense if we support both, but if we do deprecate dayofweek, dayofweek_iso seems second-class next to the deprecated dayofweek.

I'd be very interested to hear other opinions on this too. Thanks!

#4 @boonebgorges
4 years ago

  • Milestone changed from Future Release to 4.1
  • Owner set to boonebgorges
  • Status changed from new to accepted

I would offer that "having formal support for both the MySQL standard and the ISO standard" is making an option and not a decision.

If we were building this from scratch, I would say yes. But given that it already exists, we really can't change it - this would break existing sites.

Let's do this one way or another for 4.1. I'll hold off on implementing for a bit to see if anyone else wants to chime in.

#5 @boonebgorges
4 years ago

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

In 30142:

Introduced dayofweek_iso time param for WP_Date_Query.

The initial dayofweek param sets day 1 to Sunday. This is out of step with
ISO standards, which calls Monday day 1. To maintain backward compatibility
with the existing parameter, we introduce the new dayofweek_iso for the
new, more compliant param.

Props mboynes.
Fixes #28063.

#6 @DrewAPicture
4 years ago

In 30150:

Fix a couple of spacing and wrapping indents in the WP_Date_Query::__construct() hash notation.

See [30142]. See #28063.

#7 @nacin
4 years ago

I like dayofweek_iso and probably wouldn't want to break this even if we "could."

Note: See TracTickets for help on using tickets.