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

[12.x] Improve typehints for Http classes #54783

Open
wants to merge 3 commits into
base: 12.x
Choose a base branch
from

Conversation

cosmastech
Copy link
Contributor

I didn't quite get everything here, but I made some updates to improve typehints and static analysis.

@cosmastech cosmastech marked this pull request as draft February 25, 2025 02:14
@timacdonald
Copy link
Member

I don't think we should be introducing phpstan specific types or more generally try to optimize for PHPStan or other singular tool. Optimizing for a narrow scope like that will lead to detrimental impacts on the wider developer experience outside of phpstan analyze.

We should be testing more exotic changes like these changes against PHPStorm, VSCode, Intelephense language server, Phpactor language server, Psalm, etc.

@cosmastech
Copy link
Contributor Author

I don't think we should be introducing phpstan specific types or more generally try to optimize for PHPStan or other singular tool. Optimizing for a narrow scope like that will lead to detrimental impacts on the wider developer experience outside of phpstan analyze.

We should be testing more exotic changes like these changes against PHPStorm, VSCode, Intelephense language server, Phpactor language server, Psalm, etc.

I chose this because it feels a lot cleaner than just duplicating the array shapes/closure params again and again. Also, the relationship of the parent method to child methods means that it's very easy for someone to update one but not the others, which is eliminated by this local type alias.

But I can see your point. I believe there's already been at least one PR merged that has them (I was the offender on that one as well). Will be interesting to see if there are any issues that arise from that.

I can assure you that they work fine in the latest version of PHPStorm. As for the others, which I do not use:

Is there any kind of consensus on what PHPDoc declarations are broadly compatible? If there were somewhere that defined the standards for Laravel typehints, then I think contributors could improve the codebase with confidence. I hope that there's a path forward, because static analysis is a boon to our codebase. Having vendor code without precise types slows down development and introduces bugs that could've been caught.

@cosmastech cosmastech marked this pull request as ready for review February 26, 2025 02:29
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.

2 participants