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

Adjust dependencies/code for doctrine/annotations 2 compatibility #801

Merged
merged 10 commits into from Apr 3, 2023

Conversation

demiankatz
Copy link
Contributor

@demiankatz demiankatz commented Mar 6, 2023

This PR is work in progress on addressing #800.

TODO

  • Resolve cache-related issues
  • Test with doctrine/annotations 1.x after finishing code changes to see if backward-compatibility is maintained, or if we need to fully bump the required version

@greg0ire
Copy link
Member

greg0ire commented Mar 6, 2023

Regarding the cache, there is already an issue about this it seems: #786

@demiankatz
Copy link
Contributor Author

Thanks, @greg0ire -- in fact, it looks like there's work in progress at #796.

@demiankatz
Copy link
Contributor Author

I did some experimentation with merging together this PR and #786, but it still doesn't work -- I've left some more detailed comments on #786 since I think it might make sense to try to get the cache updates done before tackling this part.

@TomHAnderson TomHAnderson changed the base branch from 5.4.x to 6.0.x March 9, 2023 19:00
@demiankatz demiankatz marked this pull request as ready for review March 23, 2023 18:04
@demiankatz
Copy link
Contributor Author

@greg0ire, I found a little more time to work with this further today, and I've gotten this into a state that passes all tests with both annotations v1 and annotations v2. However, when using annotations v2, caching functionality is going to be lost (for now) because the cache objects do not comply with PSR-6. However, I believe this is written in such a way that if we return PSR-6 compliant objects in future, the caching will begin to work -- I'm not sure if #796 can help with that.

Obviously, as noted earlier, I don't have a deep understanding of this module, so it's possible I'm overlooking something important... but hopefully passing tests are a good first step. Please let me know if there's more I can do to help! I've taken this PR out of draft mode so that it can be reviewed.

@demiankatz
Copy link
Contributor Author

In reviewing #800, I was reminded that @andremartinez was encountering phpstan failures. I was able to reproduce the problem and found that it was caused by referencing a non-existent (legacy) Blackhole cache adapter in a test. The easiest solution was to simply skip the test if the newer, expected version of the adapter was missing. I'm not seeing any skipped tests when I run the suite in my environment, so I think this is probably safe for now, especially since things are going to be significantly revised by #796 when that work is completed.

@demiankatz
Copy link
Contributor Author

Apologies, I see that my phpstan fix broke the styles. Everything should be corrected now!

@demiankatz
Copy link
Contributor Author

Thanks, @greg0ire!

Copy link

@andremartinez andremartinez left a comment

Choose a reason for hiding this comment

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

There are warnings for some lines in the Driver Factory, but those doesn't look like breaking changes. Proper unit testing should be added.

Other than that, everything looks good to me

@demiankatz
Copy link
Contributor Author

Thanks, @andremartinez! It may make sense to finish the cache related work before addressing the test coverage warnings. Some lines may not be reachable until that's done, and others may become obsolete. Let me know if I can do more to help, in any case!

@TomHAnderson
Copy link
Member

The order of PRs Dennis and I are shooting for is

#799
#801
#796

@driehle Are you back and ready for #799?

@demiankatz
Copy link
Contributor Author

Thanks, @TomHAnderson, let me know if I can do anything else to help (either in terms of contributing to #799, or in terms of improving test coverage in a new PR after #796 is merged). My time is going to be pretty limited for the next couple of weeks, but if I plan ahead, I can help out a little down the road. :-)

@TomHAnderson
Copy link
Member

In the interest of moving this forward I've deprioritized #799 at this time though I expect to merge it before a release is made.

@TomHAnderson TomHAnderson merged commit 3c86ba9 into doctrine:6.0.x Apr 3, 2023
12 checks passed
@TomHAnderson TomHAnderson mentioned this pull request Apr 3, 2023
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.

None yet

4 participants