PHP 8.2 undocumented? behaviour change detection for strtotime

[aquavark]

Is your feature request related to a problem?

Related to an apparently undocumented change in behaviour in PHP 8.2 compared to prior versions - found because of a loop that pre-generates dates suddenly generating dates after a fixed point instead of before when negative offsets were used.

The output from strtotime has changed in behaviour in 8.2 compared to previous when doing addition of negative values. Foe example:
Pre-8.2 : date('Y-m-d',strtotime('2022-06-02 +-1 days')) gives 2022-06-01
In 8.2: date('Y-m-d',strtotime('2022-06-02 +-1 days')) gives 2022-06-03 (the negative is ignored)

Mathematically adding a negative number equates to a subtraction, and pre 8.2 this was handled that way. I can see from the specification this was an incorrect way to put this in to strtotime, however it did work (it occurs in code when, for example, you have a loop variable going from negative to positive and the +x days format was used with x replaced by a loop variable). The fix is to remove the + prefix entirely, because just putting "2022-06-02 1 days" works as if the + was present and doesn't break for "2022-06-02 -1 days" in any current PHP version.

Describe the solution you'd like

A sniff to detect when +- appears within a strtotime to alert of changed behaviour between pre 8.2 and 8.2.

I have looked, but cannot find this change actually documented anywhere, and I believe it was a case of previous behaviour not meeting the defined specification regex that starts with [+-]? then the digits iirc.

Additional context (optional)

Whilst a minor and possibly rare issue, it can cause siginificant but non-obvious disruption to code that calculates dates with offsets that otherwise was working in pre-8.2.

[mbabker]

It's probably related to the same change, but php/php-src#9950 describes what happened here in another context.

[aquavark]

@mbabker Thanks, I hadn't searched for the equivalent in the datetime object realm, but it does appear to be the same underlying change.

[jrfnl]

Thanks for reporting this @aquavark and for the addition of that reference @mbabker .

Based on reading through the linked issues, it's unclear to me which functions in PHP are affected, but it doesn't appear to be isolated to strtotime().

A sniff could be written to detect this in strtotime() and be expanded to encompass additional function calls/class instantiations taking the same kind of "date string" parameter at a later point in time when it becomes clear what additional functions are affected.

Am I correct in summarizing the specs as follows:

• Find function calls to strtotime() and potentially other affected functions.
• Grab the passed parameter in those function calls which potentially contains a "date string".
• Check if any tokens within that parameter are text string tokens and if they contain a +[ ]*- pattern (where [ ]* indicates optional whitespace in between).

Open question:

• Should it check for the above mentioned +[ ]*- pattern or should it check for a + no matter what ?

Side-note: Keep in mind, sniffs do not have access to the runtime value of variables, so if the parameter would be passed like any of the below, the sniff would not be able to detect it:

strtotime($time_string);
strtotime('2022-12-21 ' . $variable_part);

Also note that, based on 3v4l, the change appears to be limited to PHP 8.2+ and doesn't look to be ported to earlier 8.x patch versions, even though it is supposed to be a bugfix.
https://3v4l.org/bEiUU

[jrfnl]

Open question:
* Should it check for the above mentioned +[ ]*- pattern or should it check for a + no matter what ?

For the record: -[ ]*+ is not affected as the fix takes the first sign found, and -+ evaluates to -, so there is no change in behaviour for that pattern.

[aquavark]

From what I've tested myself, the + is entirely optional, so "2022-01-01 1 days" gives 2022-01-02 in all versions, and then works properly with negatives in all versions, it's only when the +- scenario occurs that it appears to break. That might be hard to detect though in loops where the value can be negative if you can't see the value range - maybe thats something I should raise in phpstan fora?

I hadn't testing -+value as I wouldn't normally prefix a loop value with a +, but I assume --value would also be broken (although mathematically that results in an addition)

My usage case for this was (rough code)

for ($loop=-7; $loop<8; $loop++)
{
 $thisdate = date('Y-m-d',strtotime('2022-06-16 +$loop days'));
 $array[$thisdate]=$loop;
}

The change in PHP8.2 led to array overwrite that was quite difficult to track down.

[jrfnl]

From what I've tested myself, the + is entirely optional, so "2022-01-01 1 days" gives 2022-01-02 in all versions, and then works properly with negatives in all versions, it's only when the +- scenario occurs that it appears to break.

I hadn't testing -+value as I wouldn't normally prefix a loop value with a +, but I assume --value would also be broken (although mathematically that results in an addition)

Well, in that case, any operators, other than maybe a stand-alone - could possible be set to trigger the error ?
In that case, the below would be flagged for having the + in the text string.

 $thisdate = date('Y-m-d',strtotime('2022-06-16 +$loop days'));

How does that sounds to you ?

That might be hard to detect though in loops where the value can be negative if you can't see the value range - maybe thats something I should raise in phpstan fora?

Worth a try.

[aquavark]

PHPStan equivalent request: phpstan/phpstan#8574

Your suggestion makes sense, as the + is optional, and triggers the issue if $loop is -ve. I don't know how prevalent it is that people include the optional + by default, I guess a scream test of this proposed sniff would reveal that :)

It does appear likely that the modify functions in the DateTime object are likely subject to this same issue, for completeness this was the example from the PHP bug referenced previously:

$a = (new \DateTimeImmutable('2022-05-01 22:00:00'))->modify('+-30 seconds');

[aquavark]

Just adding that the behaviour of --$loop or --number works as mathematically expected in versions 5.2.2 - 5.2.17, 5.3.0 - 5.3.29, 5.4.0 - 5.4.45, 5.5.0 - 5.5.38, 5.6.0 - 5.6.40, 7.0.0 - 7.0.33, 7.1.0 - 7.1.33, 7.2.0 - 7.2.34, 7.3.0 - 7.3.14, 7.3.16 - 7.3.33, 7.4.0 - 7.4.33, 8.0.0 - 8.0.26, 8.1.0 - 8.1.13, but not in 8.2

https://3v4l.org/ft66S