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

Bump PHP dep from 7.2 to 7.4 [v5] #1540

Open
DerManoMann opened this issue Jan 29, 2024 · 8 comments · May be fixed by #1549
Open

Bump PHP dep from 7.2 to 7.4 [v5] #1540

DerManoMann opened this issue Jan 29, 2024 · 8 comments · May be fixed by #1549

Comments

@DerManoMann
Copy link
Collaborator

DerManoMann commented Jan 29, 2024

Time to move on!?

Useful features:

  • property typehints
  • arrow functions

Other things to consider:

  • bump symfony/finder to ^5.0
  • bump symfony/yaml to ^5.0
  • bump friendsofphp/php-cs-fixer (dev/suggest) to ^3.48
  • bump doctrine/annotations (dev/suggest) to ^2.0
  • bump phpunit/phpunit (dev) to ^9.0

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 using nikic/php-parser

Plan is to move forward to v5 reasonably soon after 8.4 is out.

@DjordyKoert
Copy link
Contributor

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)

@DerManoMann
Copy link
Collaborator Author

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.
But then, its a step in the right direction and the issue was created for this type of discussion :)

@DjordyKoert
Copy link
Contributor

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 @varcomment with a bunch of different possible types.

* @var MediaType|JsonContent|XmlContent|Attachable|array<MediaType|JsonContent|XmlContent|Attachable>

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 :)).

private function validateType(string $type, $value): bool

This also comes with a con though at this current time, because of the Generator::UNDEFINED constant which is currently used everywhere as a default. This would require every property to also be typed with the string type which could cause confusion for users of this package who might think any kind of string can be used as a value for that property.

This could be fixed by either implementing a class (new Undefined()) or some kind of Type enum (which could maybe even include normal data types https://swagger.io/docs/specification/data-models/data-types/#any?) which replaces this.

Constructor property promotion

This 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)

Attributes

Annotations could of course also be fully dropped because PHP 8 now includes Attributes which can fully replace these. Additionally it looks like doctrine/annotations will be flagged as abandoned some time in the future. If/when this happens then this package can simply remove the dev dependency, suggestion & internal code which mention this package without having to bump the php version (and presumably a new major version) again.

These are just some of my thoughts about it hahaha 😄

@DerManoMann
Copy link
Collaborator Author

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 🤷

@DerManoMann DerManoMann linked a pull request Feb 9, 2024 that will close this issue
@afilina
Copy link

afilina commented Feb 29, 2024

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.

@DerManoMann
Copy link
Collaborator Author

Not at all. I agree that there are some good points in general but each project is different.
I still believe for swagger-php a slower approach is more appropriate, although I am not looking forward to dealing with updating two branches, at least for a bit.

@DerManoMann DerManoMann changed the title Bump PHP dep from 7.2 to 7.4 Bump PHP dep from 7.2 to 7.4 [v5] Mar 10, 2024
@fulldecent
Copy link
Contributor

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

@awanganddong
Copy link

My php version is 7.4 and following are the supported versions.

    "zircote/swagger-php": "^3.0",
    "doctrine/annotations": "^1.10"

Through testing, the v4 version is not supported.
It took me 8 hours to debug this problem. It is convenient for those who need it to see it.

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 a pull request may close this issue.

5 participants