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

Add UK bank holidays #75

Closed
wants to merge 9 commits into from
Closed

Conversation

SimonBarrettACT
Copy link
Contributor

@SimonBarrettACT SimonBarrettACT commented Jan 18, 2024

This adds bank holidays for Wales, England, Scotland and Northern Ireland.

If a bank holiday is on a weekend, a ‘substitute’ weekday becomes a bank holiday, normally the following Monday.

https://www.gov.uk/bank-holidays

Country codes

Wales: gb-cym
Scotland: gb-sct
Northern Ireland: gb-nir
England: gb-eng

@nebarg
Copy link

nebarg commented Jan 18, 2024

I don't think the Christmas/Boxing day is correct.

It looks like if Christmas Day is Sunday, Boxing Day stays as Monday and Christmas (substitute) is moved to Tuesday.

An example can be seen for 2022 on https://www.gov.uk/bank-holidays

@SimonBarrettACT
Copy link
Contributor Author

Thanks @nebarg, that is correct.

If Christmas is on a Friday, Boxing Day is a substitute day on the Monday
If Christmas is on a Saturday, Christmas is a substitute day on Monday and Boxing Day is a substitute day on the Tuesday.

@SimonBarrettACT SimonBarrettACT changed the title add holidays in Wales add holidays in England and Wales Jan 18, 2024
@SimonBarrettACT
Copy link
Contributor Author

Have included England, and will add Scotland and NI.

@SimonBarrettACT SimonBarrettACT changed the title add holidays in England and Wales add UK bank holidays Jan 18, 2024
@SimonBarrettACT SimonBarrettACT changed the title add UK bank holidays add GB bank holidays Jan 18, 2024
@SimonBarrettACT SimonBarrettACT changed the title add GB bank holidays add UK bank holidays Jan 18, 2024
@nanos
Copy link

nanos commented Jan 18, 2024

Just to say that I believe GB was the correct country code.

The readme references iso codes (although it doesn't specify which one). Assuming this means ISO 3166-2 the correct country codes are:

GB-ENG for England
GB-NIR for Northern Ireland
GB-SCT for Scotland
GB-WLS for Wales

(BS 6879 also allows GB-CYM for Wales, but ISO 3166-2 doesn't allow double coding, so this isn't formally part of ISO 3166-2 though most implementations allow both.)

Source: https://www.iso.org/obp/ui/#iso:code:3166:GB

Comment on lines +14 to +30
protected function christmasDay(int $year): array
{
$christmasDay = new CarbonImmutable($year . "-12-25", 'Europe/London');
$key = 'Christmas Day';

if ($christmasDay->isSaturday()) {
$key .= ' (substitute day)';
$christmasDay = $christmasDay->next('monday');
}

if ($christmasDay->isSunday()) {
$key .= ' (substitute day)';
$christmasDay = $christmasDay->next('tuesday');
}

return [$key => $christmasDay];
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great thought for this scenario.

Is it possible you move chrismas, boxing and new year to a generic file so other countries can inherit it.

Copy link
Contributor Author

@SimonBarrettACT SimonBarrettACT Jan 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks 😊

I'm not sure about this as different countries might have different rules when either fall on a weekend.

@SimonBarrettACT
Copy link
Contributor Author

I switched it because NI isn't in Great Britain.

Copy link
Member

@Nielsvanpach Nielsvanpach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, thanks for your work on this! Could you also solve the phpstan issues?

Did you verify the generated results in the snapshots?

/** @return array<string, CarbonImmutable> */
protected function variableHolidays(int $year): array
{
$easterSunday = CarbonImmutable::createFromTimestamp(easter_date($year))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you use $this->easter($year) instead? Available after having your fork in sync & rebasing your branch on our main branch

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also for the other usages of easter_date() ofcourse

@SimonBarrettACT SimonBarrettACT changed the title add UK bank holidays Add UK bank holidays Jan 21, 2024
@Nielsvanpach
Copy link
Member

I think something went wrong while rebasing: 115 updated files.

@SimonBarrettACT
Copy link
Contributor Author

I have opened a new PR request #162 to fix the rebase issue.

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

Successfully merging this pull request may close these issues.

5 participants