Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Bug #60774 (DateInterval::format("%a") is always zero when an interval i... #148

Closed
wants to merge 1 commit into from

4 participants

@lonnylot

Bug #60774 (DateInterval::format("%a") is always zero when an interval is created using the createFromDateString method)

Fixed using the same method bug 49778 used.

EC2 Default User Bug #60774 (DateInterval::format("%a") is always zero when an interva…
…l is

created using the createFromDateString method)
8aa0771
@smalyshev smalyshev commented on the diff
ext/date/tests/bug60774.phpt
((15 lines not shown))
+ int(0)
+ ["d"]=>
+ int(2)
+ ["h"]=>
+ int(0)
+ ["i"]=>
+ int(0)
+ ["s"]=>
+ int(0)
+ ["invert"]=>
+ int(0)
+ ["days"]=>
+ bool(false)
+}
+2
+(unknown)
@smalyshev Owner

I'm not sure I understand - why it's unknown if the string says "2 days"?

@lonnylot
lonnylot added a note

The %a modifier is only for the total number of days between two dates. Since there is only 1 date we don't have a difference.

@lonnylot
lonnylot added a note
@smalyshev Owner

There's actually no dates at all in this example, just the interval. And I thought the whole point of the interval is that you don't need two dates - you just have relative interval, like "2 days interval". I don't see why %a there can't be 2 days.

As far as I'm aware the %a is supposed to be the total number of days in the interval. I think you're right that if the interval is 2 days then %a could be set to 2. However, with other intervals, such as 1 month, you would not be able to set the %a unless the interval was relative to todays date.

Additionally, the example in the createfromdatestring docs say that instantiating from the constructor or createfromdatestring static method should create the same results. In bug 49778 Derick changed the the constructor so that %a would be (unknown). My patch makes the results of createfromdatestring match the constructor results.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@derickr
Owner

As comments indicated, %a should not have filled in something - only %d should have that. %a is a calculated value that only make sense if the interval is linked to two dates. I am not quite sure what I like it to say "(unknown)" though... just an empty string might be better?

@lonnylot

@derickr I am not against the change, but I want to also bring up that it would be a bigger BC break if we changed "(unknown)". The output would have to change for both the constructor AND the static call to createFromDateString. The constructor was changed to be "(unknown)" in 2010 (http://svn.php.net/viewvc/?view=revision&revision=295928).

If the BC break is fine w/ everyone I think we should also send an E_WARNING. That way people become aware the format('%a') should only be used when there is a difference between TWO dates and they are using it incorrectly. Hopefully that will help to alleviate any confusion between %a and %d.

@derickr
Owner

I wouldn't like a warning here. There is no reason for it and they are difficult to deal with. You can already detect this problem by seeing "(unknown)".

@lonnylot

Did I misunderstand your previous reply[1]? I thought you were suggesting to change "(unknown)" to ""?

If we change "(unknown)" to "" then people will start testing w/ empty(). To prevent people from doing that I think it would make sense to warn them that they are using it wrong and that "" is not the same as 0.

Of course, my opinion is only valid if we are changing "(unknown)" to "". If we aren't then I agree we should not send an E_WARNING and we should just return "(unknown)" like we currently do w/ the constructor.

[1] #148 (comment)

@derickr
Owner

I was just saying that I don't like "(unknown)"—I hadn't realised that was used before already, so no need to change it now.

@lonnylot

Any chance of this making it into 5.5?

@derickr
Owner

I've merged it after adding the .c parser files too, but for some odd reason I can't close this PR.

@derickr derickr closed this
@lonnylot

Can someone close the related bug https://bugs.php.net/bug.php?id=60774

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Aug 5, 2012
  1. Bug #60774 (DateInterval::format("%a") is always zero when an interva…

    EC2 Default User authored
    …l is
    
    created using the createFromDateString method)
This page is out of date. Refresh to see the latest.
View
3  ext/date/lib/parse_date.c
@@ -1,4 +1,4 @@
-/* Generated by re2c 0.13.5 on Mon Dec 5 22:02:27 2011 */
+/* Generated by re2c 0.13.5 on Sun Aug 5 16:55:20 2012 */
#line 1 "ext/date/lib/parse_date.re"
/*
+----------------------------------------------------------------------+
@@ -24823,6 +24823,7 @@ timelib_time* timelib_strtotime(char *s, int len, struct timelib_error_container
in.tzdb = tzdb;
in.time->is_localtime = 0;
in.time->zone_type = 0;
+ in.time->relative.days = TIMELIB_UNSET;
do {
t = scan(&in, tz_get_wrapper);
View
1  ext/date/lib/parse_date.re
@@ -1830,6 +1830,7 @@ timelib_time* timelib_strtotime(char *s, int len, struct timelib_error_container
in.tzdb = tzdb;
in.time->is_localtime = 0;
in.time->zone_type = 0;
+ in.time->relative.days = TIMELIB_UNSET;
do {
t = scan(&in, tz_get_wrapper);
View
30 ext/date/tests/bug60774.phpt
@@ -0,0 +1,30 @@
+--TEST--
+Bug #60774 (DateInterval::format("%a") is always zero when an interval is created using the createFromDateString method)
+--FILE--
+<?php
+$i= DateInterval::createFromDateString('2 days');
+var_dump($i);
+echo $i->format("%d"), "\n";
+echo $i->format("%a"), "\n";
+?>
+--EXPECT--
+object(DateInterval)#1 (8) {
+ ["y"]=>
+ int(0)
+ ["m"]=>
+ int(0)
+ ["d"]=>
+ int(2)
+ ["h"]=>
+ int(0)
+ ["i"]=>
+ int(0)
+ ["s"]=>
+ int(0)
+ ["invert"]=>
+ int(0)
+ ["days"]=>
+ bool(false)
+}
+2
+(unknown)
@smalyshev Owner

I'm not sure I understand - why it's unknown if the string says "2 days"?

@lonnylot
lonnylot added a note

The %a modifier is only for the total number of days between two dates. Since there is only 1 date we don't have a difference.

@lonnylot
lonnylot added a note
@smalyshev Owner

There's actually no dates at all in this example, just the interval. And I thought the whole point of the interval is that you don't need two dates - you just have relative interval, like "2 days interval". I don't see why %a there can't be 2 days.

As far as I'm aware the %a is supposed to be the total number of days in the interval. I think you're right that if the interval is 2 days then %a could be set to 2. However, with other intervals, such as 1 month, you would not be able to set the %a unless the interval was relative to todays date.

Additionally, the example in the createfromdatestring docs say that instantiating from the constructor or createfromdatestring static method should create the same results. In bug 49778 Derick changed the the constructor so that %a would be (unknown). My patch makes the results of createfromdatestring match the constructor results.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.