Make WordPress Core

Opened 10 years ago

Closed 9 years ago

Last modified 7 years ago

#28344 closed enhancement (wontfix)

Remove "Week Starts On" setting UI

Reported by: markjaquith's profile markjaquith Owned by: ocean90's profile ocean90
Milestone: Priority: low
Severity: normal Version:
Component: Options, Meta APIs Keywords: has-patch
Focuses: ui, administration Cc:

Description

https://i.cloudup.com/4Ui5_cnPlq-3000x3000.png

This shouldn't be a UI setting. Let's do as we've done with other settings, like charset, and hide it if it's set to the default setting (Monday).

Attachments (8)

28344.patch (450 bytes) - added by swissspidy 9 years ago.
Group locale settings
28344.2.patch (936 bytes) - added by swissspidy 9 years ago.
28344.3.patch (1.1 KB) - added by swissspidy 9 years ago.
28344.4.patch (1.1 KB) - added by swissspidy 9 years ago.
28344.diff (5.6 KB) - added by swissspidy 9 years ago.
28344.2.diff (5.8 KB) - added by swissspidy 9 years ago.
28344.3.diff (6.3 KB) - added by swissspidy 9 years ago.
28344.5.patch (5.9 KB) - added by ocean90 9 years ago.

Download all attachments as: .zip

Change History (62)

#1 @ericlewis
10 years ago

  • Component changed from Administration to Options, Meta APIs
  • Focuses ui added; administration removed
  • Keywords needs-patch added
  • Type changed from defect (bug) to enhancement

#2 @SergeyBiryukov
10 years ago

  • Focuses administration added

#3 follow-ups: @ramiy
10 years ago

What? Why?

Nacin wrote in the "Internationalization goals for 4.0" that we should be able to switch a language from the general settings screen. If i want to change from english to hebrew, i will need to be able to switch the week-starts from monday to sunday.

This is an important setting to few non-christian countries. If you do want to remove it, let's do it in a smart way.

We can move this setting to the local translation file. Right now, each language can set it's "text direction" (LTR/RTL) and there are few other settings set from there. I think that if you want to hide this setting, it's better to move it to there.

#4 in reply to: ↑ 3 @ericlewis
10 years ago

I think the point is that there aren't too many places in WordPress affected by what day of the week your week starts on. The only place I can find is in the archive widget, to build the weekly archives.

Replying to ramiy:

If i want to change from english to hebrew, i will need to be able to switch the week-starts from monday to sunday.

Why? What effect does this have on your experience in WordPress?

#5 @iseulde
10 years ago

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

#6 follow-up: @mordauk
10 years ago

This affects me personally. I have an event calendar plugin that uses this setting to determine what day to show first on the calendar view. Based on the number of support tickets I get from users asking how the day can be changed to Monday (or to Sunday), I can definitely say it's used by a lot of users.

I've had users in all parts of the world want to change this away from the default, so I'm not sure basing it on the charset is sufficient.

I love the idea of simplifying WP core settings, but if I need to add a settings page to my plugin to give users the option to change this, it would make me a little sad. I certainly will if needed though.

#7 in reply to: ↑ 3 ; follow-up: @SergeyBiryukov
10 years ago

Replying to ramiy:

We can move this setting to the local translation file. Right now, each language can set it's "text direction" (LTR/RTL) and there are few other settings set from there. I think that if you want to hide this setting, it's better to move it to there.

I think that's what this ticket means. It depends on the locale, so it should be set in language files. It just doesn't have to be a UI option.

#8 in reply to: ↑ 7 @ramiy
10 years ago

Replying to SergeyBiryukov:

Replying to ramiy:

We can move this setting to the local translation file. Right now, each language can set it's "text direction" (LTR/RTL) and there are few other settings set from there. I think that if you want to hide this setting, it's better to move it to there.

I think that's what this ticket means. It depends on the locale, so it should be set in language files. It just doesn't have to be a UI option.

Sergey, i think that we both agree that if we want to remove this setting from the UI, it should be set in language files.

I also think we need to add a filter to override the local settings. For calendar plugins, as Pippin Williamson mentioned. It should be an easy to use filter, something like:

function custom_week_starts_filter() {
  return '0'; // for sunday
}
add_filter( 'week_starts_on', 'custom_week_starts_filter' );

#9 @nofearinc
10 years ago

I could imagine a use case with several people in Europe working for the US, running different languages (per country) and having to sync with "Week starts on Sunday" option. Not sure how likely it is, but given Pippin's example, I wouldn't surprise a lot (plus my default Gnome desktop calendar always starts on Sunday with no handy UI toggle which has been bugging me for years).

This ticket was mentioned in IRC in #wordpress-dev by helen. View the logs.


10 years ago

#11 @helen
10 years ago

  • Type changed from enhancement to task (blessed)

Language pack work appears to be touching on this, so marking as a task. We should also make sure that the option can be put back on the screen via hooks (per plugin concerns) and write that up.

#12 follow-ups: @nacin
10 years ago

What I want to know is why "Sunday" isn't the default in the en_US build. This is only used in core for the Calendar widget, and I cannot recall ever seeing a calendar in the U.S. that starts on a Monday. Yes, ISO sets Monday to be the first day of the week. Anyway.

Your "locale" isn't just about language; it's also about time and date formatting and also your timezone. Ideally, even if options exist for them, we cover these things harmoniously and set smart defaults. I'm not sure we can remove this for 4.0, but what I'd like to see is a new "Locale" section on General Settings that groups timezone, date/time/week, and language, which are already together. That at least helps this seem not as extraneous.

#13 @dd32
10 years ago

what I'd like to see is a new "Locale" section on General Settings that groups timezone, date/time/week, and language, which are already together.

This. So much this, let the locale set these things by default at install (like they already do for date formats) and group/bury them together as a localisation group.

#14 in reply to: ↑ 12 @alex-ye
10 years ago

Even if this option isn't used in many places in the core, I could imagine many plugins are using it like @mordauk plugin, In addition the Time\Date settings are not really related to the Language. In all Islamic websites they set this option to "Saturday" and Islamic websites could be English, Arabic...etc

Replying to nacin:

I'd like to see is a new "Locale" section on General Settings that groups timezone, date/time/week, and language, which are already together. That at least helps this seem not as extraneous.

1+

This ticket was mentioned in IRC in #wordpress-dev by DrewAPicture. View the logs.


10 years ago

#16 follow-up: @DrewAPicture
10 years ago

  • Milestone changed from 4.0 to Future Release
  • Type changed from task (blessed) to enhancement

Still looking for a consensus on the best way forward here. Also, needs a patch. Punting.

#17 @larsemil
10 years ago

+1 on moving it to locale.

if the code sets the option from the locale it will not break plugins depending on the option.

+1 on an easy filter to override the option as well.

This ticket was mentioned in Slack in #core-flow by drew. View the logs.


10 years ago

#19 in reply to: ↑ 16 @courane01
10 years ago

Replying to DrewAPicture:

Still looking for a consensus on the best way forward here. Also, needs a patch. Punting.

Hoping this is bumped up again.

#20 @swissspidy
9 years ago

I'd like to see is a new "Locale" section on General Settings that groups timezone, date/time/week, and language, which are already together. That at least helps this seem not as extraneous.

It's worth noting that the language (#29783) and timezone (#18146) should be changeable on a per user basis. It only makes sense to extend this to date format and day of week too, so let's keep that in mind.

@swissspidy
9 years ago

Group locale settings

@swissspidy
9 years ago

#21 @swissspidy
9 years ago

  • Keywords has-patch added; needs-patch removed
  • Milestone changed from Future Release to 4.4

28344.2.patch does the following:

  • Add a Locale header for locale settings
  • Hide the Week Starts On setting if it's equal to the one specified by the locale (i.e. the default is 1, and de_DE also uses 1, therefore it's hidden)

The value of the start_of_week option can be easily filtered through the pre_option_start_of_week filter. Nothing new needs to be introduced.

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

#22 follow-up: @stephenharris
9 years ago

@swissspidy I think your latest patch only does the first point(?). In any case, if this UI option is to be hidden could we provide a filter for plugin developers to 'unhide' it if they deem it relevant to their plugin, as suggested by Helen?

On another note I personally don't see the advantage in checking if the option is at its default value. Anyone who has a fresh install wont see the option to change it, and those who have already changed that setting from the default are less likely to need to change it again, but they are the only ones who will be able to see it. You also have a confusing UX of changing at setting and it disappearing.

It would be better to simply hide the setting and add a filter to unhide it.

@swissspidy
9 years ago

#23 in reply to: ↑ 22 @swissspidy
9 years ago

Replying to stephenharris:

@swissspidy I think your latest patch only does the first point(?).

It did, I just linked to the wrong attachment. My bad!

In any case, if this UI option is to be hidden could we provide a filter for plugin developers to 'unhide' it if they deem it relevant to their plugin, as suggested by Helen?
On another note I personally don't see the advantage in checking if the option is at its default value. Anyone who has a fresh install wont see the option to change it, and those who have already changed that setting from the default are less likely to need to change it again, but they are the only ones who will be able to see it. You also have a confusing UX of changing at setting and it disappearing.

