search

alcohol / iso4217

ISO 4217 PHP Library
MIT License
142 stars 20 forks source link

PSR-4 Compliance Issue #25

Closed awadood closed 3 years ago

awadood commented 3 years ago

The users with composer 2 are unable to use this package because of PSR-4 incompatibility. The following two classes from payum/iso4217 do not comply with PSR-4.

Payum\ISO4217\Tests\CurrencyTest
Payum\ISO4217\Tests\ISO4217Test

Solution:

Move both classes to a new directory "Tests" inside iso4217.
alcohol commented 3 years ago

I am confused, those classes do not exist within this repository.

pierredup commented 3 years ago

@alcohol The original issue came from Payum (https://github.com/Payum/Payum/issues/878). This repo was forked to the payum org with the namespaces changed, hence the class names.

If I'm not mistaken, this issue exists in this repo as well, which would be the first place to add a fix and then the changes can be synced to the payum fork.

awadood commented 3 years ago

@pierredup I just checked all the files in this repo. I believe @alcohol is right and there is no PSR-4 compliance issue in this repo. I think that the changes will be in payum fork only. @pierredup what do you think?

pierredup commented 3 years ago

@awadood I just double-checked the files, and that is correct, this repo doesn't have the same namespace issue, so the issue is purely on the payum fork side.

I'm not 100% sure why the fork was necessary, I'll look into it, and maybe we don't need it anymore and can revert to using this package.

@awadood sorry for the back-and-forth, this can be addressed in the payum fork

@alcohol sorry for the noise, this issue can be closed

makasim commented 3 years ago

I'm not 100% sure why the fork was necessary, I'll look into it, and maybe we don't need it anymore and can revert to using this package.

@pierredup https://github.com/alcohol/iso4217/pull/1, https://github.com/alcohol/iso4217/pull/2

pierredup commented 3 years ago

Thanks for the pointers @makasim, I'll go through the changes in the linked PRs in more detail and compare with the current state of this package

awadood commented 3 years ago

@pierredup thanks! so should we close this issue and reopen https://github.com/Payum/Payum/issues/878 ?

pierredup commented 3 years ago

@awadood can you open the same issue on the https://github.com/Payum/iso4217 repo? As that is ultimately where the errors comes from and where it needs to be addressed

awadood commented 3 years ago

the issue has been created with the repo it belongs to. the link is https://github.com/Payum/iso4217/issues/12 I apologize for the noise and i am now closing this issue here.