DateFormatter::formatInterval() doesn't format monthly intervals correctly

Makes sense, but D6 is feature frozen, bumping to D7.

Here for D7.

This worth a WTF tag.

Committed to CVS HEAD. Changing version to Drupal 6 for Gabor.

Patch here

I don't understand why 2592000 (30x24) makes sense. It makes more sense to me to use 2628000 (year 31536000 / 12)... I guess it's a moot point now. Oh well. :)

For instance, on admin/settings/statistics, we now get the following odd intervals that will have to be changed with an upgrade path:
1 month, 3 weeks
3 months, 3 weeks

I'll have to search for all our uses of format_interval to see where we now have "odd" intervals.

For D7, I think we should use calendar approximations for months and years, because, for everyone:

* May 30, 2009 is one month ago when compared to April 30, 2009
* April 30, 2008 is one year ago when compared to April 30, 2009

Returning to D7.

Given that the length of a month can vary by ~10%, it seems like this is going to be tricky to do? I guess that's why months weren't in there in the first place...

For a simple solution, I think Dave Reid's suggestion makes the most sense. At least that way you avoid having format_interval() tell you things like 364 days = "12 months 4 days", but then one day later, 365 days = "1 year". That's probably a much bigger WTF than the code that it replaced :)

Changing this to a bug report, as per #7.

Here is a patch which uses PHP's internal date interval functions instead of guessing metrics.

All dates (eg months) are relative to the current date, so 30 days from Jan 1 is a different result to 30 days from February 1 giving a higher quality result.

W00t, this is awesome. Could you write a test post it without the patch to show it failed and then with the patch to show what it fixed and making sure we do not regress?

I have written some basic tests which can be easily extended.

It shows the old version failing for a few dates and the new version succeeding with these dates.

All dates (eg months) are relative to the current date

As @tassoman explained in #13, we can't make this assumption.

The only way to do something like the patch above would be if we decide we want to change the function signature of format_interval() so that it takes separate start dates and end dates as parameters (rather than a single $interval parameter). However, that might make some use cases difficult as the code calling format_interval() on some data might not always have enough information to know what those are.

Hey David_Rothstein,

Instead of modifying the functionality of format_interval, how about having a new function, format_difference for example, which takes two date paramters and calculates the interval between them.

That would definitely work, but doesn't quite solve the main issue here, which is that format_interval() itself is broken :)

