search

TheBigBrainsCompany / TbbcMoneyBundle

This bundle is used to integrate the Money library (https://github.com/mathiasverraes/money) into a Symfony project. This library is based on Fowler's Money pattern (https://verraes.net/2011/04/fowler-money-pattern-in-php/)
MIT License
192 stars 74 forks source link

ORM service is optional #180

Closed mustanggb closed 1 month ago

mustanggb commented 3 months ago

When using pair history ORM is now optional not required.

Removing this should allow MongoDB users to use pair history without having the ORM package installed.

johanwilfer commented 3 months ago

Thanks for the patch @mustanggb !

Did you also test this using Doctrine ORM and that still works?

mustanggb commented 3 months ago

I'd hold off on this for now, I've not had a chance to look into the failures etc.

If anyone else wants to investigate please do.

johanwilfer commented 1 month ago

@mustanggb I see the tests about MongoDB also fails in master, as this contribution came from you could you have a look at this?

mustanggb commented 1 month ago

@johanwilfer Looks like it's a bug introduced by mongodb-odm 2.8.0 (https://github.com/doctrine/mongodb-odm/pull/2630), I believe it should be fixed in https://github.com/doctrine/mongodb-odm/pull/2671.

Do you want to wait for the fix, or we can temporarily workaround by requiring <2.8 (or similar).

johanwilfer commented 1 month ago

I think the temp workaround sounds good, that way we get tests in master to not fail again.

If you want/can do it in this branch it should stop to fail and then we can merge this if that sounds good?

mustanggb commented 1 month ago

Done, unless you want them squashing into one commit.

I know GitLab supports that on merge/pull, not sure if GitHub does.

Alternatively keeping them separate would also for easy revert when the workaround is no longer needed, I'll leave it up to you.

johanwilfer commented 1 month ago

Looks great!

And yeah, Github supports squashing of merged branches into one commit (this is how it is setup in the repro currently): image

On a personal note I perfer individual commits in the feature branch to follow the process, and to squash on merge (to follow the PR).