Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8.2.9 DateTime:createFromFormat stopped parsing datetime with containing extra space. #11854

Closed
mn7z opened this issue Aug 2, 2023 · 15 comments

Comments

@mn7z
Copy link

mn7z commented Aug 2, 2023

Description

Starting of 8.2.9 version \DateTime:createFromFormat() starts to fail to parse date format, due extra space.

The date string Wed Aug 2 08:37:50 2023 - with extra space between Aug and 2. If extra space is removed
then it works as usual.

PHP versions <8.2.9 correctly parses the format, I've tested with 8.2.7 version in addition to 8.2.8.

The following code:

<?php

$dateTime = \DateTime::createFromFormat("D M d H:i:s Y", "Wed Aug  2 08:37:50 2023");

var_dump($dateTime);

Resulted in this output:

bool(false)

But I expected this output instead:

object(DateTime)#1 (3) {
  ["date"]=>
  string(26) "2023-08-02 08:37:50.000000"
  ["timezone_type"]=>
  int(3)
  ["timezone"]=>
  string(3) "UTC"
}

PHP Version

PHP 8.2.9

Operating System

No response

@damianwadley
Copy link
Member

damianwadley commented Aug 2, 2023

(Note: 8.2.9 GA is planned for tomorrow and has already been tagged.)

@derickr Is it intentional that timelib_eat_spaces now only eats a single character? Because before it would eat as many as it could.
derickr/timelib@b00612f

@levu42
Copy link

levu42 commented Aug 2, 2023

This currently breaks Laravel's artisan serve command. PHP 8.2.9 has already been shipped on at least arch linux since yesterday https://archlinux.org/packages/extra/x86_64/php/.

Is there any chance for an 8.2.10 to be shipped quickly that fixes this regression?

@nielsdos
Copy link
Member

nielsdos commented Aug 2, 2023

I'm not Derick, but I'm pretty confident that it is unintentional that it now stops at consuming a single space.
The commit changed how it worked to support non-narrow breaking spaces (#11600).
I think I know how to fix it, so maybe I can fix it quickly and we can prevent the regression in the actual release.

nielsdos added a commit to nielsdos/php-src that referenced this issue Aug 2, 2023
nielsdos added a commit to nielsdos/php-src that referenced this issue Aug 2, 2023
@damianwadley
Copy link
Member

Is there any chance for an 8.2.10 to be shipped quickly that fixes this regression?

➡️ @saundefined and @adoy

@derickr
Copy link
Member

derickr commented Aug 2, 2023 via email

@Gemorroj
Copy link
Contributor

Gemorroj commented Aug 2, 2023

@derickr

But these fixes need to be done in derickr/timelib.

derickr/timelib#148

@derickr
Copy link
Member

derickr commented Aug 2, 2023 via email

@derickr
Copy link
Member

derickr commented Aug 3, 2023

I'm in the progress of addressing this regression. But to be precise, this is not a bug. The documentation for createFromFormat lists for the ' ' format:

Whitespace and Separators
(space) | One space or one tab | Example:

I will make sure to update the docs here as well, as that also needs the new NBSP and NNBSP added for PHP 8.2.x.

derickr added a commit that referenced this issue Aug 3, 2023
@damianwadley
Copy link
Member

But to be precise, this is not a bug.

If the exact number of whitespace is required then that makes it much, much harder to parse formats where day numbers are two space-padded characters: Aug••2 could be parsed with F••d but not F•d, while Aug•12 could be parsed with F•d but not F••d.

...unless spaces are optional, which seems to be the case now? https://3v4l.org/IcBMe#v8.2.9
Then someone can throw in a whole bunch of spaces when the exact number is ever in question; the pattern is ugly but at least it works.

That aside, is there an actual problem with having a space represent one or more whitespace characters, as the behavior has been for the last 14 years?

@derickr
Copy link
Member

derickr commented Aug 3, 2023

No, there is no problem with that at all, that's why we're keeping the behaviour, even though it wasn't originally intended. I've merged the fix now, and will now be updating documentation.

@derickr derickr closed this as completed Aug 3, 2023
@thg2k
Copy link
Contributor

thg2k commented Aug 3, 2023

This is very interesting, I never realized space actually matches one or more spaces. In theory, the correct fix would be to have one space match exactly one space (or tab) and have "j" match any of these: 1 01 11 •1. But there is no reason to cause useless BC breaks.

One could argue that this would be the correct behavior so you could match something like 2023-08-•2 with Y-m-j, but that's quite useless.

@GrahamCampbell
Copy link
Contributor

Is it too late to get this into 8.2.9? I noticed the docs were updated referencing a change in 8.2.9, but that change is maybe only in 8.2.10?

@saundefined
Copy link
Member

Is it too late to get this into 8.2.9?

PHP 8.2.9 tarballs rebuilt including that fix.

@levu42
Copy link

levu42 commented Aug 3, 2023

I think by the way, the real source of the problem is somewhere different: this code (parsing the datetime) is run for re-formatting the output of php -S, which prints - for whatever reason - the date as Thu Aug 3 13:24:43 2023 with that double space.

Then Laravel tries parsing that with the format string D M d H:i:s Y .

So I guess either the php -S output is wrong, or, if it's intended, there might be a different format string for Laravel to use that fits the one used by php -S when outputting the date.

Either way, it's great that the old behavior is restored to avoid unnecessary BC breaks. But out of curiosity, does anyone have some insight whether my thoughts go into the right direction?

@achmadya-dev
Copy link

achmadya-dev commented Aug 6, 2023

sudo downgrade php this will downgrade your php version and select your last version before update

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants