laravel-cashier-mollie icon indicating copy to clipboard operation
laravel-cashier-mollie copied to clipboard

Published 20 hours ago • verified publisher icon mollie

Remove money helper from code to stop collision

Open siegerhansma opened this issue 2 years ago • 8 comments

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.

siegerhansma avatar Aug 18 '22 08:08 siegerhansma

Hi @siegerhansma !

Thanks for this PR. Can you point me to the signature of the helper in the colliding package?

sandervanhooft avatar Aug 18 '22 09:08 sandervanhooft

@sandervanhooft Sure thing, it's over here: https://github.com/akaunting/laravel-money/blob/master/src/helpers.php#L7

siegerhansma avatar Aug 18 '22 09:08 siegerhansma

Thanks! It appears to me the helper itself isn't removed in this PR?

sandervanhooft avatar Aug 18 '22 13:08 sandervanhooft

Correct, it's still used in the tests. Should I replace them in the tests and remove the helper?

siegerhansma avatar Aug 18 '22 13:08 siegerhansma

That would definitely help keeping things consistent :)

sandervanhooft avatar Aug 18 '22 14:08 sandervanhooft

No problem, I'll update the PR later this week.

siegerhansma avatar Aug 22 '22 07:08 siegerhansma

Related: #128 #89

sandervanhooft avatar Sep 01 '22 10:09 sandervanhooft

@sandervanhooft Removed the helper and updated all the tests to use the class instead of the helper. Not sure about the conflict though.

siegerhansma avatar Sep 02 '22 15:09 siegerhansma

Looks good to me! Thanks @siegerhansma !

sandervanhooft avatar Sep 05 '22 10:09 sandervanhooft

@sandervanhooft This was a breaking change in a minor release. Broke our code because of the missing money function

RemiHin avatar Nov 03 '22 12:11 RemiHin

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.

sandervanhooft avatar Nov 03 '22 14:11 sandervanhooft

hi @sandervanhooft We we're able to work around by using the Money and Currency Classes suggested by @siegerhansma, but we might not be the only people using the helper :) thanks for the quick response

RemiHin avatar Nov 03 '22 14:11 RemiHin

Thanks @RemiHin,

Exactly what I was thinking, that's why I am prepping the patch release right now :)

sandervanhooft avatar Nov 03 '22 14:11 sandervanhooft

Here you go: https://github.com/mollie/laravel-cashier-mollie/releases/tag/v2.5.4

sandervanhooft avatar Nov 03 '22 14:11 sandervanhooft