Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Week methods only work with local translator, not default locale #1967

Closed
hackel opened this issue Dec 19, 2019 · 4 comments · Fixed by #2090
Closed

Week methods only work with local translator, not default locale #1967

hackel opened this issue Dec 19, 2019 · 4 comments · Fixed by #2090
Assignees
Milestone

Comments

@hackel
Copy link
Contributor

hackel commented Dec 19, 2019

Hello,

I encountered an issue with the following code:

echo Carbon::getLocale(); // en
echo Carbon::now()->locale(); // en
echo Carbon::now()->startOfWeek()->dayName; // Monday
Carbon::setLocale('en_US');
echo Carbon::now()->startOfWeek()->dayName; // Monday

echo Carbon::now()->locale('en')->startOfWeek()->dayName; // Sunday

Trying to figure out all of the different (apparently competing?) localization settings makes my head spin. The issue seems to be that "firstWeekDay" will only ever be determined by a local translator, not the default translator:

Carbon\Traits\Localization\Date:928

case $name === 'firstWeekDay':
    return $this->localTranslator ? ($this->getTranslationMessage('first_day_of_week') ?? 0) : static::getWeekStartsAt();

I'm not sure what the point of static::$weekStartsAt is...backward compatibility? It conflicts with what the default translator is supposed to return.

As far as I can tell, the only way to solve this is to use the deprecated methods setWeekStartsAt() AND setWeekEndsAt() (which for some bizarre reason is not calculated automatically), or explicitly include the locale() call every single time I use these methods (even though it already is the default). Am I missing something obvious here?

I would propose removing the default values for static::$weekStartsAt/EndsAt, and changing the getters to only use them if they are explicitly set:

case $name === 'firstWeekDay':
    return static::getWeekStartsAt() ?? $this->getTranslationMessage('first_day_of_week', null, 0);

This is a simple change and I would be happy to submit a PR if acceptable, but it would technically be breaking, even though it's a bugfix in my opinion.

Carbon version: 2.28.0

PHP version: 7.4.0

I expected to get: (with default 'en' or 'en_US' locale)

echo Carbon::now()->startOfWeek()->dayName; // Sunday

But I actually get:

echo Carbon::now()->startOfWeek()->dayName; // Monday

Thanks!

@kylekatarnls
Copy link
Collaborator

kylekatarnls commented Dec 19, 2019

Hello,

This is a legacy. startOfWeek exists for a long time before first_day_of_week was supported in different locales.

At this time Monday as the standard start of week was the default value for startOfWeek and when the I18N has been added, default locale en has been chosen as diffForHumans returned English text.

So changing this behavior would not be a bug fix but really a breaking changes.

Many users use startOfWeek without actually using I18N (so they are implicitly on the default locale en), and Monday is the start of week for the very majority of people in the world.

This change could break apps that legitimately rely on this. An other solution must be found or it should wait a version 3.

@kylekatarnls kylekatarnls added the postponed Not yet possible but candidate for a next major release label Dec 20, 2019
@kylekatarnls kylekatarnls added this to the 3.0.0 milestone Apr 21, 2020
@kylekatarnls kylekatarnls removed the postponed Not yet possible but candidate for a next major release label Apr 21, 2020
@kylekatarnls kylekatarnls self-assigned this May 17, 2020
@kylekatarnls
Copy link
Collaborator

kylekatarnls commented May 17, 2020

Considered solution:

In Carbon 3, the default locale could be en_001 “International English”.

And default start of week and every calendar/format features would be aligned on this locale as a standard. Then our "en" locale could be identical to the "en_001" and so we would keep "en" as default locale (as in Laravel config) but it would now mean "en_001" and no longer "en_US" as in Carbon 2, and so default start of week would match the default locale. There is basically no wording difference between en_001 and en_US, so it should be a smooth change.

⚠️ The date formats still have to be checked. Some sources gives dd/MM, some other MM/dd so I have to clarify which should be considered as the standard.

@kylekatarnls
Copy link
Collaborator

While working on this for 3.0 I actually realized I could use an opt-in mode to enable the behavior of 3.0 in the next 2.35 version.

With:

Carbon::setWeekStartsAt('auto');
Carbon::setWeekEndsAt('auto');

startOfWeek() will rely on the day in the current culture (Sunday as start of en or en_US)

Open PR for this fix: #2090

kylekatarnls added a commit to kylekatarnls/Carbon that referenced this issue May 19, 2020
kylekatarnls added a commit that referenced this issue May 20, 2020
…nd-auto-mode

Fix #1967 Add auto mode for start/end of week
@kylekatarnls
Copy link
Collaborator

This can be tested requiring "nesbot/carbon": "2.x-dev",

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants