-
Notifications
You must be signed in to change notification settings - Fork 937
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
Bump PHP dep from 7.2 to 7.4 [v5] #1540
Comments
May I ask why this package will not move to php 8 (or php 8.1 because php 8.0 is already EOL). I got some valid feedback on nelmio/NelmioApiDocBundle#2171 which is why I am suggesting this. I am also currently considering moving NelmioApiDocBundle to php 8.1 nelmio/NelmioApiDocBundle#2171 (comment) |
I think it is just a conservative approach. I do understand some of the reasons for doing it, but without a solid technical reason it still feels a bit artificial. |
Union types (on top of typed property from PHP 7.4)A reason for moving to PHP 8 would be because of the support for union types. Currently a lot of annotations have a swagger-php/src/Annotations/Response.php Line 69 in bdee7f5
Currently you have written your own "type parser" (correct me if I'm wrong), this could instead be handled by PHP itself (except for arrays, php has no way to handle generics :)).
This also comes with a con though at this current time, because of the This could be fixed by either implementing a class ( Constructor property promotionThis is one of my personal favorites simply because it remove a lot of boilerplate code from classes. Maybe this could also help with refactoring the annotations to no longer pass an array of properties and instead use this in combination with named parameters to cleanup the annotations. (This is not for me to decide of course, but I personally was confused about how these annotations worked when I first looked at the codebase) AttributesAnnotations could of course also be fully dropped because PHP 8 now includes Attributes which can fully replace these. Additionally it looks like These are just some of my thoughts about it hahaha 😄 |
Fair points. Considering that a quarter of downloads are still for v3 it feels still a bit early for such a big step. OTOH, making big steps will force others to follow suit. I am not worried about another major version if - I suspect there will be at least one more in the future for an 'attibutes only' version at some point, so 🤷 |
I don't mean to hijack the conversation, but I simply had to thank @DjordyKoert for taking the time to research his arguments so thoroughly. This is food for thought even in my own projects, since I deal with legacy code all the time. |
Not at all. I agree that there are some good points in general but each project is different. |
Bumping deps is a breaking changes. Perhaps we will follow semver and do a deprecation notice now (i.e. in the current major version). And then later at any time we can actually make the new major version and jettison the old parts. Here is a PR for the first step that is actionable now #1651 |
My php version is 7.4 and following are the supported versions.
Through testing, the v4 version is not supported. |
Time to move on!?
Useful features:
Other things to consider:
Bump swagger-php version to 5
Add disclaimer about annotations being deprecated -> doctrine/annotations#486
Remove all code marked deprecated
Add
rector
to manage refactoring and code upgrades.Re-implement
TokenScanner
usingnikic/php-parser
Plan is to move forward to v5 reasonably soon after 8.4 is out.
The text was updated successfully, but these errors were encountered: