[VarDumper] Fix interval caster with PT3600S-like spec #23793
Conversation
| @@ -52,6 +52,10 @@ public static function castInterval(\DateInterval $interval, array $a, Stub $stu | |||
|
|
|||
| private static function formatInterval(\DateInterval $i) | |||
| { | |||
| if ($i->m >= 12 || $i->d >= 31 || $i->h >= 24 || $i->i >= 60 || $i->s >= 60) { | |||
There was a problem hiding this comment.
are you sure about $i->m >= 12 ? "One month" is different than "30 days" (or 28,29,31)
There was a problem hiding this comment.
Is it maybe better to use the total number of days (%a)? See https://3v4l.org/pPkNZ
| ; | ||
| $format = '%R '; | ||
|
|
||
| if ($i->m >= 12 || $i->d >= 31 || $i->h >= 24 || $i->i >= 60 || $i->s >= 60) { |
There was a problem hiding this comment.
so, do we need $i->d >= 31 ? If yes, should it be $i->d >= 29?
There was a problem hiding this comment.
It should be $i->d >= 28 in fact. See https://3v4l.org/eY28U
29b4f53 to
4e0e888
Compare
| @@ -173,6 +173,8 @@ public function provideIntervals() | |||
| array('P5M', 0, '+ 5m', null), | |||
| array('P6Y', 0, '+ 6y', null), | |||
| array('P1Y2M3DT4H5M6S', 0, '+ 1y 2m 3d 04:05:06'.$ms, null), | |||
| array('PT3600S', 0, '+ 01:00:00'.$ms, null), | |||
| array('P60D', 0, '+ 60d', null), | |||
There was a problem hiding this comment.
can you add an example with 1 month + 32 days? I would expect an output that says "1 month + 32 days" :)
| $format = '%R '; | ||
|
|
||
| if ($i->m >= 12 || $i->d >= 28 || $i->h >= 24 || $i->i >= 60 || $i->s >= 60) { | ||
| $i = date_diff($d = new \DateTime(), date_add(clone $d, $i)); // recalculate carry over points |
There was a problem hiding this comment.
new \DateTime means we change the meaning of the interval: "1 months from now" means something different every month... unless we can really make the representation independant from "now", I think we shouldn't deal with carries.
There was a problem hiding this comment.
Yes, it's why the only right way to do this is to use the total number of days (%a format). It's invariable even according to the current month. See the previous example https://3v4l.org/pPkNZ
There was a problem hiding this comment.
what if the interval says 1 month + 32 days?
There was a problem hiding this comment.
Oh sh*t, you are right https://3v4l.org/JaivD 😮
There was a problem hiding this comment.
As months and years are dependant from "now", the only right way is to recalculate carry over points only in 0 month/year case (and with the total number of days as format). This PR will always be useful in most cases, like day interval (PxD) and time interval (PTx).
|
Ready to merging? |
|
Thank you @maidmaid. |
…idmaid) This PR was squashed before being merged into the 3.4 branch (closes #23793). Discussion ---------- [VarDumper] Fix interval caster with PT3600S-like spec | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | / | License | MIT | Doc PR | / According [ISO 8601](https://fr.wikipedia.org/wiki/ISO_8601): > The standard does not prohibit date and time values in a duration representation from exceeding their "carry over points" except as noted below. Thus, "PT36H" could be used as well as "P1DT12H" for representing the same duration. But this *breaks* ``interval`` representation, and this PR improves this. ```php dump(new DateInterval('PT3600S')); // before DateInterval { interval: + 00:00:3600.0 } // after DateInterval { interval: + 01:00:00.0 } ``` Commits ------- 046f8c1 [VarDumper] Fix interval caster with PT3600S-like spec
According ISO 8601:
But this breaks
intervalrepresentation, and this PR improves this.