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

Respecting the post's "local" timezone (revisiting #4096) #6033

Open
t-brink opened this issue Apr 17, 2017 · 38 comments
Open

Respecting the post's "local" timezone (revisiting #4096) #6033

t-brink opened this issue Apr 17, 2017 · 38 comments
Labels
has-pull-request Somebody suggested a solution to fix this issue needs-decision 🤔

Comments

@t-brink
Copy link

t-brink commented Apr 17, 2017

I would like to revisit issue #4096. Summary: I record the timezone offset for every post in the front matter: date: 2017-04-17 23:00:00 +0200. Jekyll always converts that to the local timezone, a fact that will not change to avoid regressions, I understand that.

Sadly, this is not entirely satisfactory. One may want to change the timezone for the blog temporarily (holidays, staying abroad for a few months) or permanently (moving from one timezone to another inside the US should be rather common). This can have very bad side effects: When using URL schemes including the date, the URLs might change! The data in any RSS feed would also change. I would also prefer that the timestamp of any blog post retains the local time from when it was written. This last item may not be everyone's taste, though, and they may be served by always using UTC.

So the question is what can be done about this? Currently, you still have to either monkey patch Document (see link in #4096) or keep another, redundant field in the front matter. From my point of view, the optimal solution would be some sort of global option to keep the blog post date in the same timezone. Alternatively, at least storing the original string somewhere or providing a hook to get that data before conversion would be acceptable. Although, then it is still unclear to me how to generate the correct URLs.

I would be willing to try to submit a patch, but I would like some input on what should be done before I get my hands dirty, it is after all a user-facing issue. Any thoughts about the best way to do this?

@pathawks
Copy link
Member

I would love to have an option to preserve the timezone anywhere one is provided. This is certainly what I expect to happen as a user.

@t-brink
Copy link
Author

t-brink commented Apr 18, 2017

OK, looking into this further, the problem starts with SafeYAML, see https://github.com/dtao/safe_yaml/blob/master/lib/safe_yaml/parse/date.rb. It also converts dates to local time, either explicitly or by calling DateTime.to_time, which converts to local time, too. That means the behavior in jekyll would differ between dates given with or without quotes. The patch for SafeYAML would be trivial, the question is more one of API.

As there seems to be at least some interest, I will try engage this project about the issue first.

@pathawks
Copy link
Member

That means the behavior in jekyll would differ between dates given with or without quotes.

That's a non-starter. I wish that everybody would just use YAML native types, but that is not the case.

@t-brink
Copy link
Author

t-brink commented Apr 21, 2017

I agree that "date:" should behave the same in all cases. Looking at the commit history and the lack of comments on issues, it seems that SafeYAML development is currently inactive. What is the jekyll project's policy in this case? Should we monkey patch SafeYAML for the time being? Fork it?

@t-brink
Copy link
Author

t-brink commented Apr 22, 2017

Here is a proof of concept, lightly tested by me. I hope I found every place that needs fixing, the patch is pleasingly simple: t-brink@9a5f0d1. It adds a configuration option preserve_timezones (default false). Sadly, it needs a monkey patch for SafeYAML. I opted to simply disable date parsing at that state and doing that exclusively in Jekyll::Utils::parse_date. I also changed that method to use DateTime instead of Time. This fixes a bug: Time.parse assumes local time if the timezone is missing, DateTime.parse uses UTC. The latter is what SafeYAML has been doing and is what is required by the YAML spec. Now, parsing of time without timezones is consistent between dates given with and without strings.

Please let me know if that would be acceptable for the jekyll project and I will try to add documentation and tests and submit a pull request.

@t-brink
Copy link
Author

t-brink commented May 17, 2017

Is there still interest in this? If not, I have a local workaround (really a hack, but I think robust enough) that I could post here for posterity and close this issue.

@pathawks
Copy link
Member

I don't know if we should monkey patch SameYAML, but maybe they would be open to a PR?

I would definitely like to have this feature! 👍

@t-brink
Copy link
Author

t-brink commented May 17, 2017

Let's just try it. I will sent one in the next few days and we'll see if there is some activity.

Sadly there is no way to implement this feature without support from the YAML parser: Parsing dates cannot be disabled, so no chance to fix this purely inside Jekyll without monkey patching.

@pathawks
Copy link
Member

Please make a note here if/when you open a thread over at SafeYAML

@t-brink
Copy link
Author

t-brink commented May 20, 2017

Pull request is here: dtao/safe_yaml#87

@t-brink
Copy link
Author

t-brink commented Jun 15, 2017

Hmm, there is no activity in SafeYAML. Last commit is from 2014 and there are no answers to pull requests or bug reports. Any way to proceed?

@DirtyF
Copy link
Member

DirtyF commented Jun 15, 2017

@parkr What do you say? Do we want to preserve or allow to change a timezone without modifying URLs?

@t-brink
Copy link
Author

t-brink commented Jul 2, 2017

Just a heads up: I will soon start a new job and in a few weeks I will have no more time to contribute to Jekyll. Before then, I can still submit a pull request which monkey-patches SafeYAML. If that is unacceptable I am happy to work around the issue in my local installation and close this bug or leave it to whoever wants to fix this.

@pathawks
Copy link
Member

pathawks commented Jul 2, 2017

If you submit a patch, it might be easier for us to talk about if it's something we want to support.

@t-brink
Copy link
Author

t-brink commented Jul 2, 2017

OK, I'll try to submit something concrete next weekend.

@ticky
Copy link

ticky commented Jul 12, 2017

I’ve been working on a similar change, needing a fix for a similar problem. I have a blog to which I want to add posts from several time zones, and unfortunately Jekyll clobbers the zone information, and worse, has inconsistent behaviour depending on where the blog is built. I can work around the latter by overriding the “blog” time zone, but this isn’t really a satisfactory option.

My belief is that the “correct” behaviour (and that I’d like to introduce) is that times are passed through to the view without any conversion, including respecting completely the date as specified in a post’s time zone, even if that means posts which share a day directory on-disk might be considered the same “day” in different time zones.

I’ve got a work in progress patch up at master...ticky:time-zone-awareness, however, it more or less clobbers the existing behaviour right now.

I think my favoured solution for upstreaming would be to introduce this as a configuration option, allowing use of the existing behaviour or the improved time zone awareness.

Additionally, I don’t know if I’m doing something unusual, but it seems so far that the issues noted here around safe_yaml changing time zone don’t apply, at least in the feature specs?

@ticky
Copy link

ticky commented Jul 12, 2017

Relatedly, the behaviour change I propose would make issue #6177 obsolete in cases where the new behaviour is opted into.

@t-brink
Copy link
Author

t-brink commented Jul 12, 2017

Hi! I had a look at your patch and it is incomplete. When the date is not in quotes (i.e. a YAML string), SafeYAML will convert the entry to a date in localtime, see https://github.com/dtao/safe_yaml/blob/master/lib/safe_yaml/parse/date.rb (try "DateTime.parse("2017-07-12T10:00:00+0600").to_time" in irb to convince yourself that it does this, change +0600 to a timezone different from your system).

Further up in this bug we concluded that different behavior between quoted and unquoted dates is unacceptable. Your patch is also part of what I have in mind, but there are other places that need fixes. Sorry I didn't yet submit something, I will try to ASAP.

BTW: Should your system behave differently, that would be very weird and I'd like to know more about your ruby versions etc.

@ticky
Copy link

ticky commented Jul 12, 2017

Like I said, @t-brink, it’s a work in progress. 😄

I agree that string and non-string dates should behave identically. Do you have any test cases which can be added to the source tree to verify this? As far as I can see it looks like the .feature specs generate unquoted dates, which I took to mean that they were processed by safe_yaml directly.

I’m testing on ruby 2.4.1p111 (2017-03-22 revision 58053) [x86_64-darwin16], if that context helps.

@t-brink
Copy link
Author

t-brink commented Jul 12, 2017

For what it's worth, here is a patch I worked on some time ago:

t-brink@9a5f0d1

It has no tests, yet. It also disables SafeYAML's date parsing via monkey patch, which is probably not a good idea. It does add a config option, though. Maybe it'll help avoiding duplicate work, at least 😄

A hacked-together and untested spec should probably look like this:

    Scenario: Preserve post's timezone with explicitly set timezone (different than posts' time)
      Given I have a _layouts directory
      And I have a page layout that contains "Page Layout: {{ site.posts.size }}"
      And I have a post layout that contains "Post Layout: {{ content }} built at {{ page.date | date_to_xmlschema }}"
      And I have an "index.html" page with layout "page" that contains "site index page"
      And I have a configuration file with:
        | key         | value                |
        | timezone    | UTC                  |
      And I have a _posts directory
      And I have the following posts:
        | title     | date                   | layout  | content             |
        | entry1    | 2017-07-12 01:00 +0200 | post    | content for entry1. |
      When I run jekyll build
      Then I should get a zero exit status
    And the _site directory should exist
      And the "_site/2017/07/12/entry1.html" file should exist

The point is that the site is set to UTC, and the post to "2017-07-12 01:00 +0200". The post's date in UTC should be 2017-07-11, but we want it to stay 2017-07-12 (I hope I'm not pointing out the obvious here). I'm sleepy, so I'm sorry if this test is wrong, please apply common sense ;-)

@t-brink
Copy link
Author

t-brink commented Jul 15, 2017

Sent a pull request, see #6224. Basic idea in line with @ticky's patch, but adds a flag to enable/disable the feature and patches SafeYAML. Please let me know what you think.

@parkr parkr added the has-pull-request Somebody suggested a solution to fix this issue label Aug 4, 2017
@ticky
Copy link

ticky commented Aug 28, 2017

I’m pleased to see #6334 got merged, but this remains a problem. What’s the pathway to resolving this?

@parkr
Copy link
Member

parkr commented Sep 1, 2017

Reverting this completely will be a no-go since it'll have the opposite effect of breaking everyone's pre-existing URLs. There's a PR open to add an option but we have a general frustration with adding Yet Another Configuration Option lest we end up being useless because we have simply too many configuration options to keep track of. As far as I'm concerned, this is a bug in how Ruby 2.4 handles Time parsing. I'm kind of at an impasse. :(

@ticky
Copy link

ticky commented Sep 2, 2017

And from my perspective, I can’t get reliable permalinks unless I set an override for the time zone of my blog (which really shouldn’t matter), which looks very much like a Jekyll bug.

Like either the behaviour that was documented by #6334 needs to be changed because it is wrong, or this time zone parsing bug needs working around.

Would making the change to respect posts’ time zone but adding a warning on build if the date then doesn’t match the date in the file name seem reasonable? Is there a way to add this fix as a “major release” change?

@t-brink
Copy link
Author

t-brink commented Oct 14, 2017

For what it's worth, I now use a hack to get the desired result:

  • Set the timezone to UTC in _config.yml.
  • For the date fields in the front matter, I use the timestamp in a local timezone, but leave the UTC offset out. This means they are interpreted as UTC. Thus, the URLs will be correct by tricking Jekyll.
  • Add an additional field tz that contains the actual timezone offset.
  • Use a plugin (see below) to add a realdate variable, which contains the correct timestamp and timezone. Use this realdate everywhere you want to output a date or time.
  • if you use the date_to_xmlschema, date_to_rfc822, date_to_string, or date_to_long_string liquid filters, be aware that they convert to "local" time, i.e., UTC in the present case. The date filter is provided by the Liquid project itself and works correctly by not messing with the timezone. I (monkey-)patched this locally to be safe, but you can just always use date at the moment.

The plugin:

Jekyll::Hooks.register [:pages, :posts], :pre_render do |post|
  # Get date and timezone.
  next unless post.data.key? "date"
  date = post.data["date"]
  if post.data.key? "tz"
    # Normalize timezone.
    begin
      m_tz = /^([-+])(\d\d):?(\d\d)$/.match(post.data["tz"])
    rescue TypeError
      raise "The \"tz\" field in #{post.path} must be a string!"
    end
    raise "Wrong timezone format in field \"tz\" in file #{post.path}" \
      unless m_tz
    tz = m_tz[1] + m_tz[2] + ":" + m_tz[3]
  else
    # The tz field is missing.
    puts "WARNING: File #{post.path} has a date without " + \
         "a \"tz\" in the front matter, using UTC."
    tz = "+00:00"
  end
  # Store the actual date.
  post.data["realdate"] = Time.new(date.year, date.month, date.day,
                                   date.hour, date.min, date.sec,
                                   tz)
end

Note: this will output a warning if there is a date field without a tz field. Drafts automatically get dates, so this might give some spurious warnings in some cases. You might be able to filter this based on post.path if you like.

I would still like to see this get fixed in Jekyll and I still believe the current behavior is buggy. Nevertheless, I can live with this and have no time to push this further.

@parkr
Copy link
Member

parkr commented Oct 18, 2017

Would making the change to respect posts’ time zone but adding a warning on build if the date then doesn’t match the date in the file name seem reasonable?

