Skip to content

Fix PHP 8 compatibility in 2.x #882

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

Merged
merged 2 commits into from
Nov 29, 2020
Merged

Fix PHP 8 compatibility in 2.x #882

merged 2 commits into from
Nov 29, 2020

Conversation

Tobion
Copy link
Contributor

@Tobion Tobion commented Nov 25, 2020

My library fails with php 8 using swagger 2.x, see Tobion/OpenAPI-Symfony-Routing#7
This is due to the namespace not being parsed correctly.

This PR fixes PHP 8 compatibility.
It would be very hard to test with php 5.6 until php 8 due to phpunit bc breaks and incompatibilities, e.g. phpunit <9 will not support php 8. so just droppping unsupported php versions is the best choice which was also done in 3.x (master).

This backports #830, #837, 29e913a, parts of 3f1cee9 to fix php 8 errors and tests.

it would be very hard to test with php 5.6 until php 8 due to phpunit bc breaks and incompatibilities. so just droppping unsupported php versions is the best choice which was also done in master 3.x (master).
This backports zircote#830, zircote#837,
zircote@29e913a, parts of zircote@3f1cee9 to fix php 8 errors and tests.
@Tobion Tobion changed the title test 2.x with php 8 Fix PHP 8 compatibility in 2.x Nov 26, 2020
@Tobion
Copy link
Contributor Author

Tobion commented Nov 26, 2020

@DerManoMann tests pass. I would appreciate if you tag this as 2.1 release.

Copy link
Collaborator

@DerManoMann DerManoMann left a comment

Choose a reason for hiding this comment

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

Nice work - thanks.

},
"autoload-dev": {
"psr-4": {
"SwaggerTests\\": "tests/"
"SwaggerTests\\": "tests/",
"AnotherNamespace\\": "tests/Fixtures/AnotherNamespace"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Noit sure if this is really needed seeing that the fixtures do not properly use PSR-4 namespaces.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The fixtures use psr-4 namespace autoloading corrctly. Just with strange namespaces (\SwaggerTests, \AnotherNamespace). So I think this is needed to load the annotation examples.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm, I would argue that

namespace SwaggerFixtures;

is not a correct namespace according to PSR-0/PSR-4 as it does not match the folder name at all.

Nevertheless, that's not really important for the fixtures so all is good.

@DerManoMann DerManoMann merged commit 5b26395 into zircote:2.x Nov 29, 2020
@DerManoMann
Copy link
Collaborator

@bfanger @zircote I agree with @Tobion that a new tag/release for this would be nice.... As suggested, 2.1.0 seems like the right thing

@Tobion Tobion deleted the patch-1 branch November 29, 2020 23:10
@Tobion
Copy link
Contributor Author

Tobion commented Dec 1, 2020

@DerManoMann can't you create the tag?

@DerManoMann
Copy link
Collaborator

I'll double check - looks like I can... I always mix up GH and packagist permissions :/

@Tobion
Copy link
Contributor Author

Tobion commented Dec 2, 2020

@DerManoMann thanks for the tag. Unfortunately the release is not visible to packagist: https://packagist.org/packages/zircote/swagger-php. Can you trigger a refresh?
And also enable auto-update: https://packagist.org/about#how-to-update-packages

@DerManoMann
Copy link
Collaborator

damn, got distracted! autoupdate is definitely something I cannot configure

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