-
Notifications
You must be signed in to change notification settings - Fork 795
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
Conversation
I don't think failure are related @isfedorov |
@VincentLanglet Yes, the failure itself isn't related. Docker image just couldn't build. But still it's not possible to merge the changes. |
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 |
@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 |
b7084b0
to
2f16a63
Compare
There are 4 new exceptions which extends DateException:
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 ? |
2f16a63
to
936c45f
Compare
Sorry for the delay @isfedorov ; I fixed the build |
@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) |
@VincentLanglet Actually the failure says that return type of methods |
ff18d25
to
0d0ed85
Compare
I see, I'll try to understand if false is still a possible return type. |
The issue is on the PHP side. I assume we have to wait for the merge and a new PHP 8.3 release in order to have green tests ? |
@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. I update the LanguageLevelTypeAware attribute then |
c6c02d9
to
18308ca
Compare
Hi @isfedorov the PR should be ready. The failing build is unrelated, I opened another PR to fix it #1650 |
@VincentLanglet Thank you for your contribution and for the fix of cs-fixer issue! |
This PR is wrong and I can't update phpstorm-stubs in PHPStan because of it.
|
Hi Since DateMalformedException stub exists in Phpstorm, I'm not sure the PR is creating an issue for Phpstorm users (and it improve php 8.3+ experience). There will always be a need to decide between new and older php versions. I'm not sure a LanguageAware attribut exists for the exception thrown. Could an adjustment be done on phpstan side ? I'll try to take a look after my day off if needed. |
I'm going to fix the problems on PHPStan side. Also: You've forgot to add the exception above |
Thanks, I created #1652 then ; does it require another adjustment on PHPStan side @ondrejmirtes ? |
No, thanks |
Cf https://wiki.php.net/rfc/datetime-exceptions