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 for support on different Doctrine ORM versions #414

Merged
merged 5 commits into from Jan 30, 2022
Merged

Fix for support on different Doctrine ORM versions #414

merged 5 commits into from Jan 30, 2022

Conversation

icanhazstring
Copy link

@icanhazstring icanhazstring commented Dec 22, 2021

Add conflict to doctrine/persistence below 1.4 and use correct persistence classes

What is the reason for this PR?

Fix an issue with doctrine/persistnce

Author's checklist

Summary of changes

  • Add a conflict into composer.json for doctrine/persistence below version 1.4
  • Add suggest into composer.json for doctrine/orm
  • Add doctrine/orm as dev requirement to avoid static analysis baseline entries
  • Use new classes of doctrine/orm and doctrine/persistence for Faker\ORM\Doctrine namespace
  • Add class_alias for ClassMetadata and ObjectManager

Review checklist

  • All checks have passed
  • Changes are approved by maintainer

@pimjansen
Copy link

This is a BC which we do not allow for v1. Can we ensure that both are supported here?

@icanhazstring
Copy link
Author

@pimjansen I can try and do the same doctrine did and add a class_alias
So we could then remove it with v2.

@pimjansen
Copy link

@pimjansen I can try and do the same doctrine did and add a class_alias

So we could then remove it with v2.

Correct. For v2 we will remove it from core and can be a community driven extension instead.

@icanhazstring
Copy link
Author

Apparently, psalm is failing in CI but not on my machine using the same PHP version.

vendor/bin/psalm
Scanning files...
Analyzing files...

░
------------------------------
No errors found!
------------------------------

Checks took 0.71 seconds and used 234.707MB of memory
Psalm was unable to infer types in the codebase

@stale
Copy link

stale bot commented Jan 20, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed after 1 week if no further activity occurs. Thank you for your contributions.

@stale stale bot closed this Jan 27, 2022
@icanhazstring icanhazstring reopened this Jan 27, 2022
@stale stale bot removed the lifecycle/stale label Jan 27, 2022
@fdiedler
Copy link

Hope it will be fixed soon, thanks :)

@pimjansen
Copy link

@icanhazstring the pipeline is failing, can you review this?

@icanhazstring
Copy link
Author

Apparently, psalm is failing in CI but not on my machine using the same PHP version.

vendor/bin/psalm
Scanning files...
Analyzing files...

░
------------------------------
No errors found!
------------------------------

Checks took 0.71 seconds and used 234.707MB of memory
Psalm was unable to infer types in the codebase

@pimjansen see this comment.

@pimjansen
Copy link

No idea but ci is leading so needs to be green

psalm.baseline.xml Outdated Show resolved Hide resolved
psalm.baseline.xml Outdated Show resolved Hide resolved
@krsriq
Copy link

krsriq commented Jan 28, 2022

@icanhazstring Running psalm locally is red for me too, works with this psalm.baseline.xml: https://gist.github.com/krsriq/cc1c3ceea2886aa4b93a2b1825a32e7f

@icanhazstring
Copy link
Author

@krsriq then there is something weird with the local setup 🤷‍♂️

Guess I'll just add your baseline then.

@icanhazstring
Copy link
Author

After all these years.
Now it works.

@bram-pkg
Copy link
Member

@icanhazstring is this ready now?

@icanhazstring
Copy link
Author

@bram-pkg checked it again against my problem. Fixed for me 👍
LGTM

@pimjansen pimjansen changed the title Fix #413 Fix for support on different Doctrine ORM versions Jan 29, 2022
Copy link

@pimjansen pimjansen left a comment

Choose a reason for hiding this comment

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

LGTM, also test on 1.3 and 2.x went fine

@pimjansen pimjansen merged commit 067fa82 into FakerPHP:main Jan 30, 2022
@crynobone
Copy link

crynobone commented Feb 2, 2022

This PR caused an issue on projects that don't utilize FakerPHP\ORM\Doctrine

Warning: Class "Doctrine\Persistence\Mapping\ClassMetadata" not found in /Projects/laravel/nova/orion/vendor/fakerphp/faker/src/Faker/ORM/Doctrine/backward-compatibility.php on line 6

Warning: Class "Doctrine\Persistence\ObjectManager" not found in /Projects/laravel/nova/orion/vendor/fakerphp/faker/src/Faker/ORM/Doctrine/backward-compatibility.php on line 10

It seems that FakerPHP expect doctrine/persistent or doctrine/orm to be available for all installation since it always load src/Faker/ORM/Doctrine/backward-compatibility.php, so I don't understand why it was added to require-dev

@icanhazstring
Copy link
Author

It was added to require-dev because it is used for development.

But the other case you are correct. Will create a quick fix for that.

@pimjansen
Copy link

@crynobone fix has been applied in main

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.

Faker/ORM/Doctrine not working with latest doctrine implementations
6 participants