It would be better to simply hide the setting and add a filter to unhide it.

You're right, this could be very confusing. 28344.3.patch introduces a new show_start_of_week_setting filter which defaults to false. That would hide the option for everyone and give plugin authors the ability to bring it back.

#24 @DrewAPicture
9 years ago

Seems to me that the precedent in removing core option UIs is to leave the option in place but remove the UI altogether. I think we should follow that. If plugins want to add it back they can do that via the settings API just like any other setting. I think adding a filter to restore the UI kind of defeats the purpose of removing it.

@swissspidy
9 years ago

#25 @swissspidy
9 years ago

That sounds reasonable as well. I created 28344.4.patch for this case. Now we need to decide which route works best.

#26 @SergeyBiryukov
9 years ago

  • Keywords commit added

#27 follow-up: @SergeyBiryukov
9 years ago

  • Keywords commit removed

I think the option should depend on a translatable string, like wp_trim_words() does.

Version 0, edited 9 years ago by SergeyBiryukov (next)

#28 in reply to: ↑ 27 @swissspidy
9 years ago

Replying to SergeyBiryukov:

Before removing the UI, we should probably make the option depend on a translatable string, see wp_trim_words() for example.

The default option is set in populate_options() in wp-admin/includes/schema.php:

/* translators: default start of the week. 0 = Sunday, 1 = Monday */
'start_of_week' => _x( '1', 'start of week' ),

Isn't this fine already or do we need a pre_option filter to bypass getting the database option?

#29 @swissspidy
9 years ago

  • Severity changed from minor to normal

#30 @dd32
9 years ago

The default option is set in populate_options() in wp-admin/includes/schema.php:
Isn't this fine already or do we need a pre_option filter to bypass getting the database option?

The problem is when the language is changed, or translation is fixed after the site is created.

We can probably just deprecate the function entirely, replace any uses of it with _x( '1', 'start of week' ) and add a pre_option filter to return the translated version for compat with plugins/themes which use the function directly.

@swissspidy
9 years ago

#31 @swissspidy
9 years ago

28344.diff implements what @dd32 suggested. What it does:

  • Adds a Locale heading to the settings in Options -> General
  • Moves the _x( '1', 'start of week' ) call from wp-admin/includes/schema.php to wp-includes/locale.php
  • Adds a pre_option_start_of_week filter using a new private function _wp_get_start_of_week
  • Unhook delete_get_calendar_cache from update_option_start_of_week
  • Removes start_of_week from $whitelist_options in wp-admin/options.php

Please let me know what parts could be improved.

#32 @stephenharris
9 years ago

Wouldn't using default_option_ be more suitable than pre_option_? If I have an existing install then this patch will forcibly over-ride my chosen option.

Using default_option_ means existing users' choices are respected, and has an added benefit for developers in that they do not need to remove the callback should they wish to re-instate the setting.

#33 @obenland
9 years ago

I think using default_option_ over pre_option_ makes sense.
We're missing a space in add_filter( 'pre_option_start_of_week','_wp_get_start_of_week' );.

Other than those two things 28344.diff looks good in a visual review.

@swissspidy
9 years ago

#34 @swissspidy
9 years ago

The new patch uses default_option_ instead of pre_option_ and also adds that missing space.

#35 @ocean90
9 years ago

The option should be added to $unusedoptions too I think. What about comment:12, should the default be 0/Sunday?

@swissspidy
9 years ago

#36 @swissspidy
9 years ago

Adding the option to $unusedoptions sounds reasonable. I attached a new patch that does it and also changes the default start of the week to Sunday.

This ticket was mentioned in Slack in #core by ocean90. View the logs.


9 years ago

#38 in reply to: ↑ 37 @ocean90
9 years ago

Replying to slackbot:

This ticket was mentioned in Slack in #core by ocean90. View the logs.

To answer the question "Where does the setting get used?":

start_of_week is currently used in get_weekstartend() which is only used by wp_get_archives(). wp_get_archives() passes the value of start_of_week to get_weekstartend().

The value is also used in _wp_mysql_week() for WP_Date_Query::get_sql_for_clause() and again in wp_get_archives().

And finally in get_calendar() which is used by WP_Widget_Calendar.

This ticket was mentioned in Slack in #core by sergey. View the logs.


9 years ago

#40 @wonderboymusic
9 years ago

  • Owner set to ocean90
  • Status changed from new to assigned

#41 follow-up: @ocean90
9 years ago

