Make DateInterval objects uncomparable

[nikic]

Fixes https://bugs.php.net/bug.php?id=70810 among others.

The problem is that right now DateInterval uses normal object property comparison. As it doesn't have properties, all DateIntervals are considered equal, which is ... bad.

I've went through a couple iterations here, but in the end went with the simplest variant: When trying to compare two DateInterval objects, throw a warning and return 1 (uncomparable).

The problem here is that there are some intervals like P1M vs P30D that just don't have a well-defined comparison (in absence of a start point). We could carve out some special rules (e.g. consider DateIntervals equal if all components are equal), but I think we might be better off forbidding this entirely. A well-defied comparison of DateIntervals is possible by adding them to a start DateTime and comparing the result.

[frederikbosch]

@nikic Thanks for your work here. Would it be possible to add a __toString method DateInterval? Then developers can still do (string)$interval1 === (string)$interval2. Moreover, __toString would allow easy human-readable serialization of this object.

[nikic]

@frederikbosch What format would __toString() return though (especially for "special" DateIntervals)?

[Antnee]

So this will completely break this? https://3v4l.org/O8Okj

[nikic]

@Antnee Yes. All comparisons will be false and calling var_export() will have no effect on comparison (which it really shouldn't...).

[frederikbosch]

@nikic __toString might return the same value as it was constructed with? What would you consider a special DateInterval, with milliseconds?

Return values could be PT1004199059S, PT130S, PT2M10S, P1DT2S, -P1Y, or P1Y2M3DT5H20M30.123S.

[nikic]

@frederikbosch By special I meant things like DateInterval::createFromDateString("next monday").

[frederikbosch]

@nikic Yeah, I forgot that that is possible. When #3887 is merged, __toString could throw in (special) cases that serialization is not possible.

[Antnee]

Hmm, I can see that there are cases where we shouldn't be doing this, @nikic, especially as the behaviour is undefined. But I also know that in cases such as the example in my link where this does currently work, where the interval was calculated via a DateTime::diff(), that this is in production and will break

[nikic]

@Antnee To clarify, you are saying that you are calling var_export() on DateInterval objects and subsequently comparing them, in production code?

[Antnee]

I'm not at the moment, @nikic, but I know that some code that does this exists in production

[nikic]

Okay, then their code will break (in a nice way: with a warning). If you put this kind of hack in production, then you need to be prepared to deal with the inevitable consequences.

I'm happy to discuss alternatives here to allow DateInterval comparisons in some well-defined subset of cases, but the fact that some people thought it was a sensible idea to exploit a var_export side-effect should not prevent us from making a change here.

[frederikbosch]

I agree with you @nikic. The alternative could be to return the correct comparison when possible. This is possible if the interval represents a constant. That is: the interval represents the same amount of time at any given moment. This would be the case if the interval can be converted to (milli)seconds. Would that be possible (reasonably with the current code) at all?

[derickr]

There shall not be a _toString method for this, as it is not possible to pick a format that would satisfy everybody. The DateTime class does not have one either.

[nikic]

I'm thinking that the exception we can carve out here is something like this:

• Comparison of special/relative DateIntervals is always forbidden.
• Comparison of DateIntervals that only have components up to (and including) days is always allowed.
• Comparison is also allowed if there is a days component, because that disambiguates the correct interpretation of y/m/d. In particular this includes DateTimes created via DateTime::diff().

Would that be sound? The only potential issue I would see here are leap seconds.

[derickr]

PHP doesn't care about leap-seconds, so that won't be a problem. I also don't think it's wise to allow comparisons for some intervals, but not others.

[derickr]

I'm OK with the patch as it stands.

[frederikbosch]

@derickr Right, I think no __toString is better indeed. However, I do feel there is a need for a ->format() that returns the P1DT2S format.

[Antnee]

Okay, then their code will break (in a nice way: with a warning). If you put this kind of hack in production, then you need to be prepared to deal with the inevitable consequences.

I'm happy to discuss alternatives here to allow DateInterval comparisons in some well-defined subsets of cases, but the fact that some people thought it was a sensible idea to exploit a var_export side-effect should not prevent us from making a change here.

I agree. I wrote some code that did this a few years ago, and I always expected that it would break one day, although I'd hoped that it would work without needing to be inspected first, rather than the comparison being taken completely away. I appreciate the oddities where P1M and P30D could be equal in some cases, which is why properties like days are useful. HOWEVER...

If you only care about the days, you can currently compare those. So this (crude) example shows comparing whether January has more days than Feb, which is fine. So we can compute an interval and compare which one is larger by this property, and you don't need to inspect the object first - this just works (and we confirm the number of days later).

If we need to look at something other than just days, though, it doesn't work. You have to check loads of properties manually, which is a pain. So comparing the objects works nicely: https://3v4l.org/5QC3W

The alternative must surely be something like this, right? https://3v4l.org/QVSfC

So we just calculate the total number of seconds and it works for me

[Antnee]

OK, so even that doesn't work...

https://3v4l.org/hg2q5

Needs some additional handling of the days/d property to get a better comparison...

https://3v4l.org/btf87

See, it's a pain to compare the two reliably

[nikic]

PHP doesn't care about leap-seconds, so that won't be a problem. I also don't think it's wise to allow comparisons for some intervals, but not others.

The main case I'm concerned about here is comparison of DateIntervals produced by doing a $date1->diff($date2). Two such intervals always have a well-defined comparison and it would be nice if comparing them would "just work".

Could we maybe say that only DateIntervals that are created by DateTime::diff() (i.e. have days) are comparable? That would allow DateInterval comparison for one pretty useful case, while also having a very simple rule about what can be compared and what can't.

[derickr]

@nikic I still think it's messy. What I think would be best is that if we have a comparable DateInterval (ie. with a days property), we create a subclass (DateAbsoluteInterval?) that can then have a working conparator. The old DateInterval will than have one that always errors.

[frederikbosch]

@derickr That is a good consideration, and definitely better than having different behaviour when comparing different intervals. I still have one question. The reason I opened the bug report referenced in the start of the thread to begin with was to detect the fact that new DateInterval('P1D') does not equal new DateInterval('P1Y'). See https://3v4l.org/r9ZHV.

var_dump(new DateInterval('P1D') == new DateInterval('P1Y'));
var_dump(new DateTime('2010-01-01') == new DateTime('2010-01-01'));

When using DateTime one is able to detect equality/inequality, while it is not for DateInterval. Would it be an option to compare equality and return true/false without warning while comparing greater/smaller would return always false with a warning?

[nikic]

@frederikbosch Apart from not being technically possible right now (there is no distinction between equality and inequality comparisons), restricting this to equality doesn't really help, because the P1M vs P30D issue stays the same. Those could be either equal or not equal.

[frederikbosch]

@nikic I was not proposing different comparison for equality and inequality. From my point of view those two (P1M and P30D) are always not equal while it is ambiguous who of the two is bigger than the other. My suggestion was to discover whether the following would be a considerable alternative.

new DateInterval('P1M') == new DateInterval('P30D') // false
new DateInterval('P1M') != new DateInterval('P30D') // true
new DateInterval('P1M') == new DateInterval('P1M') // true
new DateInterval('P1M') != new DateInterval('P1M') // false

new DateInterval('P1M') < new DateInterval('P1M') // false
new DateInterval('P1M') > new DateInterval('P1M') // false

new DateInterval('P1M') > new DateInterval('P30D') // false + warning
new DateInterval('P1M') < new DateInterval('P30D') // false + warning

So my suggestion would be that == and != are always possible. > and < are only possible when the two objects have the same (internal) value. Other cases will return false and trigger a warning.

[nikic]

I was not proposing different comparison for equality and inequality.

To be clear, by "inequality" I was referring to < > etc here, not !=.

From my point of view those two (P1M and P30D) are always not equal while it is ambiguous who of the two is bigger than the other.

That doesn't really make sense to me. Stepping away from that particular problematic example, I would very much expect that PT1M and PT60S compare equal. They represent exactly the same interval and will always behave identically in arithmetic -- they just use two different storage representations.

[frederikbosch]

@nikic Clear. Then this solution is best, and the suggestion by @derickr can be an enhancement for the future.

[nikic]

Merged as 3cfbbf2 into 7.4. I'll look into implementing Derick's suggestion as a followup.

[frederikbosch]

Thanks for the work!

[frederikbosch]

This does not have a NEWS entry yet. Maybe add it after #4063 was merged?