Skip to content

Fix for support on different Doctrine ORM versions#414

Merged
pimjansen merged 5 commits intoFakerPHP:mainfrom
icanhazstring:fix/413
Jan 30, 2022
Merged

Fix for support on different Doctrine ORM versions#414
pimjansen merged 5 commits intoFakerPHP:mainfrom
icanhazstring:fix/413

Conversation

@icanhazstring
Copy link
Copy Markdown

@icanhazstring icanhazstring commented Dec 22, 2021 •
edited
Loading

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
Copy Markdown

pimjansen commented Dec 30, 2021

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

@icanhazstring
Copy link
Copy Markdown
Author

icanhazstring commented Dec 30, 2021

@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
Copy Markdown

pimjansen commented Dec 30, 2021

@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
Copy Markdown
Author

icanhazstring commented Jan 5, 2022

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
Copy Markdown

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
Copy Markdown

fdiedler commented Jan 27, 2022

Hope it will be fixed soon, thanks :)

@pimjansen
Copy link
Copy Markdown

pimjansen commented Jan 27, 2022

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

@icanhazstring
Copy link
Copy Markdown
Author

icanhazstring commented Jan 27, 2022

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
Copy Markdown

pimjansen commented Jan 27, 2022

No idea but ci is leading so needs to be green

krsriq
krsriq reviewed Jan 27, 2022
View reviewed changes
krsriq
krsriq reviewed Jan 27, 2022
View reviewed changes
@krsriq
Copy link
Copy Markdown

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
Copy Markdown
Author

icanhazstring commented Jan 28, 2022

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

Guess I'll just add your baseline then.

@icanhazstring
Copy link
Copy Markdown
Author

icanhazstring commented Jan 28, 2022

After all these years.
Now it works.

@bram-pkg
Copy link
Copy Markdown
Member

bram-pkg commented Jan 28, 2022

@icanhazstring is this ready now?

@icanhazstring
Copy link
Copy Markdown
Author

icanhazstring commented Jan 28, 2022

@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
pimjansen
pimjansen approved these changes Jan 30, 2022
View reviewed changes
Copy link
Copy Markdown

@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
Copy Markdown

crynobone commented Feb 2, 2022 •
edited
Loading

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
Copy Markdown
Author

icanhazstring commented Feb 2, 2022

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.

@icanhazstring icanhazstring deleted the fix/413 branch February 2, 2022 04:18
This was referenced Feb 2, 2022
Closed
Merged
@pimjansen
Copy link
Copy Markdown

pimjansen commented Feb 2, 2022

@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

Reviewers

@pimjansen pimjansen pimjansen approved these changes

@localheinz localheinz Awaiting requested review from localheinz

+1 more reviewer

@krsriq krsriq krsriq left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Faker/ORM/Doctrine not working with latest doctrine implementations

6 participants

@icanhazstring @pimjansen @fdiedler @krsriq @bram-pkg @crynobone