Skip to content
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

Composer and packagist support #7

Open
wants to merge 16 commits into
base: master
Choose a base branch
from
Open

Conversation

snufkin
Copy link

@snufkin snufkin commented Apr 16, 2017

For various modern PHP frameworks composer support is essential for proper library and dependency management. I'd like to propose that the library adopts this based on this PR.

Changes contained:

  • Restructuring the repository based on the payment library provided by Stripe as a reference
  • Adding a composer file with the proper requirements (TBD - e.g. PHPUnit 6 and PHP 5.6 are incompatible, it's not clear if we need to restrict the PHP version), license
  • Updated the changelog to reflect the composer related updates

Once merged I'd remove the packagist entry for my fork, it was created for testing mainly anyway.

@KristofMorva
Copy link
Contributor

KristofMorva commented Apr 17, 2017

@snufkin why does it require PHPUnit 6 exactly? I don't see a single test anywhere.
Also, the package user is not barion, and there are many other issues.
But I'd support a better composer.json.

@snufkin
Copy link
Author

snufkin commented Apr 17, 2017

Good point, I've removed the phpunit dependency, and also changed the name attribute. I would like to know what those "many other issues" are so I can follow up on them.

@KristofMorva
Copy link
Contributor

KristofMorva commented Apr 17, 2017

@snufkin I have created some PR directly in your fork, but some ideas:

  • The requirement is ext-curl instead of php-mod/curl.
  • The autoload is not working, since it's incorrect. The library itself does not contain namespaces, so Barion namespace does not exist, and PSR-4 cannot be done. Either add the namespaces (that would be the professional way, but I don't know if the Barion guys have time to review and test a change like that, ping @barion-vinczei @attilabotz @emery303). If not, you can use "classmap": [ "library" ], which will load every file.
  • The library type is the default, so you can omit that.
  • About the license are you sure it's Apache, did you talk to the Barion guys? If you are not sure, just omit it, and let them fill it out if they want to.
  • I don't see the goal of the authors with this value.
  • The api keyword is a little too general, I don't think it's useful.
  • php ^5.6 does not allow php 7, you should use php >=5.6. Also, are you sure PHP 5.6 is required at all?

snufkin and others added 3 commits April 17, 2017 22:21
@snufkin
Copy link
Author

snufkin commented Apr 17, 2017

Done or resolved:

  • PRs merged and/or updated relating to CURL and PHP version restrictions. Thanks for contributing the suggested changes.
  • Apache 2.0 seems to be the official licence - as it is stated in numerous files
  • Keyword may be general, however it worked for Stripe, so I figured it would work for Barion.
  • Authors: suggestions are welcome, I have no issues against the current statement.

ToDo:

  • Fixing the autoload, based on team feedback

@KristofMorva
Copy link
Contributor

@snufkin great, thanks! :) Hopefully they'll have the time to check this out. In my opinion Composer & autoload is a must-have.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants