Stick to PSR-2

[LuisUrrutia]

The code must follow PSR-2 standards but It has many issues.

I used PHPCS to check the problems with PSR-2:

phpcs --standard=PSR2 dir/

onepay.txt webpay.txt

[DarkGhostHunter]

Yeah, after tinkering with Webpay SDK I can fully assure it's a bloody mess from 2012 at best.

And OnePay SDK tries to abide to new standards, but only tries.

The only thing that keeps me against remaking Onepay and Webpay SDKs and let rot the old version as deprecated, are the docs. And I think most of the team are thinking the same. I can assure you Webpay SDK hasn't been touched in the last 5 years until recently. And when you're bound to WS-SOAP - a 2001 standard - there is little you can do without breaking something on the user-end.

Personally I would remake Webpay SDK using PHP 7.1 and making new docs, but in the time I end it, the team will come with a REST implementation, so why even bother with a new branch if it will die quickly as a fly. Onepay is the only bound to the docs, so there is hope.

The only thing I could do was a wrapper around the actual Webpay SDK to make it expressive. I was working on something like this:

<?php

$webpay = Webpay::environment('integration');

$transaction = $webpay->transaction()->makeNormal([
    ...//
]);

if ($transaction->send()) {
    return $transaction;
}

throw new \Exception(
    "Transaction $transaction->buyOrder couldn't be processed. Try again"
);

The only thing I want is a beta for the new Webpay SDK. or a WIP branch, and to not abide by a port from other language, but an have an specific PHP-tailored solution - in the end, only the dev won't give a damn about other languages as long it works beautifully in its server. For example, Algolia has an API reference for every language, and some defer greatly.

Also, PHPDocs. That thing saves lives. I need vacations.

[LuisUrrutia]

@DarkGhostHunter even before than 2012, my first time with Webpay was in 2007 and KCC was ... well... not cool...

About SOAP I think there's nothing wrong with it, REST it's from 2000 and internet has it's roots since 1960 and everything has been improved on the user-end since then.

https://github.com/freshworkstudio/transbank-web-services is a good example that you can stick to PSR-2 in a Webpay library (although of course, still has a few details)

PS: I really think that @gonzunigad did such a great job!

[DarkGhostHunter]

After some digging in the Onepay SDK, I came to the conclusion that trying to remake it to abide to better standards, rather than just stick to PSR-2 and efficient patterns, I would need to change the docs. @leosoto said any change was welcome but without touching the docs.

The main offense is using OnepayBase::class and this line:

<?php

namespace Transbank\Onepay;

use Exception;

/**
 * Class OnepayBase
 *
 * @package Transbank\Onepay
 */
class OnepayBase
{
    // ... 
}

OnepayBase::setIntegrationApiKeyAndSharedSecret();

At this point my stance is the following: make a new "beta" branch with better code, which will mean better long-term maintainability. It's not that the API would change every day, but most people won't be prepared for the changes.

My point is simple: the SDK should be expressive for the dev. If I want to add a new Cart Item, I could just simple do this:

<?php

$onepay = Onepay::create('test');

$item = Onepay::makeItem([
    'description' => 'My product',
    'amount' => 19990,
    'quantity' => 1
])

$onepay->makeCart([$item, [
    'description' => 'My Product 2'
    'amount' => 4990,
    // 'quantity' defaults to 1 if not set
]);

$cart = $onepay->cart();

if ($onepay->cart()->send()) {
    var_dump($onepay->cartResponse)
};

Also, PHP 7 comes with a lot of useful features, so much it would clean a lot of code (for example, type declarations will get rid of a lot of if var is type, do this or throw exception.

[leosoto]

Regarding changes: What we don't want is to make it backwards incompatible. So we can add new stuff but we can't break the existing documented API. That said, your snippet has a distinct style not shared by the SDK (which follows the REST operations closely by design). I kind of like[1] the expressive code nevertheless. Have you thought about making it as a wrapper of the official SDK?

Also: Why is OnepayBase::setIntegrationApiKeyAndSharedSecret(); a problem?

[1] With the exception of the weird parts where what should just be return values (the cart, the cart response) appear surprisingly elsewhere. But maybe that's just personal preference.

[leosoto]

(And we are going a bit off-topic but PHP7-only features will only be allowed once we officially drop PHP5.x. I'm not totally sure we'll be able to do so at the end of the year)

[DarkGhostHunter]

Have you thought about making it as a wrapper of the official SDK?

Yeah, I thought about making a wrapper around the SDK, but then it struck me: Why a wrapper if the SDK could be better from the ground up? That would make the SDK better for everybody that uses the PHP version when they come here. But of course, the "don' t change the docs" stopped me, and the wrapper won't make the code better nor maintainable, only more readable.

[1] With the exception of the weird parts where what should just be return values (the cart, the cart response) appear surprisingly elsewhere. But maybe that's just personal preference.

You're right. In that example, it should be getResponse(), receiveTransactionResult() or any other thing that indicates you will get something in return.

Also: Why is OnepayBase::setIntegrationApiKeyAndSharedSecret(); a problem?

1) Forcing static and singleton-esque classes, like OnepayBase:class, makes it impossible to unit test. Indeed, I didn't see any test for that class in particular. 2) If it's an instance, the __construct takes care of setting up any logic. 3) But in the end, the $apiKey and $sharedSecret should be set already (they're actually null). 4) Allows to play nicely with Dependency Injection on major frameworks (Yii, Symfony, Zend, CakePHP, CodeIgniter). If I don't need a singleton, I simply tell the framework not to use it as is (but why would I?). 5) Never get environment variables. That forces the dev to use environment variables as a fallback, which may mean accessing below PHP. Accidental dump and, wholá, there they are.

