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

feat: symfony 7 support #2164

Merged
merged 54 commits into from
Dec 31, 2023
Merged

feat: symfony 7 support #2164

merged 54 commits into from
Dec 31, 2023

Conversation

faizanakram99
Copy link
Contributor

@faizanakram99 faizanakram99 commented Dec 11, 2023

  • makes doctrine/annotations optional
  • updates tests and ci to test with both symfony 7 and < symfony 7 version
  • adds routes-attributes.yaml file for symfony 7 compatible controllers/routes
  • updates some fixtures to use attributes when sf7 kernel is installed
  • skips tests/packages related to packages like jms serializer, bazinga, etc not supported by symfony 7 for symfony 7 matrix

Fixes #2143 and #2147, supersedes #2151 and #2155

#SymfonyHackday

@faizanakram99
Copy link
Contributor Author

Tests are failing with this error, i am not sure why

Multiple @OA\Response() with the same response="200":
  \Nelmio\ApiDocBundle\Tests\Functional\Controller\InvokableController->__invoke() in path/to/NelmioApiDocBundle/Tests/Functional/Controller/InvokableController.php
  \Nelmio\ApiDocBundle\Tests\Functional\Controller\SerializedNameController->serializedNameAction() in path/to/NelmioApiDocBundle/Tests/Functional/Controller/SerializedNameController.php" at DefaultLogger.php line 31

- makes doctrine/annotations optional
- updates tests and ci to test with both symfony 7 and < symfony 7 version
- adds routes-attributes.yaml file for symfony 7 compatible controllers/routes
- skips tests/packages related to packages like jms serializer, bazinga, etc not supported by symfony 7 for symfony 7 matrix
- somehow rebase deletes it again :(
@faizanakram99
Copy link
Contributor Author

faizanakram99 commented Dec 11, 2023

Ok from the looks of it, i don't think it will be possible to support both sf 7 and previous versions, sf 7 conflicts with sensio framework extra and it is probably required for all previous sf versions.

Looks like we need a major version bump and drop all those packages altogether.

nvm

Copy link
Collaborator

@DjordyKoert DjordyKoert left a comment

Choose a reason for hiding this comment

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

Thanks a lot :)

Tests/Functional/TestKernel.php Show resolved Hide resolved
Tests/Functional/TestKernel.php Show resolved Hide resolved
@RobertMe
Copy link

I find it quite rude to use mine (and @DjordyKoert) our changes and commit them under your own name. Especially since at least my PR is waiting for review, and not because I abondended it (/not because didn't incorperate requested changes).

Your could have just merged both branches / pull requests into your own branch so it would be clear that someone else wrote the code, and add your own changes on top of it.

- addresses feedback from review (import OpenApi\Attribtues as OAT)
@faizanakram99
Copy link
Contributor Author

faizanakram99 commented Dec 11, 2023

I find it quite rude to use mine (and @DjordyKoert) our changes and commit them under your own name. Especially since at least my PR is waiting for review, and not because I abondended it (/not because didn't incorperate requested changes).

Your could have just merged both branches / pull requests into your own branch so it would be clear that someone else wrote the code, and add your own changes on top of it.

apologies, i did mention it in a comment that I missed that there was a PR #2151 (comment)

Later I tried to fork your repositories so that I add my changes on top of it but since I had already made all the changes locally (including both making doctrine/annotations optional and adding sf 7 support), i found it hard to separate them and apply on top of your changes. If you wish, I can give you write permissions to this repository (actually you and @DjordyKoert both), so that you may re-add your commits or even ammend existing commits.

It doesn't really matter to me whom the commits are attributed to, I want support for sf7 in this lib because we are using it and it is blocking our sf 7 upgrade and it seemed a good task for symfony hack day. Again apology for any miscommunications @RobertMe

@DjordyKoert
Copy link
Collaborator

I find it quite rude to use mine (and @DjordyKoert) our changes and commit them under your own name. Especially since at least my PR is waiting for review, and not because I abondended it (/not because didn't incorperate requested changes).

Your could have just merged both branches / pull requests into your own branch so it would be clear that someone else wrote the code, and add your own changes on top of it.

I personally have no issue with my 'changes'/commits being lost, since my PR contained the bare minimum (and didn't even work to begin with). I am just very happy @faizanakram99 was able to make it (almost? because CI is still failing) work for Symfony 7. 😄

@DjordyKoert
Copy link
Collaborator

Tests are failing with this error, i am not sure why

Multiple @OA\Response() with the same response="200":
  \Nelmio\ApiDocBundle\Tests\Functional\Controller\InvokableController->__invoke() in path/to/NelmioApiDocBundle/Tests/Functional/Controller/InvokableController.php
  \Nelmio\ApiDocBundle\Tests\Functional\Controller\SerializedNameController->serializedNameAction() in path/to/NelmioApiDocBundle/Tests/Functional/Controller/SerializedNameController.php" at DefaultLogger.php line 31

I can also reproduce this locally. This is a very weird one since it's appending the response from InvokableController to the SerializedNameController /api/serializename path?????

@DjordyKoert
Copy link
Collaborator

@faizanakram99 I have created a PR on your fork repo with changes to make phpunit work for Symfony 7. https://github.com/faizanakram99/NelmioApiDocBundle/pull/1

Older versions are still failing but this should be a good starting point for fixing the others 😄

@faizanakram99
Copy link
Contributor Author

@faizanakram99 I have created a PR on your fork repo with changes to make phpunit work for Symfony 7. faizanakram99#1

Older versions are still failing but this should be a good starting point for fixing the others 😄

Thank you so much @DjordyKoert , I've merged your changes (with rebase)

I think you also need to run cs-fix on changed files, style ci is complaining

@DjordyKoert
Copy link
Collaborator

DjordyKoert commented Dec 19, 2023

@faizanakram99 could you please run a style fix 😄, mine doesn't really fix anything for some reason

@DjordyKoert
Copy link
Collaborator

@faizanakram99 could you please run a style fix 😄, mine doesn't really fix anything for some reason

Nevermind that, I simply applied the diff from styleci https://github.com/faizanakram99/NelmioApiDocBundle/pull/3 😃

@faizanakram99 a merge and styleci should be fixed

@DjordyKoert
Copy link
Collaborator

@GuilhemN any possibility you can look at this? ❤️

@Gemorroj
Copy link
Contributor

@GuilhemN ping

@GuilhemN GuilhemN merged commit 634a16b into nelmio:master Dec 31, 2023
3 of 8 checks passed
@GuilhemN
Copy link
Collaborator

Thank you very much for the huge work!
I will try to look into fixing the CI a bit later.

Happy new year, best wishes!

@DjordyKoert
Copy link
Collaborator

Thank you very much @GuilhemN !

Happy new years & best wishes to you too ❤️

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.

Bundle cannot be used without Doctrine Annotations
7 participants