SlimPay / hapiclient-php

HAPI Client for PHP.
MIT License
14 stars 12 forks source link

Start using php-fig standards #2

Closed ScullWM closed 8 years ago

ScullWM commented 8 years ago

Very few improvements. Can't be used in production yet.

Start using http://www.php-fig.org/

Some ideas:

Best read here https://github.com/php-fig/fig-standards

YoussefBO commented 8 years ago

Thanks for the link, will definitely look into it. Could you please explain what do you mean by "Can't be used in production yet"?

Also, thanks for the contribution, will review and merge it ASAP.

ScullWM commented 8 years ago

We have choose to implement it by ourself without using your depot for this two reasons:

YoussefBO commented 8 years ago

The HAPI Client is versioned and tagged. We follow the MAJOR.MINOR.PATCH semantic so any external code depending on a major version of this client is backward compatible with all previous releases under the same major version.

Also, as it has come to your attention, the master may contain minor commits not compromising the stability of the code and not important enough to cause a tag and release. These minor commits will be included in the next release.

Regarding the syntax, thank you again for the link.

I would like to add that we chose to publish this HAPI Client on GitHub because it can be of use to anyone using an API based on HAL+json and not only the SlimPay API. We are hoping that people like you would help improve it. As you can see, we are still at the v1.0.0 which means that we still have room for improvements :)

PS: Making the entry point URL as a const is not possible since APIs may have a baseUrl looking like https://example.org and an entry point not at the root ("/") but at a custom URL (e.g https://example.org/index).

ScullWM commented 8 years ago

If you have a release strategy, maybe you should write on it in the readme to make it clear for the new users :smile:

I've made some small patch for the php compatibility: main php framework like Symfony 2.X are php 5.3.9 compatible. It's not a great idea to exclude all thoses php versions (and so framework and existing tools) just for using array shortsyntax.

I don't understand where there is a problem with the entry point url declaration like this

const ENTRY_POINT_URL = '/';
public function __construct($apiUrl = null, $entryPointUrl = self::ENTRY_POINT_URL, $profile = null, AuthenticationMethodInterface $authenticationMethod = null)

And this ?

public function __construct($apiUrl = null, $entryPointUrl = '/', $profile = null, AuthenticationMethodInterface $authenticationMethod = null)
YoussefBO commented 8 years ago

I didn't see you were still using the const as default value in the constructor. What if the $entryPointUrl is set to some other value when instantiating? You would have a public const with a value different from the attribute itself inducing some incoherence.

Regarding your commits could you please review your changes to only provide PSR-2 compliance like your commit messages suggest. If you want to push other commits in order to be PHP 5.3 compatible, please use another pull request/commit or at least specify it in the commit message.

Thank you.

ScullWM commented 8 years ago

I didn't see you were still using the const as default value in the constructor. What if the $entryPointUrl is set to some other value when instantiating? You would have a public const with a value different from the attribute itself inducing some incoherence.

No this don't work like this. It won't change the constant value, it will just don't use the default value (the constant).

Regarding your commits could you please review your changes to [..]

No I won't. I've spend enough time on a repos I don't use ;)

YoussefBO commented 8 years ago

I understand, no problem. We will split it ourselves since it is bad practice to "Mix two unrelated functional changes." in one commit.

Thanks again for the contribution!

YoussefBO commented 8 years ago

PSR-2 coding style released under v1.0.1. PHP 5.3 compatibility won't be merged since this version is obsolete (support for security issues ended on 14 Aug 2014 so you should definitely not use it in production anyway) More PHPDoc will be added when time allows it :)

Sorry for the delayed merge and have a good day.