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

Ensure time parser raises if timezone is missing #5382

Conversation

straight-shoota
Copy link
Member

@straight-shoota straight-shoota commented Dec 14, 2017

Reviewing the changes from #5317 I noticed that the time parser does not raise if it expects a time zone but there is none. Until now it only raised if the format was detected as invalid, but not if it was entirely missing.

There might be similar issues in the time parser, but most other fields are at least somewhat tested while %z is not (#5324 will add some tests for %z).

/cc @bcardiff

@RX14
Copy link
Member

RX14 commented Dec 14, 2017

From your commit message it's very hard to tell if you mean

Time parser raises if timezone is missing [and it's a bug, which this commit fixes]

or

[Ensure] Time parser raises if timezone is missing

Please use the latter style for commit messages. If it helps, imagine telling someone what to do.

@straight-shoota
Copy link
Member Author

straight-shoota commented Dec 14, 2017

Fixed. Thanks @RX14

Now this has brought forward an additional issue: The formats by HTTP.parse_date expect %z to match GMT and interpret it as UTC. In fact it doesn't. This was silently ignored and the output still worked so nobody noticed.
The only time zone identifiers %z matches beside numerical offsets are Z and UTC both representing Time::Kind::UTC (or Time::Location::UTC in #5324).
To solve this, we could either let the parser accept GMT as a timezone string or add additional patterns to HTTP::DATE_FORMATS which include GMT instead of %z. I don't think we should change the semantics of %z just to accomodate this single use case. GMT can be used as a time zone identifier and it shouldn't cause to much trouble if it is accepted, but one should not expect that.

The implementation of HTTP.parse_date is very primitive anyway so it shouldn't matter to put some more formats to try into HTTP::DATE_FORMATS as a quick intermediary solution.

A proper overhaul is proposed in #5123 which implements a customized parser for http date strings in Time::Format::HTTP_DATE (and also adds a time_zone_gmt method to the parser).

@straight-shoota straight-shoota changed the title Time parser raises if timezone is missing Ensure time parser raises if timezone is missing Dec 14, 2017
@jkthorne
Copy link
Contributor

@straight-shoota what is the status of this PR?

@straight-shoota
Copy link
Member Author

It's waiting for a review.

@RX14
Copy link
Member

RX14 commented Jun 2, 2018

I don't see the downsize in %z parsing GMT?

@straight-shoota
Copy link
Member Author

I'm not sure about it. IMHO it doesn't make sense to change the semantics of date format parsing just because HTTP dates use GMT as UTC. We can certainly discuss if %z should be extended, but I don't see a real reason for this.

#5123 has a proper implementation of date parses for HTTP and other protocol purposes which will eliminate the need for using %z for this anyway. I'm pretty much in favour of merging that instead of this PR which was only intended as a quick fix (six months ago...).

@RX14
Copy link
Member

RX14 commented Jun 4, 2018

%z is meant to parse a zone,GMT is a zone, why not parse it?

@straight-shoota
Copy link
Member Author

No, it parses a time zone offset. Time zone abbreviations in general are ambiguous.

@bcardiff
Copy link
Member

Closed in favor of #5123

@bcardiff bcardiff closed this Jun 10, 2018
@straight-shoota straight-shoota deleted the jm-time-parser-raise branch June 10, 2018 11:30
@straight-shoota straight-shoota restored the jm-time-parser-raise branch June 10, 2018 11:37
@straight-shoota
Copy link
Member Author

@bcardiff #5123 lacked the spec to ensure the time format parser raises if the time zone is missing. Thanks for stepping in, but I'd have liked to finialize that PR myself before it gets merged.

Now I've rebased this one, please reopen.

@bcardiff
Copy link
Member

Somehow the reopen button is disabled.

I understood from the rights to edit the PR you were ok to jump in (and you did already lots) but i will don’t do it again on your PRs.

Please creat another PR.

@straight-shoota
Copy link
Member Author

Maybe because I deleted the branch in between? dunno.

I allow maintainers to edit my PRs, but I see this mostly as a means to step in if the original author doesn't respond, needs help or have the time to address changes. Apart from such situations, I don't think it is really useful to edit someone else's PRs without coordination.

You should now I'm not offended or anything. I just think it's not very effective.

#6174

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.

None yet

4 participants