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

DTSTAMP must be specified in UTC #432

Merged
merged 2 commits into from Oct 26, 2018
Merged

DTSTAMP must be specified in UTC #432

merged 2 commits into from Oct 26, 2018

Conversation

Tragen
Copy link
Contributor

@Tragen Tragen commented Oct 25, 2018

DTSTAMP must be specified in UTC
See https://www.kanzaki.com/docs/ical/dtstamp.html

@codecov
Copy link

codecov bot commented Oct 25, 2018

Codecov Report

Merging #432 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #432   +/-   ##
=========================================
  Coverage     98.87%   98.87%           
  Complexity     1805     1805           
=========================================
  Files            65       65           
  Lines          5157     5157           
=========================================
  Hits           5099     5099           
  Misses           58       58
Impacted Files Coverage Δ Complexity Δ
lib/Component/VEvent.php 100% <100%> (ø) 11 <0> (ø) ⬇️
lib/Component/VJournal.php 100% <100%> (ø) 7 <0> (ø) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 18e76a3...595ffce. Read the comment docs.

@staabm
Copy link
Member

staabm commented Oct 26, 2018

I am wondering, that we dont need to adjust a testcase. I guess this method is not covered.

@Tragen would you mind adding a testcase? dont worry if you dont feel confident enought to do it though.

@DeepDiver1975
Copy link
Member

I am wondering, that we dont need to adjust a testcase. I guess this method is not covered.

php env has timezone set to utc? In this case this change would have no impact - right?

@evert
Copy link
Member

evert commented Oct 26, 2018

You can't assume the timezone is set to UTC though. It's highly recommended but often not true.

@DeepDiver1975
Copy link
Member

You can't assume the timezone is set to UTC though.

sure - my comment was about the fact that tests are passing with this change

@Tragen
Copy link
Contributor Author

Tragen commented Oct 26, 2018

I'm sorry. I have no idea how to write the unit tests for sabre.
I was only checking why we had a wrong date and it was a simple fix.

I think we have some more bugs when the server isn't running with UTC.
We will test it and I will create new tickets then.

@staabm staabm merged commit a2a813d into sabre-io:master Oct 26, 2018
@staabm
Copy link
Member

staabm commented Oct 26, 2018

Ok, no problem. Thx for the PR though 🤟

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