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

Feature request, extract the http client and support PSR-18 Http Clients #355

Open
Richard87 opened this issue Oct 20, 2020 · 9 comments
Open

Comments

@Richard87
Copy link

Richard87 commented Oct 20, 2020

Hi!

I would love to use my own (Symfony) HttpClient for this package, instead of the built in Curl version.

Would you accept a PR to extract the Client Code into it's own class, and optionally provide a custom HttpClient?

For the why:

I want to use Symfony's HttpClient because it has tracing and devtools built in, and because it would simplify configuration. Basically keep all the HttpClient configurations at the same place, instead of each ApiClient doing their own thing :)

@lesstif
Copy link
Owner

lesstif commented Oct 21, 2020

hi @Richard87

the built in curl based code is bloated and uncleaned, but i had no time.

so i'm glad to your PR.

thanks

@Richard87
Copy link
Author

Richard87 commented Oct 21, 2020

Great!

Is it important for you to ship your own Curl client, but make it easilty replacable, or would you prefer including an existing PSR-18 client (I would recommend Guzzle as a popular client)?

That would minimize the size of the project and the code you would need to maintain :)
That would also make it muche easier for me to build than extracting existing code to a PSR-18 compliant intierface :D

An alternative to requiring a 3rd party client, you could recommend it, and make the user configure the appropriate client, but that would be a breaking change.

List of poular Http clients:
https://packagist.org/packages/psr/http-client/dependents?order_by=downloads

@Richard87
Copy link
Author

Maybe require Guzzle(or similar!) now, and automatically configure it, and in a next major version, you could suggest Guzzle, and leave it for the user to configure it?

The best of both worlds!

@Richard87
Copy link
Author

Hi!

I have started to work on it, but I have a hard time with the "loose definition" of the API, it's incredibly hard to maintain backwards compatibility when there is so many public helper methods...

I will wait for your feedback before continuing.

Most tests pass, but there are a bunch of tests that are hardwired to your Jira instance that fails.

I would love to target this PR for a new Major version, and then make the public API way more restrictive.

  • Require a 'pre-authenticated' Http Client (no more authenticate etc)
  • Remove the custom logging feature, but simply require a PSR-3 compatible Logger and use it if supplied
  • Add proper types in parameter list and return
  • Require PHP 7.3 or 7.4 so we can always use strict json decoding
  • Maybe simplify exception handling, use the Client's error handling, possibly wrap it in JiraException, with the exception trace
  • Not return downloaded-file-content in download() method
  • remove public setRestApiV3 / setJsonOptions / setCookieFile / toHttpQueryParameter / setAPIUri / $cookieFile

Possibly more :)

@lesstif
Copy link
Owner

lesstif commented Oct 30, 2020

Great!

Is it important for you to ship your own Curl client, but make it easilty replacable, or would you prefer including an existing PSR-18 client (I would recommend Guzzle as a popular client)?

That would minimize the size of the project and the code you would need to maintain :)
That would also make it muche easier for me to build than extracting existing code to a PSR-18 compliant intierface :D

An alternative to requiring a 3rd party client, you could recommend it, and make the user configure the appropriate client, but that would be a breaking change.

List of poular Http clients:
https://packagist.org/packages/psr/http-client/dependents?order_by=downloads

sorry for lately reply.

i'm like to guzzlehttp, but if you are familiar other PSR-18 compatible http client, it' ok.

@lesstif
Copy link
Owner

lesstif commented Oct 30, 2020

Hi!

I have started to work on it, but I have a hard time with the "loose definition" of the API, it's incredibly hard to maintain backwards compatibility when there is so many public helper methods...

I will wait for your feedback before continuing.

Most tests pass, but there are a bunch of tests that are hardwired to your Jira instance that fails.

I would love to target this PR for a new Major version, and then make the public API way more restrictive.

  • Require a 'pre-authenticated' Http Client (no more authenticate etc)
  • Remove the custom logging feature, but simply require a PSR-3 compatible Logger and use it if supplied
  • Add proper types in parameter list and return
  • Require PHP 7.3 or 7.4 so we can always use strict json decoding
  • Maybe simplify exception handling, use the Client's error handling, possibly wrap it in JiraException, with the exception trace
  • Not return downloaded-file-content in download() method
  • remove public setRestApiV3 / setJsonOptions / setCookieFile / toHttpQueryParameter / setAPIUri / $cookieFile

Possibly more :)

Great suggestion!

Also i will be new major version targeting support only JIRA Cloud and remove server version support.
If does not need server api support, i can remove setRestAPIV3 method too. (support only V3 API or above)

another plan is rename all setter() method's prefix, for example setIssueType() to issueType(), because it's conflict to jsonmapper' library (See #337 )

thanks!

@Richard87
Copy link
Author

Hi!

Hmm, we are using Jira Server (rest v2), but Atlassian is dropping the product in february... But does the Datacenter version still use v2, or are they on v3 as well?

If they are on v3 I think it's a great idea to drop v2 support :)

@lesstif
Copy link
Owner

lesstif commented Nov 3, 2020

hi. @Richard87

i'm just use jira server and cloud only, so i never ever used jira data center, so i don't know what is it api version.

i will be ask data center api version to the atlassian's local partner company, and will be share do this.

@lesstif
Copy link
Owner

lesstif commented Nov 9, 2020

hi @Richard87

I 'm cancel the plan for drop the V2 API, because JIRA DC still using V2 API. :(

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

No branches or pull requests

2 participants