My only goals are to make the code maintainable for the responsible, and expressive to the dev. Both means: less time writing, more time getting payments 😃

BTW: Sorry for the wallpost.

[leosoto]

Wallpost is welcomed! We are happy to discuss these kind of things in the open.

You are right about singletons vs testing. Our medium/long term plan there is having a non-singleton machinery but still keep the singleton interface (if I'm not making myself sufficiently clear let me know and I go deeper into the topic). That way we can have decent test coverage while still keeping the interface simple. And I know it's a contentious topic, but we are not alone in preferring simpler, closer-to-the-rest-api SDKs. Stripe for example does a similar thing with https://github.com/stripe/stripe-php.

That's why I think a more "expressive API" can easily be done as a layer above the "simpler API". We would be happy to advertise it (so that as much people as possible can use the more expressive thing if it suits their need) and perhaps (no promises here) bundle it on the official SDK if it becomes so popular that everyone is using the expressive API. It's also not a weird approach: Most frameworks evolve in that way (allowing people to build upon them and then the most successful parts become built in).

You are right that we are constraining you. It would be awesome to make the this code more as mantainable as possible. By telling you that you can build the expressive API as a layer above this one we are losing a bit of your help on improving this codebase. But it's not black and white.

You are still super welcomed to make any PR that for example fixes what is reported on this issue (many many warning from not sticking to PSR-2). Internal refactors that make the code more testable while keeping the documented API backwards compatible (it doesn't mean it's frozen, just don't break anything and do a good job justifying any additions) are also great and super welcomed. I bet that if you build the expressive API on top of our SDK you'll wish some things were different and you'll be able to make the necessary changes without breaking the public documented methods.

[DarkGhostHunter]

Okay, I will do the Expressive Wrapper to the SDK. Im sold.

People who want to stick to the old documentation, can download the SDK and use as-it-is, but those who want something more simple, can download the wrapper on top of the SDK.

The only thing I won't back down: It will require PHP 7.1+. 🔱 🔥 Death to old software 🔥 🔱 .

[DarkGhostHunter]

In the end I decided to not waste my time trying to redo this package. It has a lot of bad practices, and these bad practices are reflected up in the docs.

Doing something better means depreciation. If the team doesn't want to break backwards compatibility for the greater good, so be it.

[gdespirito]

We can integrate One Click on my sdk, maybe

On Thu, 26 Sep 2019 at 13:30 Italo notifications@github.com wrote:

In the end I decided to not waste my time trying to redo this package. It has a lot of bad practices, and these bad practices are reflected up in the docs.

Doing something better means depreciation. If the team doesn't want to break backwards compatibility for the greater good, so be it.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/TransbankDevelopers/transbank-sdk-php/issues/48?email_source=notifications&email_token=AAINNBXGALOC43JVKCU7Z3DQLTPQ5A5CNFSM4GALGR42YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD7WFU5I#issuecomment-535583349, or mute the thread https://github.com/notifications/unsubscribe-auth/AAINNBT276CE5SRJ5LFQMP3QLTPQ5ANCNFSM4GALGR4Q .

--

GONZALO DE​-​SPIRITO

FRESHWORK STUDIO CHILE

​+569 8815 3776 | ​ gonzalo@freshwork.clgonzalo@freshwork.cl

[gdespirito]

This PR should fix the PSR-2 coding style. https://github.com/TransbankDevelopers/transbank-sdk-php/pull/149