chargebee / chargebee-php

PHP library for the Chargebee API.
https://apidocs.chargebee.com/docs/api?lang=php
MIT License
72 stars 62 forks source link

Refactoring: testable, PSR-18 http client #53

Closed Wojciechem closed 3 months ago

Wojciechem commented 1 year ago

As a follow-up to #50, I propose to integrate following changes. I tried to introduce smallest changes possible and ensure backwards compatibility (hence no strict typing and reliance on static properties instead of proper dependency injection).

Anyway, if you choose to accept this contribution, I suggest to make it a major release.

Please let me know what you think :)

yched commented 1 year ago

The PR looks awesome ! I'm not sure why it would need to be released in a new 4.x major version though, since it ensures strict backward compatibility by defaulting to the same Guzzle dependency as currently ?

cb-khushbubibay commented 1 year ago

@Wojciechem I sincerely appreciate your effort in submitting this pull request. I will promptly review the changes and endeavor to release them as soon as possible if they align with our project's requirements.

Thank you for your contribution.

sonarcloud[bot] commented 1 year ago

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

KerbenII commented 1 year ago

Nice work Wojciech!

@yched @cb-khushbubibay, we still have some improvements made in our codebase, so if you would be interested we would be happy to add them to the vendor

yched commented 11 months ago

Any updates on this @cb-khushbubibay ? Having a better control on the http client used by the Chargebee library is critical to us.

MarcEspiard commented 9 months ago

@cb-khushbubibay Any chance this could get merged soon? We also really need it in our implementation, mostly to unit test it and have better control on the http client.

sonarcloud[bot] commented 9 months ago

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

yched commented 7 months ago

Very sad to see this getting no attention. Mocking Chargebee requests in our application test suite is still a pain because of this.

This change is a much needed step in getting the Chargebee SDK in line with modern PHP practices.

@cb-khushbubibay a couple months ago you asked @Wojciechem to take the time and effort to create a proper pull request, which he did. Please don't let this rot in silence :-)

zenire commented 6 months ago

This change would really benefit us too. Hoping to see this getting merged soon.

terley commented 6 months ago

Nice work @Wojciechem ! Would also benefit us immensely @cb-khushbubibay

karolwojcik-htg commented 6 months ago

+1 Can't wait to see this PR merged 🤞

cb-sriramthiagarajan commented 6 months ago

Hey all, Sriram here from Chargebee. I'm really sorry that we've not been able to look into this for so long. We're currently working on revamping the build process of all our SDKs and unfortunately we won't be able to make any major changes at this time. We'll definitely pick this up once we're done with our ongoing work by end of March. My apologies again for the delay.

A ton of thanks to @Wojciechem for taking the time & effort for this PR and @yched for your review. Totally appreciate this 🙏

sonarcloud[bot] commented 3 months ago

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

cb-sriramthiagarajan commented 3 months ago

Hi @Wojciechem @yched, thanks a lot again for your contribution. This has been released in v3.28.0. It'd be great if you could give it a try and report if you see any issues. Thanks 🙏

cb-sriramthiagarajan commented 3 months ago

Hi folks, I'm sorry but we had to revert these changes in v3.29.0 as it broke in old PHP versions. We'll get these back in after more investigation. Thanks for your patience.

zenire commented 3 months ago

@cb-sriramthiagarajan which PHP versions were affected causing it to fail?

cb-sriramthiagarajan commented 3 months ago

@zenire — this was failing in PHP 5.x. I guess this would fail in 6.x as well since this issue looks to be related to https://github.com/guzzle/guzzle/issues/2740. It seems that we'd have to upgrade guzzle to v7.4.5 which requires PHP ^7.2.5 || ^8.0.

zenire commented 3 months ago

@cb-sriramthiagarajan thank you. I figured I would try to write a bug fix but since PHP 5.x is EOL since december 2018, I don't think that makes much sense.

tsriram commented 3 months ago

Right! Some of our customers are still on older PHP versions and we don't want to break this especially with a new minor version 😅

We'll analyse this further and see what can be done here. I'll keep this thread posted.