doctrine / DoctrineModule

Doctrine Module for Laminas
http://www.doctrine-project.org/projects/doctrine-module.html
MIT License
398 stars 270 forks source link

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

Closed demiankatz closed 1 year ago

demiankatz commented 1 year ago

This PR is work in progress on addressing #800.

TODO

greg0ire commented 1 year ago

Regarding the cache, there is already an issue about this it seems: https://github.com/doctrine/DoctrineModule/issues/786

demiankatz commented 1 year ago

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

demiankatz commented 1 year ago

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.

demiankatz commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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

demiankatz commented 1 year ago

Thanks, @greg0ire!

demiankatz commented 1 year ago

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 commented 1 year ago

The order of PRs Dennis and I are shooting for is

799

801

796

@driehle Are you back and ready for #799?

demiankatz commented 1 year ago

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 commented 1 year ago

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.