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

Replace "Europe/Kiev" timezone with "Europe/Kyiv" #51703

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

K-S-A
Copy link

@K-S-A K-S-A commented May 1, 2024

Motivation / Background

This Pull Request has been created because of the request to use common timezone name "Europe/Kyiv" instead of the "Europe/Kiev". There are a few approaches to introduce this change in the rails repo:

  1. Update Kyiv timezone mapping value #40959
  2. Fix TimeZone::MAPPING for 'Kyiv' #24375
  3. Rename Europe/Kiev to Europe/Kyiv in ActiveSupport::TimeZone #44273

All mentioned related sources are changed in the last couple of years:

  1. https://github.com/tzinfo/tzinfo-data (https://github.com/tzinfo/tzinfo-data/blame/master/lib/tzinfo/data/definitions/Europe/Kiev.rb is an alias for https://github.com/tzinfo/tzinfo-data/blame/master/lib/tzinfo/data/definitions/Europe/Kyiv.rb since this commit tzinfo/tzinfo-data@5a58dcb).
  2. https://www.iana.org/time-zones (eggert/tz@e13e9c5).

Detail

This Pull Request changes internal tzinfo data from "Europe/Kiev" to "Europe/Kyiv" for ActiveSupport::TimeZone.

Additional information

Current workaround:

{ 'Kyiv' => 'Europe/Kyiv' }.each do |zone_name, tzinfo_name|
  time_zone = ActiveSupport::TimeZone.send(:zones_map)[zone_name]
  tzinfo = ActiveSupport::TimeZone.find_tzinfo(tzinfo_name)

  warn("Remove timezone patch for '#{tzinfo_name}' at #{__FILE__}:#{__LINE__}") if time_zone.tzinfo.name == tzinfo_name

  time_zone.instance_variable_set(:@tzinfo, tzinfo)
end

This update may break some existing applications that rely on time zone names. Though, the data migration should be trivial.

Checklist

Before submitting the PR make sure the following are checked:

  • This Pull Request is related to one change. Unrelated changes should be opened in separate PRs.
  • Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]
  • Tests are added or updated if you fix a bug or add a feature.
  • CHANGELOG files are updated for the changed libraries if there is a behavior change or additional feature. Minor bug fixes and documentation changes should not be included.

@mechnicov
Copy link
Contributor

Hi Serhii

I think it is not good idea because of backward compatibility in applications that use RDBMS timezones and map it using Active Support. For example, PostgreSQL

SELECT name FROM pg_timezone_names WHERE name ~ 'Kiev';
       name        
-------------------
 Europe/Kiev
 posix/Europe/Kiev
(2 rows)
SELECT name FROM pg_timezone_names WHERE name ~ 'Kyiv';
 name 
------
(0 rows)

I believe it's better firstly to add aliases to PostgreSQL (and other systems), and after that make this change

@K-S-A
Copy link
Author

K-S-A commented May 1, 2024

@mechnicov, thank you for raising these issues.

I work closely with PostgreSQL and MySQL. Can't provide much information about other RDBMSs.

PostgreSQL respects IANA (Olson) time zone database (https://www.postgresql.org/docs/16/datatype-datetime.html#DATATYPE-TIMEZONES) and there should be no issues in a long run.

MySQL does not follow IANA according to documentation (https://dev.mysql.com/doc/refman/8.0/en/time-zone-support.html#time-zone-installation). Though, it provides good tooling to import system timezone data.

With that information, I can state that the output of SELECT name FROM pg_timezone_names WHERE name ~ 'K(ie|yi)v'; query will include updated time zone version for maintained systems and the number of such cases will increase over time.

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

Successfully merging this pull request may close these issues.

None yet

2 participants