-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Fixed issue #883 #884
Fixed issue #883 #884
Conversation
Very nice @Ovsyanka - let me have a look at it later |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some small changes, but looks good !
src/Carbon/Carbon.php
Outdated
if ($this->local) { | ||
return parent::modify($modify); | ||
} | ||
$shouldBeModifiedInUtc = preg_match('/(sec|second|min|minute|hour)s?/', $modify); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is a variable really useful here?
tests/Carbon/ModifyTest.php
Outdated
|
||
/** | ||
* @dataProvider getDealWithDayLightTests | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@param
docblocks missing :-(
tests/Carbon/ModifyTest.php
Outdated
$date = new Carbon($dateString, $timezone); | ||
$date->modify($modify); | ||
$this->assertSame($expected, $date->format('Y-m-d H:i:s P')); | ||
$this->assertInstanceOf('Carbon\Carbon', $date); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feel free to use assertInstanceOfCarbon
src/Carbon/Carbon.php
Outdated
$instance = parent::modify($modify); | ||
$this->setTimezone($timezone); | ||
if ($shouldBeModifiedInUtc) { | ||
$timezone = $this->getTimezone(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe use a negated assertion and an early return here ?
Okay! I will make done it very soon. |
👍 |
Oh, i need to clarify - how i have to handle it. Should i make another commit in that branch or just make another pull-request? it is my first real pull-request =) |
I wish all your next PRs could be as good as your first one then 👌!
You can simply add another commit to thir PR
Le dim. 5 févr. 2017 à 22:29, Igor <notifications@github.com> a écrit :
… Oh, i need to clarify - how i have to handle it. Should i make another
commit in that branch or just make another pull-request? *it is my first
real pull-request =)*
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#884 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAfWQ_2RMpeZ6reoqIGNbObPtA0RPSvdks5rZj9BgaJpZM4L10AV>
.
|
Actially i changed my mind a little, about early return. I think it is not good in this case. I think that it is fine to handle errors, when you just stopping the function and do not doing it's main task. But if you doing useful tasks in different manner, i surely suppose, that you have to use if-else block. It is more clearly to understand. |
Sure @Ovsyanka |
Okay, i already did this in the last commit before your commentю It contains all changes i supposed to do. |
First of all I like this fix. In my opinion it would be nice to have a small extension in the documentation of modify about when DST is included and when it is ignored. |
88d3bd5
to
0d94cc9
Compare
src/Carbon/Carbon.php
Outdated
// If we changing time in some timezone with daylight correction, there can be the case, | ||
// that this correction influence to time in nonwanted way. | ||
// So we have to make this modification in UTC timezone. | ||
if (!preg_match('/(sec|second|min|minute|hour)s?/', $modify)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
case-insensitive search ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great suggestion! I will fix it.
I rebased branch to actual master and made regexp case-insensitive. |
Just restarting Travis |
@kaufholdr wrote:
I totally agree with you, but i think it should be in another issue (and pull-request) because that is not nesessary to apply this pull-request. And i dont know where to place such documentstion. I think there is appropriate existed issue #890 |
@lucasmichot, why you do not merging this pull-request? Should i do something else? |
Would love to see this merged. We are experiencing the same issue and have had to create workarounds. |
Is anyone going to merge this? It's pending for so long. |
Ouch, I just got notified of this issue too. Looks like this PR deserves a bump :) |
src/Carbon/Carbon.php
Outdated
// If we changing time in some timezone with daylight correction, there can be the case, | ||
// that this correction influence to time in nonwanted way. | ||
// So we have to make this modification in UTC timezone. | ||
if (!preg_match('/(sec|second|min|minute|hour)s?/i', $modify)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically, this regex is equivalent to /sec|min|hour/i
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, what is the situation concerning the other modifiers ("day", "week", etc.)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically - yes, but this is more explicit and obvious for human reading. I will move the long regexp into comments and replace it by short one in the code.
Others is OK. Modifications by units bigger than hour leaves time of the day non changed.
You could add some tests for that, btw.
Hm, actually the pull-request have some tests for this already (day
unit).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had to dig a bit to find this documentation page: Relative Formats.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I realised that sec
matches to "ordinal" second
(like first
, second
, third
). But it must not. I am not sure about how to solve this. I have to think about that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose to make something like this: /[+-]?[0-9]+ (sec|min|hour)\w*( ago)?/i
.
The idea is: the unit
tokens goes only with numbers behind it and|or ago
after.
What do you think?
Upd: hmmm. I see potential problems. It could be
second hour of <some date>
(I have to investigate if it is a problem)10 hours
(If it just set the time of a day this shouldn't be "fixed")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vlakoff, Look at #883 (comment) plz.
All relative modifications, except seconds, minutes and hours was broken because modifications was forced processed in UTC timezone. Now only seconds, minutes and hours modifications processing in UTC. For example i have the date 2014-03-30 00:00:00 in Europe/London (new Carbon('2014-03-30 00:00:00, 'Europe/London')), then if i want to increase date by 1 day, i expect 2014-03-31 00:00:00, but if want to increase date by 24 hours, then i expect 2014-03-31 01:00:00, because in this timezone there will be that time after 24 hours (timezone offset changes because of Daylight correction). The same for minutes and seconds.
Any news? |
I am waiting for votes on my comment #883 (comment). I am not using Carbon by myself, so I don't want to do some work that I am not sure will be needed. |
I decide to rid off this pull-request because that is wrong approach to solve the problem. Look at the #883 (comment) for details. |
No description provided.