search

CyberSource / cybersource-rest-client-php

PHP client library for the CyberSource REST API
29 stars 65 forks source link

Why is APCU a requirement? #92

Closed omerida closed 1 week ago

omerida commented 2 years ago

Looking at the dependencies, there seems to be a hard requirement for APCU for this library to work. However, it doesn't seem to be used outside of lib/Authentication/Jwt/JsonWebTokenHeader.php.

Related, there's a dependency for cache/apcu-adapter in composer.json, but I can't find any place where the class from that package is used. Is it used anywhere?

Longer term, why are you dictacting a caching back end? The REST client could use a PSR-6 Caching compatible implementation and then leave the choice of back ends to the end user.

jim-crawford commented 2 years ago

I second this, We have a syustem where we cannot guarentee the accuracy of this method. All the fucntion does is test if its available and stores the previously collected JWT file. it is then never used again!

Please remove this or at least allow someone to create a fork...

rszrama commented 11 months ago

+1 bump

willevanstyxan commented 4 months ago

+1 bump. Cybersource, this needs to be clarified as it seems not a lot of hosting providers support ACPU and many use other caching solutions.

rszrama commented 4 months ago

@willevanstyxan not a perfect solution, but for my case I ended up just forking this and creating a branch with a couple commits to ensure compatibility with my framework (Drupal 10): https://github.com/centarro/cybersource-rest-client-php/tree/drupal-compatibility

(Not sure I'd recommend using my fork directly, but this approach did work for us for now.)

willevanstyxan commented 4 months ago

@rszrama thanks for the pointer in the right direction.

I noticed on sandbox that the lack of APCU doesn't seem to matter at all anyway. Does it though throw an error on production instead?

rszrama commented 4 months ago

@willevanstyxan I couldn't even install, tbh, due to conflicting dependencies in Drupal 10. If you don't have that problem, it does look like you'd only ever encounter apcu_exists() in some implementations, not necessarily all. I don't know that I really traced it that far out, but I've definitely not had any problems in our production sites. 🙈

rszrama commented 4 months ago

Oh, I see an apcu_fetch() call as well. It's just kinda silly that this is all for a micro-optimization in not loading an encryption key from disk multiple times per second, though I suppose every little bit would help in prod. Just feel like that should be left up to the implementer based on their cache system of choice.

rajvpate commented 3 months ago

@omerida @jim-crawford @rszrama @willevanstyxan : Thank you for reaching out and reporting this.

We are looking into it and will respond back.

gaubansa commented 1 week ago

@omerida @rszrama @willevanstyxan @jim-crawford Thank you for reaching out. We wanted to inform you that the dependency cache/apcu-adapter has been removed from the SDK. This change has been implemented from the released version v0.0.52 onwards. Thus, it is no longer necessary for the SDK. We recommend updating to the latest version to reflect this change. Thank you for your understanding and let us know if there are any other queries or concerns.