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

DateTimeImmutable::modify() can no longer return false since PHP 8.3 #3094

Closed
wants to merge 1 commit into from

Conversation

JanTvrdik
Copy link
Contributor

@JanTvrdik JanTvrdik commented May 25, 2024

Since PHP 8.3, DateTimeImmutable::modify() can no longer return false, and throws DateMalformedStringException instead.

Relevant RFC: https://wiki.php.net/rfc/datetime-exceptions

Relevant php-src commit: php/php-src@b7860cd

Relevant code change:

obrazek

@JanTvrdik JanTvrdik force-pushed the patch-1 branch 3 times, most recently from 5867da6 to bfc036f Compare May 25, 2024 11:06
@ondrejmirtes
Copy link
Member

Some tests failed, otherwise 👍

Btw what about @throws for the method?

@JanTvrdik
Copy link
Contributor Author

Btw what about @throws for the method?

It throws DateMalformedStringException. But you can't put @throws into function map, can you?

@ondrejmirtes
Copy link
Member

There would have to be conditionally loaded stub file for 8.4+.

@janedbal
Copy link
Contributor

Can we fix @throws in next step? I believe this already improves things.

@ondrejmirtes
Copy link
Member

I feel like fixing the return type and adding @throws is an atomic change. We know it should be done, let's do it while we have the context.

@JanTvrdik
Copy link
Contributor Author

Looks to be fixed as part of #3209.

@JanTvrdik JanTvrdik closed this Jul 17, 2024
@ondrejmirtes
Copy link
Member

I totally forgot about this PR, sorry! I was forced to fix this as part of update to jetbrains/phpstorm-stubs about exception-throwing on 8.3.

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

Successfully merging this pull request may close these issues.

3 participants