Fix DateInterval database value truncation (string overflow)

[vbartusevicius]

DateIntervalType now stores data in P%YY%MM%DDT%HH%IM%SS format as %M-%DT%H:%I:%S brings a lot of problems with large intervals, i.e.: P100DT500M

[MisatoTremor]

Despite my remark that 20 can be to short for large intervals, 255 might be a bit over the top. :)
Question is what a sensible limit would be ...
To have an interval spanning the seconds past since the "beginning" of the visible universe would be P00Y00M00DT00H00M435400000000000000S = 66 characters. I can't imagine right now any higher possible overflowing, but with adding some "buffer" length I think any value between 70 or 80 should be safe.

[vbartusevicius]

Yeah, but theoretically you can add as many numbers in any interval part as you want, so i.e. MySQL 64, 128 or other length lower than 255 (varchar) reservers same amount of memory, so is it really needed to calculate potential minimum?

[MisatoTremor]

Afaik that only applies to the memory storage type.
This being DBAL the length should not consider only one specific platform. And if I'm not mistaken a developer could adjust the length if needed when defining the schema.
So this is mainly about finding a sensible default length.

[Ocramius]

Tweaking is not really up to us - we just need to accommodate a generic use-case scenario, and that's it.

[Ocramius]

Linking #2314

[juliangut]

@Ocramius Do you plan to include this new type is next release? 2.4.6? or is it too soon?

[Ocramius]

New features can only go to minor versions, not to patch releases
On Feb 17, 2016 21:22, "Julián Gutiérrez" notifications@github.com wrote:

@Ocramius https://github.com/Ocramius Do you plan to include this new
type is next release? 2.4.6? or is it too soon?

—
Reply to this email directly or view it on GitHub
#2316 (comment).

[vbartusevicius]

@zeroedin-bill, @Ocramius - could you please review the code?

[juliangut]

You are right @Ocramius, then plans for 2.6 😉 ?

[Ocramius]

@vbartusevicius code looks ok to me. Assigned to @deeky666 for review.

@juliangut @deeky666 is the gateway keeper for DBAL: he's in command

[deeky666]

This comment is not needed, please remove.

[vbartusevicius]

@deeky666 - any luck on this?

[galeaspablo]

@vbartusevicius @deeky666 Let's not store an invalid \DateInterval

Invalid according to \DateInterval::__construct
From http://php.net/manual/en/dateinterval.construct.php

The specification can also be represented as a date time. A sample of one year and four days would be P0001-00-04T00:00:00. But the values in this format can not exceed a given period's roll-over-point (e.g. 25 hours is invalid).

We should convert any invalid \DateInterval passed to convertToDatabaseValue accordingly. For instance a \DateInterval with 61 seconds, should be stored as a \DateInterval with 1 minute and 1 second. Years supports more than 4 digits (new \DateInterval('P123456789Y12M4DT00H00M00S') works just fine), so increasing the number of year digits stored solves our problems for big intervals.

Or throw an exception in convertToDatabaseValue if an invalid \DateInterval is passed.

Note that invalid \DateIntervals will only come up if you create a new \DateInterval and then change the public properties themselves, without any regard for what's valid. So IMO, we should throw an exception.

[vbartusevicius]

@galeaspablo - what do you mean by increasing the number of years? this PR already solves the problem you're addressing here:

(new \DateInterval('P123456789Y12M4DT00H00M00S'))->format('P%YY%MM%DDT%HH%IM%SS');
// "P123456789Y12M04DT00H00M00S"

In addition, why new \DateInterval('PT61S'); looks invalid to you? You can easily manipulate such intervals:

$a = new \DateInterval('PT61S');
$b = new \DateTime('2016-12-15 12:00:00');
var_dump($b->add($a));

class DateTime#3 (3) {
  public $date =>
  string(26) "2016-12-15 12:01:01.000000"
  public $timezone_type =>
  int(3)
  public $timezone =>
  string(15) "Europe/Helsinki"
}

[galeaspablo]

@vbartusevicius Like I said,

Invalid according to \DateInterval::__construct
From http://php.net/manual/en/dateinterval.construct.php

The specification can also be represented as a date time. A sample of one year and four days would be P0001-00-04T00:00:00. But the values in this format can not exceed a given period's roll-over-point (e.g. 25 hours is invalid).

---

Plus, the PR does not keep sortability, which was considered for this feature. And should remain so. Your PR would mean the database would sort 10, 111, and 12 seconds as 10, 111, 12.

If you'd like to keep your approach, we'd need to str_pad() seconds, minutes, etc.

By increase the number of year digits, I mean that we store 10 digits of a year instead of 4 (as before), or 2 (as you propose).

Edit 1: Rethinking this, your approach with str_pad sounds like the way to go. Unless I'm missing something.

Edit 2: Actually, nevermind Edit 1. We need to keep ISO8601, and store valid (according to the specification intervals), to keep sortability. Otherwise 1 minute 10 seconds < 0 minutes 80 seconds

[vbartusevicius]

@galeaspablo - your case with invalid spec only applies to datetime representation (please read again):

The specification can also be represented as a date time. [...] But the values in this format can not exceed a given period's roll-over-point (e.g. 25 hours is invalid).

If you use a regular DateInterval spec, you can easily create valid date intervals with 30 hours or so:

new \DateInterval('PT30H61S');

just try this.

As of 2 digits for year - where do you see that? 'P%YY%MM%DDT%HH%IM%SS' - here %Y is a placeholder and Y is just a letter.

[galeaspablo]

@vbartusevicius As of 2 digits for year - where do you see that?

You are right. My mistake.

@vbartusevicius You can easily create valid date intervals with 30 hours.

True.

@galeaspablo The PR does not keep sortability, which was considered for this feature. And should remain so. Your PR would mean the database would sort 10, 111, and 12 seconds as 10, 111, 12.

You cannot address this by using str_pad() - see below

@galeaspablo store valid (according to the datetime specification), to keep sortability. Otherwise 1 minute 10 seconds < 0 minutes 80 seconds

So we're back to, throw an exception if we have an "invalid" (according to the datetime specification) interval.
Or converting, e.g. 61 seconds is stored as 1 minute 1 second.

[vbartusevicius]

@galeaspablo - thanks for information, but I like to hear the opinion of @deeky666 or @Ocramius on this.

[galeaspablo]

Agreed. :)

[MisatoTremor]

@galeaspablo

Or converting, e.g. 61 seconds is stored as 1 minute 1 second.

This could lead to invalid behavior, as it actually changes the meaning of that interval.
Consider this years leap second as an example. At 2016-12-31 18:59:59 there will be an additional second. So when using your example interval in a calculation 61 seconds from 18:59:00 will be 19:00:00. After converting it to 1 minute 1 second the same calculation would yield 19:00:01.
The same would apply to leap years and a possible day to month and day conversion.

[galeaspablo]

@MisatoTremor Happy to hear more.

So far, I see these options

1. Store DateIntervals, as they come. But lose enforced sort-ability at the database level (a database would tell you that 00001months00090days < 00002months00002days).
2. Only store DateIntervals, where values don't exceed their moduli (i.e. not 61 seconds, 61 minutes, 25 hours, etc). But force users who want to use something like 2 minutes, 1000 seconds, to use separate int columns.
3. Convert DateIntervals, where values exceed their moduli (i.e. 61 seconds becomes 1 minute, and 1 second). But, this changes the meaning of the interval.... Plus, how do we convert (for instance how many days and months is 35 days). Even though I proposed this, I can now see how this is a bad idea. 🗡

[MisatoTremor]

As I see it, DateIntervals can't be reliably sorted anyway so I'd choose 1).
2) Seems to restricting to the user to be worth it.

[deeky666]

I really can't get my head around that weird interval stuff, sorry. Will leave that one up for @Ocramius to decide :P Please just make sure to keep BC. Maybe only possible by introducing a new type (ugh).

[galeaspablo]

Thanks for the taking the time to chime in @deeky666 . As I mentioned in #2579,

Is this really a BC break? This isn't on 2.5.

[Ocramius]

This is ready to merge. Sorry for the delay, @vbartusevicius!

Also, for readers, this is not a BC break, as this class was not in any tagged release.

[MisatoTremor]

Thanks @Ocramius for merging and ofc thanks @vbartusevicius for your work on this!

[mattdillon100]

The conversion format is set to '%RP%YY%MM%DDT%HH%IM%SS'. This is incorrect for Postgres. The prepended %R is not accepted.

There is also an issue with the convertToPHPValue() method. The Postgres DB $value from an interval type field is in the format: %YY years %MM mons %DD days %HH:%IM:%SS.