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

Fix Doctrine\Common\Persistence deprecations #32

Closed
wants to merge 4 commits into from

Conversation

ndench
Copy link
Contributor

@ndench ndench commented Dec 20, 2019

Just a basic find and replace of Doctrine\Common\Persistence with Doctrine\Persistence.

There are two failing tests but they are unrelated and were failing before.

@ndench
Copy link
Contributor Author

ndench commented Dec 30, 2019

Oops, messed up my rebase onto master and it automatically closed the PR.

@weirdan
Copy link
Member

weirdan commented Jan 4, 2020

This can't be merged as is because it makes Psalm think there are Doctrine\Persistence\* classes when the plugin is used with older Doctrine versions:

$ composer update --prefer-lowest --prefer-stable
.......
$ rg 'Doctrine\\Persistence' vendor # no results
$

The options I see:

  • bump the dependency to the version that introduced Doctrine\Persistence (doctrine/common:^2.9 as far as I can tell). Pro: easiest to implement; Contra: narrows the supported version range
  • do a runtime check for the version and load different stubs depending on the doctrine version used. Pro: still fairly easy to implement (Plugin::getPackageVersion()), works with all supported versions; Contra: harder to test (both variants would have to be tested).

Regardless of the approach chosen we also need a way to test that stubs are actually overriding existing classes (vimeo/psalm#1252 would be very handy here).

@orklah
Copy link
Collaborator

orklah commented Jan 4, 2020

There might be a third option. You could release a new major version which will support Doctrine from 2.9.
Pros:

  • Easy to implement
  • Keep the supported version range
  • Easy way to handle future structural changes in Doctrine (Which will probably happen when Doctrine 3 is out)

Cons:

  • Harder to maintain the package as every new functionnality(in psalm for example) will have to be backported

@ndench
Copy link
Contributor Author

ndench commented Jan 6, 2020

I think @orklah is the accepted way to do this sort of thing in most open source projects yeah? Though I think it would be a pain to maintain two versions going forward for something as basic as a namespace change. If there were drastic differences for Doctrine 2.9 it might be the way to go. Maybe we'll need to do this for Doctrine 3.0?

I think I prefer the runtime check though @weirdan, surely there's a nice way we can run all tests using Doctrine\Common\Persistence and Doctrine\Persistence?

@orklah
Copy link
Collaborator

orklah commented May 26, 2020

Now that doctrine/persistence 2.0 and doctrine/orm 3.0 are released, maybe we could reconsider this?

@weirdan could you let us know what solution you might accept or not regarding this issue?

@weirdan
Copy link
Member

weirdan commented May 26, 2020

With upstream major bump adding new branch to support that seems to be the best option to me.

@orklah
Copy link
Collaborator

orklah commented May 26, 2020

@weirdan does that means that this PR could be merged on master and released as a new major?

@enumag
Copy link
Contributor

enumag commented Aug 25, 2020

Just ran into this as well. Can we have a new version with a fix?

@ddebin
Copy link
Contributor

ddebin commented Oct 14, 2020

@ndench You missed a Doctrine\Common\Persistence rename in bundle-stubs/ServiceEntityRepository.php.

@ndench
Copy link
Contributor Author

ndench commented Oct 20, 2020

Thanks @ddebin I'll add that in.

@enumag
Copy link
Contributor

enumag commented Oct 30, 2020

Could you rebase this? There is most likely going to be a new major version with Psalm 4 support anyway.

@enumag enumag mentioned this pull request Nov 5, 2020
@ndench ndench force-pushed the doctrine-common-deprecations branch from 1df9fc1 to 2dd8e17 Compare December 5, 2020 01:57
@ndench
Copy link
Contributor Author

ndench commented Dec 5, 2020

Sorry for the radio silence. I've rebased this branch onto master.

@weirdan weirdan force-pushed the doctrine-common-deprecations branch from 2dd8e17 to e26fa32 Compare January 3, 2021 21:28
@weirdan weirdan changed the base branch from master to 1.x January 3, 2021 21:30
@VincentLanglet
Copy link
Contributor

  • do a runtime check for the version and load different stubs depending on the doctrine version used. Pro: still fairly easy to implement (Plugin::getPackageVersion()), works with all supported versions; Contra: harder to test (both variants would have to be tested).

I think it's the best solution:

  • it keeps the wild range
  • it doesn't require to support multiple branches of this branch. Or you'll end with lot of branches like psalm-3, psalm-3-doctrine-2.9, psalm-4, psalm-4-doctrine-2.9, ...

This is already done in the phpunit plugin https://github.com/psalm/psalm-plugin-phpunit/blob/master/src/Plugin.php#L15-L17

@weirdan
Copy link
Member

weirdan commented Jan 4, 2021

There will be no Psalm 3 branches.
So what's left to do here is to bump the doctrine/persistence dependency to the version that introduced new names.

@iluuu1994
Copy link

The new names were introduced in doctrine/persistence@548f75f (version 1.3.0). So this could be bumped to ^1.3|^2.0 or possibly even just to ^2.0.

@orklah orklah closed this in #96 Oct 31, 2021
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.

7 participants