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

Update date exception thrown #1646

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

VincentLanglet
Copy link
Contributor

@VincentLanglet
Copy link
Contributor Author

I don't think failure are related @isfedorov

@isfedorov
Copy link
Member

@VincentLanglet Yes, the failure itself isn't related. Docker image just couldn't build. But still it's not possible to merge the changes.
Screenshot 2024-05-29 at 16 28 51
Could you please rebase your fork and force push changes after that?

@VincentLanglet
Copy link
Contributor Author

@VincentLanglet Yes, the failure itself isn't related. Docker image just couldn't build. But still it's not possible to merge the changes. Screenshot 2024-05-29 at 16 28 51 Could you please rebase your fork and force push changes after that?

Is it better now ?

I'm getting "Everything is up to date", and if I look at my branch https://github.com/VincentLanglet/phpstorm-stubs/tree/date-exception, there is only one extra commit over master
image

@isfedorov
Copy link
Member

@VincentLanglet Still the same problem. Don't know what can it be, but nevermind, I'll merge it manually, but before that, could you please clarify why you have changed exception to DateException but not DateInvalidTimeZoneException? According to RFC Errors with timezone creation cause an DateInvalidTimeZoneException.

@VincentLanglet
Copy link
Contributor Author

VincentLanglet commented May 29, 2024

@VincentLanglet Still the same problem. Don't know what can it be, but nevermind, I'll merge it manually, but before that, could you please clarify why you have changed exception to DateException but not DateInvalidTimeZoneException? According to RFC Errors with timezone creation cause an DateInvalidTimeZoneException.

There are 4 new exceptions which extends DateException:

  • DateInvalidTimeZoneException
  • DateInvalidOperationException
  • DateMalformedStringException
  • DateMalformedIntervalStringException

Since I wasn't sure about when they are thrown I wanted to update the stubs in a way I was sure it was correct.

But I re-read the RFC, looked at the implementation and tried on PHP and I would say (cf php/php-src@66a1a91)

I updated the PR.

I discovered there is also some other behavior changes and I didn't know how to update the stubs correctly in order to not give a wrong stub for PHP < 8.3 users.

I tried LanguageLevelTypeAware in 0df0010, is it the right way @isfedorov ?

@VincentLanglet
Copy link
Contributor Author

Sorry for the delay @isfedorov ; I fixed the build

@isfedorov
Copy link
Member

@VincentLanglet Thank you! The approach with LanguageLevelTypeAware looks correct in this case but I'd remove return types from method signatures in this case, since return types are already declared by attributes. Could you please update signatures?

@VincentLanglet
Copy link
Contributor Author

VincentLanglet commented Jun 12, 2024

@VincentLanglet Thank you! The approach with LanguageLevelTypeAware looks correct in this case but I'd remove return types from method signatures in this case, since return types are already declared by attributes. Could you please update signatures?

As shown by the CI https://github.com/JetBrains/phpstorm-stubs/actions/runs/9487379401/job/26143867750?pr=1646 @isfedorov I need to keep the method signature to have a green CI. (The previous commit 3a01281 had green tests)

@isfedorov
Copy link
Member

@VincentLanglet Actually the failure says that return type of methods modify should contain false in PHP 8.3. And we can check that it's true using simple example https://3v4l.org/IZXbp

@VincentLanglet
Copy link
Contributor Author

VincentLanglet commented Jun 13, 2024

@VincentLanglet Actually the failure says that return type of methods modify should contain false in PHP 8.3. And we can check that it's true using simple example 3v4l.org/IZXbp

I see, I'll try to understand if false is still a possible return type.
I'll send a mail to the PHP internals, to see if it need to be fixed on PHP side or if FALSE is still possible.

@VincentLanglet
Copy link
Contributor Author

@VincentLanglet Actually the failure says that return type of methods modify should contain false in PHP 8.3. And we can check that it's true using simple example 3v4l.org/IZXbp

The issue is on the PHP side.
Reported and validated here php/php-src#10366 (comment)
The PR is in progress php/php-src#14600

I assume we have to wait for the merge and a new PHP 8.3 release in order to have green tests ?

@VincentLanglet
Copy link
Contributor Author

@isfedorov I got an anwser.

The return type was buggy in PHP 8.3, but it's fixed and the fix will be landed in PHP 8.4.
See php/php-src#14600 (comment)

I update the LanguageLevelTypeAware attribute then

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