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

Fix timezone incosistencies between different ruby version #6697

Merged
merged 6 commits into from Jan 25, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 5 additions & 5 deletions features/permalinks.feature
Expand Up @@ -71,14 +71,14 @@ Feature: Fancy permalinks
And I should see "Totally custom." in "_site/03-27-2009/custom-permalink-schema.html"

Scenario: Use custom permalink schema with date and time
Given I have a _posts directory
Given I have a configuration file with:
| key | value |
| permalink | "/:year:month:day:hour:minute:second.html" |
| timezone | UTC |
And I have a _posts directory
And I have the following post:
| title | category | date | content |
| Custom Permalink Schema | stuff | 2009-03-27 22:31:07 | Totally custom. |
And I have a configuration file with:
| key | value |
| permalink | "/:year:month:day:hour:minute:second.html" |
| timezone | UTC |
When I run jekyll build
Then I should get a zero exit status
And the _site directory should exist
Expand Down
4 changes: 4 additions & 0 deletions features/step_definitions.rb
Expand Up @@ -4,6 +4,7 @@
FileUtils.rm_rf(Paths.test_dir) if Paths.test_dir.exist?
FileUtils.mkdir_p(Paths.test_dir) unless Paths.test_dir.directory?
Dir.chdir(Paths.test_dir)
@timezone_before_scenario = ENV["TZ"]
end

#
Expand All @@ -13,6 +14,7 @@
Paths.output_file.delete if Paths.output_file.exist?
Paths.status_file.delete if Paths.status_file.exist?
Dir.chdir(Paths.test_dir.parent)
ENV["TZ"] = @timezone_before_scenario
end

#
Expand Down Expand Up @@ -85,6 +87,7 @@

if status == "post"
parsed_date = Time.xmlschema(input_hash["date"]) rescue Time.parse(input_hash["date"])
input_hash["date"] = parsed_date
filename = "#{parsed_date.strftime("%Y-%m-%d")}-#{title}.#{ext}"
end

Expand Down Expand Up @@ -116,6 +119,7 @@
{}
end
config[key] = YAML.load(value)
Jekyll.set_timezone(value) if key == "timezone"
File.write("_config.yml", YAML.dump(config))
end

Expand Down
2 changes: 1 addition & 1 deletion lib/jekyll/document.rb
Expand Up @@ -426,7 +426,7 @@ def merge_categories!(other)

private
def merge_date!(source)
if data.key?("date") && !data["date"].is_a?(Time)
if data.key?("date")
Copy link
Member

Choose a reason for hiding this comment

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

I see the addition of tests 👍

What’s this line about?

Copy link
Member Author

@Crunch09 Crunch09 Jan 18, 2018

Choose a reason for hiding this comment

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

Sorry, didn't had time yesterday to properly update this PR with more information.

This change actually fixes the inconsistent behaviour, because as long as data['date'] is set it will call Utils.parse_date which transforms the given data into localtime (as determined by ENV['TZ']). This is exactly the behaviour that SafeYAML was doing automatically before Ruby 2.4.
Because of the old version of this line, there were no inconsistencies between the different Ruby versions when data['date'] was interpreted as a String rather than a Time.

That said, I am not sure the behaviour of Ruby 2.3 is my preferred one, i would actually prefer the behaviour of newer ruby versions but the most important thing is to fix the incosistency and i would argue that jekyll users are more used to the behaviour of everything gets converted into whatever ENV['TZ'] is.

data["date"] = Utils.parse_date(
data["date"].to_s,
"Document '#{relative_path}' does not have a valid date in the #{source}."
Expand Down
4 changes: 4 additions & 0 deletions test/source/_dates/date_without_time.md
@@ -0,0 +1,4 @@
---
date: 2015-10-01
---
Here is the content.
4 changes: 4 additions & 0 deletions test/source/_dates/time_with_timezone.md
@@ -0,0 +1,4 @@
---
date: 2015-10-01 01:00:00 +0800
---
Here is the content.
4 changes: 4 additions & 0 deletions test/source/_dates/time_without_timezone.md
@@ -0,0 +1,4 @@
---
date: 2015-10-01 01:00:00
---
Here is the content.
43 changes: 43 additions & 0 deletions test/test_document.rb
Expand Up @@ -16,6 +16,19 @@ def setup_encoded_document(filename)
}).tap(&:read)
end

def setup_document_with_dates(filename)
site = fixture_site("collections" => ["dates"])
site.process
docs = nil
with_env("TZ", "UTC") do
docs = Document.new(site.in_source_dir(File.join("_dates", filename)), {
:site => site,
:collection => site.collections["dates"],
}).tap(&:read)
end
docs
end

context "a document in a collection" do
setup do
@site = fixture_site({
Expand Down Expand Up @@ -558,4 +571,34 @@ def setup_encoded_document(filename)
Jekyll::Renderer.new(@document.site, @document).render_document
end
end

context "a document with a date with timezone" do
setup do
@document = setup_document_with_dates "time_with_timezone.md"
end

should "have the expected date" do
assert_equal "2015/09/30", @document.data["date"].strftime("%Y/%m/%d")
end
end

context "a document with a date with time but without timezone" do
setup do
@document = setup_document_with_dates "time_without_timezone.md"
end

should "have the expected date" do
assert_equal "2015/10/01", @document.data["date"].strftime("%Y/%m/%d")
end
end

context "a document with a date without time" do
setup do
@document = setup_document_with_dates "date_without_time.md"
end

should "have the expected date" do
assert_equal "2015/10/01", @document.data["date"].strftime("%Y/%m/%d")
end
end
end