A list of plugins which are using the start_of_week option: https://gist.github.com/ocean90/e184d050fe58664b3bb5

After looking at this list I don't feel really comfortable with droping the option. I think we should go with "hide it if it's set to the default". The default would be what $wp_locale->start_of_week; returns.

@ocean90
9 years ago

#42 follow-up: @ocean90
9 years ago

28344.5.patch is what I think we should do.

#43 in reply to: ↑ 42 @swissspidy
9 years ago

Replying to ocean90:

28344.5.patch is what I think we should do.

Do it! :)

#44 @ocean90
9 years ago

In 35336:

WP Locale: Add a start_of_week property to store the start of the week per locale.

Props swissspidy.
See #28344.

#45 in reply to: ↑ 41 @stephenharris
9 years ago

Replying to ocean90:

After looking at this list I don't feel really comfortable with droping the option. I think we should go with "hide it if it's set to the default". The default would be what $wp_locale->start_of_week; returns.

This brings us back to my earlier comment (https://core.trac.wordpress.org/ticket/28344#comment:22) - I think 'hiding if default' doesn't present any advantages to removing the field completely: new installs won't see the option, and existing installs are unlikely to need to change it.

And if there's an easy way for a plug-in to bring the option back(?), then plug-ins that need it can do that.

#46 @ocean90
9 years ago

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

In 35337:

Options: Hide the week starts on setting for installs that have the default setting already.

The default setting is the value of $wp_locale->start_of_week which holds the value per locale, see [35336].

Props swissspidy, ocean90.
Fixes #28344.

#47 in reply to: ↑ 6 @pderksen
9 years ago

Following up on some older comments...

mordauk:

This affects me personally. I have an event calendar plugin that uses this setting to determine what day to show first on the calendar view. Based on the number of support tickets I get from users asking how the day can be changed to Monday (or to Sunday), I can definitely say it's used by a lot of users.

I've had users in all parts of the world want to change this away from the default, so I'm not sure basing it on the charset is sufficient.

I love the idea of simplifying WP core settings, but if I need to add a settings page to my plugin to give users the option to change this, it would make me a little sad. I certainly will if needed though.

Same goes for my calendar plugin. Right now it's actually breaking the implementation of our own "start of week" override option which is working in WP 4.3.

nacin:

What I want to know is why "Sunday" isn't the default in the en_US build. This is only used in core for the Calendar widget, and I cannot recall ever seeing a calendar in the U.S. that starts on a Monday. Yes, ISO sets Monday to be the first day of the week. Anyway.

Did I miss the answer here? Here in the U.S. I've always been confused by this as well, and now with the option going away, I think this will confuse many U.S. users with any calendar plugin (or the built-in WP calendar widget).

#48 @gsexton
9 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

This is not working as expected.

I'm using WordPress 4.4-beta4-35636 and my site language is set to English (United States). The value for WP_Locale()->start_of_week is returning one. The correct value for locale en_US is 0, or Sunday as documented here in the CLDR:

http://unicode.org/repos/cldr/trunk/common/supplemental/supplementalData.xml

#49 in reply to: ↑ 12 @rmccue
9 years ago

I am -1 on hiding this at all. If we're deprecating options with the intent of removing them, then hiding for default makes sense. However, it doesn't seem like there was agreement here on whether we actually should remove it. Additionally, assuming that this is purely something determined by the language also strikes me as incredibly weird.

As an example, the ICLU data above notes the week starting on Monday for the UK, while starting on Sunday for the US and Australia. This is inconsistent with the current defaults (UK, US, and AU all start on Monday in the current data), and makes an unreasonable assumption that there's a single choice (in AU, which you use likely depends on your age).

I get that we're trying to simplify the options here, especially for those that aren't used in core (although as noted, this one is), but this is an option that belongs in core more than it does anywhere else.

Recommend revert.

This ticket was mentioned in Slack in #core by wonderboymusic. View the logs.


9 years ago

#51 @ocean90
9 years ago

In 35685:

Revert [35336] and [35337].

See #28344.

#52 @ocean90
9 years ago

  • Milestone 4.4 deleted
  • Resolution set to wontfix
  • Status changed from reopened to closed

This is not working as expected.

It was working as expected. Changing the default value for US wasn't the goal of this issue. Anyway, closing as wontfix.

This ticket was mentioned in Slack in #core-i18n by swissspidy. View the logs.


8 years ago

#54 @ocean90
7 years ago

In 42718:

I18N: Remove unused $start_of_week property from WP_Locale.

Missed in [35685], see #28344.

Props birgire, tonybogdanov.
Fixes #43344.

Note: See TracTickets for help on using tickets.