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

Support for range integers + drop support for PHP 7.2 & 7.3 #2236

Merged
merged 18 commits into from
Mar 14, 2024

Conversation

bnowak
Copy link
Contributor

@bnowak bnowak commented Mar 6, 2024

@bnowak
Copy link
Contributor Author

bnowak commented Mar 8, 2024

@DjordyKoert That feature would work on any versions except the lowest ones (the single CI job with --prefer-lowest flag). I've checked that it works from the very lowest dependencies (comparing with the lowest installed by default):

SYMFONY_REQUIRE=5.4.* composer update \
phpdocumentor/reflection-docblock=4.3.4 \
symfony/property-info=5.4.10 \
phpstan/phpdoc-parser=1.13.0 \ # dependency of phpdocumentor/type-resolver=1.8.2 so not needed to specify in composer
phpdocumentor/type-resolver=1.8.2

but phpdocumentor/type-resolver=1.8.2 requires PHP 7.3 at least so my question is: could we bump dependencies and php to that version as minimum?

I know bumping PHP to next major version could be to too radical, so my alternative would be bumping only phpdocumentor/reflection-docblock & symfony/property-info versions (root dependencies) and make the feature (tests actually) PHP & phpdocumentor/type-resolver versions specific (that it will work from these the lowest versions).

Would do you think?

@DjordyKoert
Copy link
Collaborator

I wouldn't even mind bumping minimum php version to 7.4. Both 7.2 & 7.3 barely get any installs anymore see packagist stats.

@bnowak
Copy link
Contributor Author

bnowak commented Mar 12, 2024

Cool 👍

Would you mean just only bumping it up in composer or also any other related adjustments in code (like getting rid of some backward-compatible workarounds etc.)?

@DjordyKoert
Copy link
Collaborator

Would you mean just only bumping it up in composer or also any other related adjustments in code (like getting rid of some backward-compatible workarounds etc.)?

I don't think we have any real backward-compatible workarounds, only in a few tests I think it looks like.

Feel free to remove this, otherwise I will remove them myself 😃

@bnowak
Copy link
Contributor Author

bnowak commented Mar 12, 2024

👍 thanks for replying

I'm going to handle that tomorrow 😉

@DjordyKoert
Copy link
Collaborator

@DjordyKoert That feature would work on any versions except the lowest ones (the single CI job with --prefer-lowest flag). I've checked that it works from the very lowest dependencies (comparing with the lowest installed by default):

SYMFONY_REQUIRE=5.4.* composer update \
phpdocumentor/reflection-docblock=4.3.4 \
symfony/property-info=5.4.10 \
phpstan/phpdoc-parser=1.13.0 \ # dependency of phpdocumentor/type-resolver=1.8.2 so not needed to specify in composer
phpdocumentor/type-resolver=1.8.2

but phpdocumentor/type-resolver=1.8.2 required PHP 7.3 at least so my question is: could we bump dependencies and php to that version as minimum?

I think we should also bump symfony/property-info to ^5.4.10|^6.0|^7.0, that should ensure that the test ran on --prefer-lowest will pass

@bnowak
Copy link
Contributor Author

bnowak commented Mar 14, 2024

@DjordyKoert in general PR is ready for review. Finally, I decided to bump only to PHP 7.3 (plus needed composer dependencies) which is enough for this feature.

However, I noticed failed pipelines (on SF >= 6.3) which are not related to my changes. It seems that the change which broke them was introduced in https://github.com/nelmio/NelmioApiDocBundle/actions/runs/8231364006/job/22506509816 but I'm not sure how it should be fixed.

@DjordyKoert
Copy link
Collaborator

However, I noticed failed pipelines (on SF >= 6.3) which are not related to my changes. It seems that the change which broke them was introduced in nelmio/NelmioApiDocBundle/actions/runs/8231364006/job/22506509816 but I'm not sure how it should be fixed.

I already have an issue open at swagger-php about this :) zircote/swagger-php#1556

@DjordyKoert
Copy link
Collaborator

@DjordyKoert in general PR is ready for review. Finally, I decided to bump only to PHP 7.3 (plus needed composer dependencies) which is enough for this feature.

LGTM, I would still like to drop support for PHP 7.3 in this and make PHP 7.4 the minimum supported version would you like & be able to do this? :)

Additionally an update inside the changelog.md would be appreciated.

@bnowak
Copy link
Contributor Author

bnowak commented Mar 14, 2024

👍 will handle that

@bnowak bnowak changed the title Support for range integers Support for range integers + drop support for PHP 7.2 & 7.3 Mar 14, 2024
@bnowak
Copy link
Contributor Author

bnowak commented Mar 14, 2024

@DjordyKoert adjusted to PHP 7.4 as minimum version, but I'm struggling with 2 errors in BazingaFunctionalTest (https://github.com/nelmio/NelmioApiDocBundle/actions/runs/8279847555/job/22655096369?pr=2236) which I cannot reproduce locally in docker (using PHP 7.4 image). It occurs only in --prefer-lowest job. I double checked that have all composer installed dependencies exactly the same as in CI job, but different results.

Do you have any idea what else I could check?

@DjordyKoert
Copy link
Collaborator

DjordyKoert commented Mar 14, 2024

Do you have any idea what else I could check?

It looks like there is a caching issue. On my local machine the first test (testModelComplexDocumentationBazinga) passes whenever no cache is created yet. The tests after fail (because they use the cache created in testModelComplexDocumentationBazinga).

Rerunning the full BazingaFunctionalTest without deleting that /var directory (aka without removing cache) also causes testModelComplexDocumentationBazinga to fail.....

@DjordyKoert
Copy link
Collaborator

@bnowak 514239c fixed it 🥳

@bnowak
Copy link
Contributor Author

bnowak commented Mar 14, 2024

cool, thanks for help with that :)

CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Show resolved Hide resolved
tests/Functional/FunctionalTest.php Show resolved Hide resolved
Co-authored-by: Djordy Koert <[email protected]>
@bnowak
Copy link
Contributor Author

bnowak commented Mar 14, 2024

@DjordyKoert thanks for approving ❤️

will zircote/swagger-php#1556 be a blocker for merging it or not?

@DjordyKoert
Copy link
Collaborator

will #2236 (comment) be a blocker for merging it or not?

Nope! 😄

@bnowak
Copy link
Contributor Author

bnowak commented Mar 14, 2024

sorry, I meant swagger-php issue (comment updated) 😅

@DjordyKoert
Copy link
Collaborator

sorry, I meant swagger-php issue (comment updated) 😅

Still a nope hahaha. That is an issue on its own.

@DjordyKoert DjordyKoert merged commit bbfe64a into nelmio:master Mar 14, 2024
7 of 11 checks passed
@DjordyKoert
Copy link
Collaborator

Thanks a lot @bnowak for this new feature + the other things that you were willing to pick up ❤️

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