-
Notifications
You must be signed in to change notification settings - Fork 46
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
Remove money helper from code to stop collision #119
Conversation
Hi @siegerhansma ! Thanks for this PR. Can you point me to the signature of the helper in the colliding package? |
@sandervanhooft Sure thing, it's over here: https://github.com/akaunting/laravel-money/blob/master/src/helpers.php#L7 |
Thanks! It appears to me the helper itself isn't removed in this PR? |
Correct, it's still used in the tests. Should I replace them in the tests and remove the helper? |
That would definitely help keeping things consistent :) |
No problem, I'll update the PR later this week. |
@sandervanhooft Removed the helper and updated all the tests to use the class instead of the helper. Not sure about the conflict though. |
Looks good to me! Thanks @siegerhansma ! |
@sandervanhooft |
Hi @RemiHin , Thanks for reporting and sorry to hear that. I was under the impression it was only used by Cashier internally. But I now see it's also used by the charge builder. Let me revert this and put the money helper back. @siegerhansma this will have to wait until next major release. |
hi @sandervanhooft |
Thanks @RemiHin, Exactly what I was thinking, that's why I am prepping the patch release right now :) |
In one of my projects I installed a package that depended on the akaunting/money package. These both use a money helper. The akaunting/money package overrides the money helper in the laravel-cashier-mollie package in my case.
This pull requests removes the money helper from the client code and directly uses the Money and Currency class. The tests still use the helper, but for users of the package that shouldn't matter.