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 issue #883, add/sub seconds/minutes/hours should handle DST correctly #1005

Closed
wants to merge 3 commits into from

Conversation

knutdx
Copy link

@knutdx knutdx commented Sep 7, 2017

I know there already is a pull request on this (Ovsyanka@eb6581b) but I think my solution may be cleaner. Seems to me that DateTime::modify is really bad when it comes to DST, so it is better to do these addXxx and subXxxx operations with timestamps instead. Since timestamps doesn't have timezone and doesn't care about DST.

@knutdx knutdx changed the title Fix issue #883 Fix issue #883, add/sub seconds/minutes/hours should handle DST correctly Sep 7, 2017
@KnutHelland
Copy link

The failing tests now are not because of code, but because of misconfiguration of the CI

@Glavic
Copy link
Collaborator

Glavic commented Feb 14, 2018

@knutdx: can you please rebase? TY

@Ovsyanka
Copy link
Contributor

Ovsyanka commented Feb 14, 2018

I don't think it is good approach because it will be inconsistent with the DateTime default behaviour. And you just came to decision we already tagged as wrong. Look at #883 (comment) There is lot of discussions, I suggest you to read it carefully.

@KnutHelland
Copy link

Fyi: when I submitted this pull request, I altso added a bug report and a merge request to phps timelib to fix this issue: https://bugs.php.net/bug.php?id=75167 there is a really ugly (and easy to fix) bug in timelib here, and it is annoying that noone has fixed it. Almost feels like the datetime library in php is abondoned.

@KnutHelland
Copy link

Pull request to the timelib used in php source: derickr/timelib#24

@KnutHelland
Copy link

So at least I think carbon could do a hack to compensate for that bug

@KnutHelland
Copy link

Sorry for all the comments, but I also have to say that I don't longer work with PHP and don't have a environment to test these things any more (and don't have access to that old work-account on github neiter), so I'll not rebase. But seems like you don't need it after all...

@kylekatarnls
Copy link
Collaborator

modify finally removed, we will kept the PHP native behavior and adopted it in the diffInSeconds/Minutes/Hours method all using intervals now.

So this could be done in other methods? For now, I have no better ideas than the following names:

addRealSeconds
addRealMinutes
addRealHours
diffInRealSeconds
diffInRealMinutes
diffInRealHours

So I will open a new issue for this and close this one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants