Date format granularity should only render adjacent units

So... This will mean that if the interval is for instance from January 3 2014 to February 2 2015, the output would be "1 year", and then the next day it would jump to "1 year 1 month". No days/weeks would be allowed after "1 year", only months.

Is that really what we want? I'm on board with not having "1 year 1 second", which although it has always been how the function has behaved, is definitely silly, but I'm not sure "1 year 3 weeks" is so silly. Thoughts? I guess it's not horrible to lose this.

Also we'll need to update #2456521: Add DateFormatter::formatDiff() as a non-buggy alternative to DateFormatter::formatInterval() when the start and end of the interval are known to do the same as formatInterval(), plus its tests. I would prefer to get that in first, then do the update of both formatDiff() and formatInterval() here, since that one is (again) RTBC and it's been going quite a while, but either way.

Yeah, I think I agree that we can lose all of the "skip a granularity level" outputs.

So. Nitpicks on the patch:

+      elseif ($parts) {
+       break;
       }

Needs 1 more space of indentation here (this is in formatInterval()). I also think it could use a comment to explain why we want to break in this case, something like:

Break if there was previous output but not any output at this level, to avoid skipping levels and getting output like "1 year 1 second".

Otherwise, this looks good to me. I checked over the tests, and I think there is adequate test coverage with the patched test, both for adjacent intervals working and for not getting things like "1 year 1 sec".

#3 addressed.

Do we have a preferred commit order for all of these date/timestamp related patches?

I think this is fine now. It will conflict with #2456521: Add DateFormatter::formatDiff() as a non-buggy alternative to DateFormatter::formatInterval() when the start and end of the interval are known though, which is up to 156 comments and I'd love to see it get in before we do any of the other patches... [just my personal "This has been going on long enough" preference]

We need to add this same fix to the new formatDiff() function from #2456521: Add DateFormatter::formatDiff() as a non-buggy alternative to DateFormatter::formatInterval() when the start and end of the interval are known (which just landed, woot!), and this may also need a reroll.

Looks pretty good! The test coverage is very thorough too.

I do have one suggestion, about the "break 2" code:

+++ b/core/lib/Drupal/Core/Datetime/DateFormatter.php
@@ -373,12 +381,21 @@ public function formatDiff($from, $to, $options = array()) {
+            if($parts > 0 && $weeks == 0) {
+              break 2;
+            }
+
             if ($granularity > 0 && $days > 0) {
               $interval_output .= ($interval_output ? ' ' : '') . $this->formatPlural($days, '1 day', '@count days', array(), array('langcode' => $options['langcode']));

I also don't like the "break 2" very much... so I think we can combine these two if() statements and drop the "break 2" line. So the 2nd if would become:

if (($parts == 0 || $weeks > 0) && $granularity > 0 && $days > 0) {

I think that's right... We'd better make sure we definitely have a case to test that
1 month 1 day
cannot be created as output in formatDiff, since this is handled with special logic!

Let's see, yes:

+++ b/core/tests/Drupal/Tests/Core/Datetime/DateTest.php
@@ -343,33 +343,33 @@ public function providerTestFormatDiff() {
-      array('2 months 1 day', $this->createTimestamp('2013-10-10 10:09:08'), $request_time),
-      array('2 months 2 days', $this->createTimestamp('2013-10-09 08:07:06'), $request_time),
-      array('2 months 2 days 2 hours', $this->createTimestamp('2013-10-09 08:07:06'), $request_time, $granularity_3),
-      array('2 months 2 days 2 hours 2 minutes', $this->createTimestamp('2013-10-09 08:07:06'), $request_time, $granularity_4),
-      array('6 months 2 days', $this->createTimestamp('2013-06-09 10:09:08'), $request_time),
-      array('11 months 3 hours', $this->createTimestamp('2013-01-11 07:09:08'), $request_time),
+      array('2 months', $this->createTimestamp('2013-10-10 10:09:08'), $request_time),
+      array('2 months', $this->createTimestamp('2013-10-09 08:07:06'), $request_time),
+      array('2 months', $this->createTimestamp('2013-10-09 08:07:06'), $request_time, $granularity_3),

So yes, we have the "if months and no weeks don't do days" cases covered. Excellent!

You know, I don't really like this $parts integer variable anyway. Really it's being used as a binary: it's "had we already made some output before we got to this point in the loop", right?

I think if we switched both functions to that logic, it might work better and be easier to read than having this cryptic variable called $parts that measures how many times we have made output, when really we just need to know if there is some output (which we could test by seeing if the output text variable evaluates to FALSE, like if($interval_output) or whatever).

Also in formatDiff() I was suggesting:

+            $weeks = floor($days / 7);
             if ($days >= 7) {
-              $weeks = floor($days / 7);

that if can now be

if($weeks)

Anyway, let's see where the failures are.

RE using $output as as check, sure. The new patch looks reasonable; still 3 fails...

Thanks! Looks good, and the tests pass!

OK so I found one in-code comment that I didn't like... this is a rather long explanation of why... but I think it's a bit misleading...

+++ b/core/lib/Drupal/Core/Datetime/DateFormatter.php
@@ -373,15 +378,21 @@ public function formatDiff($from, $to, $options = array()) {
+            if ((!$output || $weeks > 0) && $granularity > 0 && $days > 0) {
               $interval_output .= ($interval_output ? ' ' : '') . $this->formatPlural($days, '1 day', '@count days', array(), array('langcode' => $options['langcode']));
             }
+            else {
+              // If we have months but no weeks, explicitly set granularity to
+              // zero so we don't get output like "1 day 1 hour".
+              $granularity = 0;
+            }
             break;

Um...

The code comment in the else doesn't make sense to me.

The if part of this is basically "If we should be outputting days", so the else means "We are not outputting days".

The reason we are not outputting days is either that

(a) there were months but no weeks and we didn't want "1 month 1 day" output

or

(b) there was an even multiple of 7 days to render, so we output "1 month 1 week" and there were no days left over to output as "3 days".

And the reason we want to set the granularity to 0 in this else() is thus because we don't want to have hours, as the comment says, but it's not true that "we have months but no weeks".

So, that was kind of a long commentary...

I think this comment should say:

If we did not output days, set the granularity to 0 so that we will not output hours and get things like "1 week 1 hour".

Thanks, looks great!

Nice :)