But I guess it would be possible to do both things (write a new function using the more sophisticated method, and fix format_interval() more along the lines of #7).... not sure what others think of that.

Linking to a related issue, which is basically implementing the new function as discussed above. (Technically that could have been a duplicate of this one, but it has a patch that's further along than the one here. However I left a note that maybe it could incorporate some of the patch here, such as the tests.)

I think the problem that format_interval() doesn't format monthly intervals in a reasonable way still remains, though?

On that other issue, what we've done is to define a formatDiff() method that, if you actually are comparing two specific datetimes, such as "now and the last time cron was run", it uses PHP date diffs to get a real idea of the months/years. And it also, in 8.x anyway, fixes core so that it uses formatDiff() instead of the flawed formatInterval().

We probably still need formatInterval to cover the case wehre you are not comparing two dates, such as representing "Run cron every X seconds but display that in a more human-friendly way as every 2 days instead of every 340398432 seconds or whatever".

For that case, it is not possible to do better than approximating the months/years, however, because it's just a bare interval in numbers of seconds, not a comparison of two specific dates/times. So... formatInterval() currently is saying "1 month == 30 days and 1 year == 365 days"... I think the best we can do is to document that this is what is being used and leave it at that.

That is done on the other patch... what else do you think we can do here? I mean, we could choose some other approximation of months/years but ... wouldn't really improve things in a meaningful way? The problem is that there simply isn't an exact way to convert "a bare interval of X seconds" to "Y months", without knowing what the starting point is. You can convert "X seconds from now", using the new formatDIff() function on the other issue or the patch here, but you cannot convert "X seconds in general" in an exact way.

So... I do think this is a duplicate of the other issue, but I won't close it...

So... formatInterval() currently is saying "1 month == 30 days and 1 year == 365 days"... I think the best we can do is to document that this is what is being used and leave it at that.

That is done on the other patch... what else do you think we can do here?

Well, the current approach leads to some nonsensical behavior like "364 days" => "12 months 4 days". And then worse, if you add one day to that, "365 days" => "1 year", so apparently it thinks a year is more than 12 months. See the related issue linked to from here (#2430529: "Member for" date format has a funny way of describing 11 years).

I think a good goal would be to have it never print something that's totally nonsensical to the human eye. This could be done by having some kind of a variable length month calculation so that 12 months equals 1 year. Some examples:

1. Assume the first 11 months are 30 days, and the 12th is 35 days. Then you get everything the same as now except in situations like above ("12 months 4 days" would become 11 months 4 weeks (6 days)" which seems like an improvement. I think this would be backportable to Drupal 7 also.
2. Assume months alternate roughly between 31 days and 30 days, hitting 365 days for 12 months in the end.
3. Assume the interval is always measured from January 1 and use the actual number of days per month.

The last two have some advantages but also make it a little more annoying for someone who deliberately wants to get "6 months" out of the function (they'd have to look into the function internals and count up the correct number of days to pass in).

We could also do what Dave Reid suggested earlier, use 2628000 seconds per month (1/12 of a year). I was thinking that might be a problem because it means a month won't be treated as an integer number of days, though. But maybe that doesn't matter in practice.

Yeah... So let's think about the goals and/or use cases etc. of formatInterval() given that we have formatDiff() from the other patch. A few thoughts:
- formatInterval() would hopefully only ever be used to format integer numbers of seconds, representing intervals that are not associated with a particular start date. Are there actually any use cases for this? I think in Core there are not.
- One possible use case would be where you are making a select list for something like how often to run Cron. In this case, usually the options are up to maybe a week, with only a granularity of 1, and in this case formatInterval works fine (it only has problems for month/year).
- If you really did want an exact month/year interval as an option, such as "Send people an email exactly 1 year after they joined", you couldn't use a defined number of seconds to do that anyway, no matter how smart formatInterval tried to be about representing integer number of seconds as months/years. So using formatInterval would not be appropriate here either. You'd instead need to do something else in your code, like storing a string of "1 year" and using PHP's date/time addition functionality to calculate if it's time to send the reminder.

So I just don't think we should even worry about this. The issue report here seems to be mostly covered by converting to use formatDiff(), and documenting formatInterval() so it says it's only good up to weeks and you should use formatDiff() if you have a start/end date/time... Am I missing a use case?

Well, statistics_settings_form() is an example that uses this for some larger numbers (up to a few months).

More generally, I think there are a fair number of use cases for storing a single number representing a time interval of some arbitrary length which would run into this issue (even if there aren't other cases in core).

My point is this:

If a module is displaying choices to a user that are actually a specific number of seconds, then formatting them as a specific number of months or years is kind of arbitrary, and probably the choice in the current formatInterval/format_interval functions of 30/365 are as good as anything, as long as they're documented, because the user of this strategy is just going to want to present a UI and use formatInterval to format the UI, so knowing they need to pass in 30 * 24 * 60 * 60 to get "1 month" out is good.

On the other hand, if a module wants to display choices that are actual time/date intervals like "1 month" or "3 years", then it should not be storing them as number of seconds, but as strings, and then using actual date interval addition, not seconds.

Any expectation that a specific number of seconds corresponds to a specific exact 'X months from now" interval is just wrong.

I'm pretty sure no one has an assumption that we use format_interval for "X time from now." It's for intervals only.

What I meant to say in #30 is that if you specify an interval for format_interval, it's a specific number of seconds. Translating that into a specific number of months is impossible.

So another use case where you might need to use formatInterval()... what if you are calculating the average duration of something? You can't have a start or end date for the average; you'll just have a number of seconds. And if the number corresponds to something a little under 11 years you definitely don't want it displayed as 10 years 12 months (taking the numerical example from #2430529: "Member for" date format has a funny way of describing 11 years).

I agree there aren't too many use cases where this matters once formatDiff() is in place, but since there are some I think we should still do something to improve this function (after the other issue is in place).

Yeah. So there are a couple of use cases.

a) Formatting a list of options for how often to run something like cron or other periodic processes.
==> In this case, I would argue that formatInterval() is actually the wrong way to do it:
- If it's crucial that this action happen in an exact number of months or years, then you need to store "3 months" in your module settings, not x thousands of seconds, and you need to test using a real date diff, not that a number of seconds has elapsed.
- If it's not crucial, just something like "If cron hasn't run in a month or so, run it now", then the module that puts up the settings form can just make the correspondence between the number of seconds it's waiting and the string it puts on the screen, without calling formatInterval.

b) Average intervals... OK, this is a legitimate use, but here the concept of "months" is intrinsically approximate. We need to fix the function so it behaves better. Ideally, it would be desirable if:
- The number of seconds in a month was an integer multiple of the number of seconds in a day.
- The number of seconds in a year was 12 times the number of seconds in a month.
- The number of seconds in a year was 365 times the number of seconds in a day.

It is not possible to choose values for number of seconds in a month/year to satisfy those three constraints. So the only way around it would be to use some logic to make this work better, rather than the simplistic "a month is X seconds" logic we are using now.

Like a zombie from the dead, I think this issue arose again. I just noticed this little gem on my user profile.

https://www.drupal.org/u/gchaix

History

On Drupal.org for 14 years 12 months

I think we should close this issue since it is fixed in D9. As noted in the comments for the formatInterval() method:

"Note that for intervals over 30 days, the output is approximate: a "month" is always exactly 30 days, and a "year" is always 365 days. It is not possible to make a more exact representation, given that there is only one input in seconds. If you are formatting an interval between two specific timestamps, use \Drupal\Core\Datetime\DateFormatter::formatDiff() instead."

In D9, we already do this:
\Drupal::service('date.formatter')->formatTimeDiffSince($account->getCreatedTime())

There may be a case for backporting the formatTimeDiffSince() functionality to Drupal7, but I'm not sure if it is worth the effort. At any rate, that would either be a new ticket or this one would be changed to only apply to D7.

Ahh good stuff I couldn't remember this having been fixed in Drupal 9 and didn't check the code yet. Going to close as outdated then, and Drupal.org can update to Drupal 9 eventually.