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

Fix support of Enum stubs from PHPStorm #1445

Merged
merged 2 commits into from
Sep 4, 2024

Conversation

VincentLanglet
Copy link
Contributor

Hi @Ocramius,

An (old) assert was added by @kukulich in 933dcb9

This one is not true anymore, cause PHPStorm stubs use @since on Enum like
JetBrains/phpstorm-stubs@2630c46#diff-11cfa64a565c311363b652c7494df2be636c9934afac14510bdd70e50d7044ceR6

Such error was discovered by the PHPStan CI which run on PHP 8.4 https://github.com/phpstan/phpstan-src/actions/runs/10680283358/job/29601421334?pr=3393 ; but I'm unsure how much work it would require so far to have a full Ci running on PHP 8.4. I can try in another PR if you want to.

@Ocramius
Copy link
Member

Ocramius commented Sep 3, 2024

@VincentLanglet are the stubs in here already bumped here? We bump them regularly, but it needs adjustments in composer.json too

@Ocramius Ocramius added enhancement dependencies Pull requests that update a dependency file labels Sep 3, 2024
@VincentLanglet
Copy link
Contributor Author

@VincentLanglet are the stubs in here already bumped here? We bump them regularly, but it needs adjustments in composer.json too

No, those stubs are on master branch and but no tags are created yes.
JetBrains/phpstorm-stubs@v2024.2...master

Should I target the latest commit ?

@Ocramius
Copy link
Member

Ocramius commented Sep 3, 2024

Should I target the latest commit ?

For trying them out: yes.

For merging this (once fixed): needs to be a tagged version.

@VincentLanglet
Copy link
Contributor Author

VincentLanglet commented Sep 4, 2024

Should I target the latest commit ?

For trying them out: yes.

Done. I also tried to add a PHP 8.4 Ci (for testing purpose), because the error occured only on PHP 8.4 PHPStan ci. (cf https://github.com/phpstan/phpstan-src/actions/runs/10680283358/job/29601421334?pr=3393)

(Not sure if there is an easy way to avoid waiting for workflow approval at every commit so I try to offer an easy-merge PR #1446)

@kukulich
Copy link
Collaborator

kukulich commented Sep 4, 2024

@VincentLanglet You can remove the PHP 8.4 changes and use random extension for unit tests.

composer.json Outdated Show resolved Hide resolved
@VincentLanglet VincentLanglet changed the title Fix support of PHP 8.4 stubs from PHPStorm Fix support of Enum stubs from PHPStorm Sep 4, 2024
@VincentLanglet
Copy link
Contributor Author

I reworked the PR @Ocramius,

Since I dunno when a new tag of PHPStorm stubs will be released,

  • I added a test with the enum case of random extension => tests are failing
  • I added the fix in a new commit => tests are green

This way it should be merge-able now, without waiting for a PHPStorm stub new tag.
(and https://github.com/ondrejmirtes/BetterReflection will be able to rebase the fix too)

Also, I discovered you'll need a fix for SplObjectStorage in PHP 8.4 so I opened another PR #1448.

Copy link
Member

@Ocramius Ocramius left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @VincentLanglet!

@Ocramius Ocramius self-assigned this Sep 4, 2024
@Ocramius Ocramius merged commit 31b5dd1 into Roave:6.42.x Sep 4, 2024
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants