Bug #60774 (DateInterval::format("%a") is always zero when an interval i... #148
Conversation
…l is created using the createFromDateString method)
| bool(false) | ||
| } | ||
| 2 | ||
| (unknown) |
There was a problem hiding this comment.
I'm not sure I understand - why it's unknown if the string says "2 days"?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
On Sun, Aug 5, 2012 at 11:37 PM, Stanislav Malyshev <
reply@reply.github.com
wrote:
- int(0)
- ["d"]=>
- int(2)
- ["h"]=>
- int(0)
- ["i"]=>
- int(0)
- ["s"]=>
- int(0)
- ["invert"]=>
- int(0)
- ["days"]=>
- bool(false)
+}
+2
+(unknown)I'm not sure I understand - why it's unknown if the string says "2 days"?
Reply to this email directly or view it on GitHub:
https://github.com/php/php-src/pull/148/files#r1309145
Lonny Kapelushnik
http://lonnylot.com
(732) 807-5509
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
Ping @derickr |
|
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? |
|
@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. |
|
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)". |
|
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) |
|
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. |
|
Any chance of this making it into 5.5? |
|
I've merged it after adding the .c parser files too, but for some odd reason I can't close this PR. |
|
Can someone close the related bug https://bugs.php.net/bug.php?id=60774 |
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.