@ticky That would certainly work initially. I'm afraid I'm not really sure how to go about fixing this myself, and I am kind of stepping away from maintainership of this project. Even gating the offending localtime call on RUBY_VERSION < 2.4 or something could potentially help here. I spent a few days looking into this ages ago and the solution I have in the code is what I came up with. I'm sorry it's not satisfactory. 😦

@ashmaroli
Copy link
Member

@Crunch09 Can this issue be closed now..?

@pathawks
Copy link
Member

Can this issue be closed now..?

Nope.

@ticky
Copy link

ticky commented Jan 26, 2018

Can confirm. #6697 is a welcome improvement to consistency of time zone handling, but does not resolve the crux of this issue, which is post time zones being completely lost.

@ticky
Copy link

ticky commented Jun 20, 2018

Is a fix for this (like #6224?) likely to be integrated in some way any time soon?

@parkr
Copy link
Member

parkr commented Jun 20, 2018

I’d be down to approve something like that myself, but @oe really has final say! 😄

@obskyr
Copy link

obskyr commented May 9, 2020

I hereby bumpeth this – it's two years after this issue was last discussed, and it's still around! How 'bout that decision be made?

@jekyll jekyll deleted a comment May 9, 2020
@obskyr
Copy link

obskyr commented May 12, 2020

Here's a plugin to monkeypatch the date-to-string functions. Use this in conjunction with @t-brink's plugin, and you can get per-post time zones as long as you make sure to use post.real-date (or whatever you named it) instead of post.date. It doesn't support 3.8.0's ordinal arguments, but hey.

module Jekyll
    module RealDateFilters
        def date_to_xmlschema(time)
            time.strftime "%Y-%m-%dT%H:%M:%S%:z"
        end

        def date_to_rfc822(time)
            time.strftime "%a, %d %b %Y %H:%M:%S %z"
        end

        def date_to_string(time)
            time.strftime "%d %b %Y"
        end

        def date_to_long_string(time)
            time.strftime "%d %B %Y"
        end
    end
end

Liquid::Template.register_filter(Jekyll::RealDateFilters)

Here's hoping Jekyll gets in a fix for this sometime! That your posts' times fluctuate if you move to another country or go on vacation strikes me as a bit of an oversight.

@Gothicania
Copy link

I would like to revisit issue #4096. Summary: I record the timezone offset for every post in the front matter: date: 2017-04-17 23:00:00 +0200. Jekyll always converts that to the local timezone, a fact that will not change to avoid regressions, I understand that.

Sadly, this is not entirely satisfactory. One may want to change the timezone for the blog temporarily (holidays, staying abroad for a few months) or permanently (moving from one timezone to another inside the US should be rather common). This can have very bad side effects: When using URL schemes including the date, the URLs might change! The data in any RSS feed would also change. I would also prefer that the timestamp of any blog post retains the local time from when it was written. This last item may not be everyone's taste, though, and they may be served by always using UTC.

So the question is what can be done about this? Currently, you still have to either monkey patch Document (see link in #4096) or keep another, redundant field in the front matter. From my point of view, the optimal solution would be some sort of global option to keep the blog post date in the same timezone. Alternatively, at least storing the original string somewhere or providing a hook to get that data before conversion would be acceptable. Although, then it is still unclear to me how to generate the correct URLs.

I would be willing to try to submit a patch, but I would like some input on what should be done before I get my hands dirty, it is after all a user-facing issue. Any thoughts about the best way to do this?

@fauno
Copy link
Member

fauno commented Jan 2, 2021

seems related to #8425

@UlyssesZh
Copy link

It has been some time since last activities, but I need to say I am encountering similar problems. I previously live in China Standard Time, now I live in Pacific Standard Time.

@UlyssesZh
Copy link

Is it a good idea to just add a plugin like this:

module Jekyll::Utils
	def parse_date input, msg = "Input could not be parsed."
		Time.parse input
	rescue ArgumentError
		raise ::Jekyll::Errors::InvalidDateError, "Invalid date '#{input}': #{msg}"
	end
end

(Just removed the Time#localtime call by the result of Time::parse.)

@y377
Copy link
Member

y377 commented Feb 10, 2023

@ashmaroli

hi!

{{ site.time | date_to_string: "ordinal", "US" }}
Is there an option for China? I type in

{% assign postsByYearMonth = site.posts | group_by_exp:"post", 'post.date | date_to_string: "ordinal", "CHN"' %}

and it doesn't work!
image
Expectation is the output:

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
has-pull-request Somebody suggested a solution to fix this issue needs-decision 🤔
Projects
None yet
Development

No branches or pull requests