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

Jekyll with Ruby 2.4 ignores timezone for date field of posts #5963

Closed
stomar opened this issue Mar 17, 2017 · 23 comments
Closed

Jekyll with Ruby 2.4 ignores timezone for date field of posts #5963

stomar opened this issue Mar 17, 2017 · 23 comments

Comments

@stomar
Copy link
Contributor

stomar commented Mar 17, 2017

We have posts with a date field in the YAML front matter that includes a timezone offset. The site is generated with UTC as local timezone.

The expected and established behavior in this setting is that post URL's are generated based on the date converted to UTC, such that e.g. a post with date 2017-01-01 06:00:00 +1200 appears under the URL .../2016/12/31/....

This ensures for example that posts authored in different timezones will still be sorted and displayed in the proper chronological order.

Strangely, this behavior changes when switching from Ruby 2.3 to Ruby 2.4: with 2.4, the timezone offset seems to be ignored, and the post appears under the URL .../2017/01/01/....
This occurs without changing the Jekyll version; here I tested with Jekyll 3.3.1, but observed the same behavior also for earlier Jekyll releases.

For Ruby 2.4:

$ ls -R
.:
_config.yml _posts

./_posts:
2000-01-01-post-1.md
$ cat _posts/2000-01-01-post-1.md
---
date: 2017-01-01 06:00:00 +1200
---
$ cat _config.yml
timezone: UTC
$ ruby -v
ruby 2.4.0p0 (2016-12-24 revision 57164) [i686-linux]
$ jekyll build
Configuration file: none
[snip]
$ ls _site/*/*/*/post-1.html
_site/2017/01/01/post-1.html

For Ruby 2.3 the URL is as expected:

$ ruby -v
ruby 2.3.3p222 (2016-11-21 revision 56859) [i686-linux]
$ jekyll build
Configuration file: none
[snip]
$ ls _site/*/*/*/post-1.html
_site/2016/12/31/post-1.html
@DirtyF
Copy link
Member

DirtyF commented Mar 25, 2017

/cc @jekyll/stability

@stomar
Copy link
Contributor Author

stomar commented Mar 25, 2017

I found out that SafeYAML.load changed the way how dates are handled. I don't know whether this change is intended or a bug.

The behavior until 2.3 was that the parsed date is converted to the local timezone (that's also what Ruby's YAML.load does):

RUBY_VERSION  # => "2.3.3"

ENV["TZ"] = "UTC"  
data = "date: 2017-01-01 06:00:00 +1200"

require "yaml"
YAML.load data      # => {"date"=>2016-12-31 18:00:00 +0000}

require "safe_yaml"
SafeYAML.load data  # => {"date"=>2016-12-31 18:00:00 +0000}

require "safe_yaml/version"
SafeYAML::VERSION   # => "1.0.4"

With 2.4, for the same SafeYAML version, this changes to keeping the timezone as provided in the date attribute:

RUBY_VERSION  # => "2.4.0"

ENV["TZ"] = "UTC"  
data = "date: 2017-01-01 06:00:00 +1200"

require "yaml"
YAML.load data      # => {"date"=>2016-12-31 18:00:00 +0000}

require "safe_yaml"
SafeYAML.load data  # => {"date"=>2017-01-01 06:00:00 +1200}

require "safe_yaml/version"
SafeYAML::VERSION   # => "1.0.4"

(Note that YAML.load has the same behavior for 2.3 and 2.4.)

Jekyll then simply takes the date part of the post's date attribute, without further considering the timezone when generating the URLs (if I remember correctly, I didn't check this part).

@Manishearth
Copy link

I basically monkeypatched SafeYAML to fix this

https://github.com/Manishearth/manishearth.github.io/blob/source/plugins/manish_custom_slug.rb

@jekyllbot
Copy link
Contributor

This issue has been automatically marked as stale because it has not been commented on for at least two months.

The resources of the Jekyll team are limited, and so we are asking for your help.

If this is a bug and you can still reproduce this error on the 3.3-stable or master branch, please reply with all of the information you have about it in order to keep the issue open.

If this is a feature request, please consider building it first as a plugin. Jekyll 3 introduced hooks which provide convenient access points throughout the Jekyll build pipeline whereby most needs can be fulfilled. If this is something that cannot be built as a plugin, then please provide more information about why in order to keep this issue open.

This issue will automatically be closed in two months if no further activity occurs. Thank you for all your contributions.

@jekyllbot jekyllbot added the stale Nobody stepped up to work on this issue. label Jul 14, 2017
@Manishearth
Copy link

This is still a bug, and can be easily reproduced by writing a post with a given timezone specified, and then generating the blog with a different system time.

@jekyllbot jekyllbot removed the stale Nobody stepped up to work on this issue. label Jul 14, 2017
@yous
Copy link
Contributor

yous commented Jul 25, 2017

I also can reproduce this bug, which makes me stick with Ruby 2.3. Here is some test code regarding timezone: https://github.com/yous/yous.github.io/blob/source/spec/site_spec.rb.

@wh0
Copy link

wh0 commented Aug 1, 2017

https://pages.github.com/versions/

Github Pages has recently switched to Ruby 2.4.0. Most of the permalinks on my website are currently broken, and I think it's because of this.

(earlier: 2.3.3)

@Crunch09
Copy link
Member

Crunch09 commented Aug 3, 2017

I guess this issue is the same as described in #6033 ? (and possible fix in #6224 )

@jekyllbot
Copy link
Contributor

This issue has been automatically marked as stale because it has not been commented on for at least two months.

The resources of the Jekyll team are limited, and so we are asking for your help.

If this is a bug and you can still reproduce this error on the 3.3-stable or master branch, please reply with all of the information you have about it in order to keep the issue open.

If this is a feature request, please consider building it first as a plugin. Jekyll 3 introduced hooks which provide convenient access points throughout the Jekyll build pipeline whereby most needs can be fulfilled. If this is something that cannot be built as a plugin, then please provide more information about why in order to keep this issue open.

This issue will automatically be closed in two months if no further activity occurs. Thank you for all your contributions.

@jekyllbot jekyllbot added the stale Nobody stepped up to work on this issue. label Oct 3, 2017
@Manishearth
Copy link

This still exists, and I still have to work around it with the monkeypatch

@jekyllbot jekyllbot removed the stale Nobody stepped up to work on this issue. label Oct 3, 2017
@pathawks
Copy link
Member

pathawks commented Oct 3, 2017

@Manishearth Do you have time to turn that monkeypatch into a pull request?

@Manishearth
Copy link

I'm unsure if it's such a good idea to have such a monkeypatch in the jekyll tree? It's possible it will break other things.

But sure, I can submit it if you can point me to where such monkeypatches go.

@pathawks
Copy link
Member

pathawks commented Oct 3, 2017

@Manishearth I'm sorry, I misunderstood. You are patching SafeYAML rather than Jekyll.

There is a PR for SafeYAML to preserve timezones of dates: dtao/safe_yaml#87
But since the last commit was in 2014, I'm not sure what the odds are of that PR ever being merged.

This is a pretty bad situation. I wonder if we need our own fork of SafeYAML?

@Crunch09
Copy link
Member

Crunch09 commented Oct 3, 2017

@pathawks Maybe this is a stupid question but do we really need SafeYAML ? Couldn't we just use something like psych instead which is maintained?

@pathawks
Copy link
Member

pathawks commented Oct 3, 2017

@Crunch09 That's not a bad thought. The nice thing about SafeYAML is that it is pure Ruby and doesn't require any C dependencies, which has been a source of frustration in the past.

The nice thing about Psych is that it is actually maintained.

How much work would it be to create a PR that removed SameYAML in favor of Psych?

@Crunch09
Copy link
Member

Crunch09 commented Oct 3, 2017

How much work would it be to create a PR that removed SameYAML in favor of Psych?

Don't think it would be too much work but i can try to come up with a PR sometime this week if you want and we can go from there?

@parkr
Copy link
Member

parkr commented Oct 5, 2017

How much work would it be to create a PR that removed SafeYAML in favor of Psych?

Does Psych offer "safe loading"?

@Crunch09
Copy link
Member

Crunch09 commented Oct 8, 2017

@parkr Yes, it has a method safe_load which only deserializes whitelisted classes.

@jekyllbot jekyllbot added the stale Nobody stepped up to work on this issue. label Dec 8, 2017
@Manishearth
Copy link

Still a bug. For the same reasons.

@jekyllbot jekyllbot removed the stale Nobody stepped up to work on this issue. label Dec 8, 2017
@jekyll jekyll deleted a comment from jekyllbot Dec 8, 2017
@DirtyF DirtyF added this to the 4.0 milestone Dec 8, 2017
@stomar
Copy link
Contributor Author

stomar commented Dec 20, 2017

Note that with Ruby 2.5, Psych will preserve the timezone offset of dates parsed from YAML, similar to what SafeYAML (accidently?) already seems to do in 2.4.

This probably means that Jekyll has to handle the conversion to localtime or specified timezone itself, instead of relying on the YAML parser doing that.

@DirtyF DirtyF added the pinned label Dec 21, 2017
@parkr
Copy link
Member

parkr commented Jan 2, 2018

Would anyone here be willing to come up with some tests to help us get this one right? It sounds like Ruby and our YAML parsers are changing out from under us so we would ideally behave the same in all cases.

Anecdotally, the dates change for me quite a bit when I travel to Europe/Asia/east of the Prime Meridian. That’s not super desirable.

Crunch09 added a commit that referenced this issue Jan 11, 2018
it fails with ruby 2.4 onwards and passes up to ruby 2.3
@Crunch09
Copy link
Member

@parkr @stomar Just added some tests in eaac869 , one of the test fails in Ruby 2.4 (the one with the timezone). I converted the times to Year/Month/Day because that's what's used in the URLs so it felt reasonable to test for this.

@DirtyF
Copy link
Member

DirtyF commented Nov 2, 2018

Fixed in #6697

@DirtyF DirtyF closed this as completed Nov 2, 2018
@DirtyF DirtyF removed this from Ideas/Unconfirmed in Jekyll 4.1 Aug 3, 2019
@DirtyF DirtyF removed this from the 4.0 milestone Aug 3, 2019
@jekyll jekyll locked and limited conversation to collaborators Aug 2, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

9 participants