Skip to content

Introduce new PSR-18 based client #257

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

Merged
merged 34 commits into from
Mar 22, 2021
Merged

Conversation

Art4
Copy link
Collaborator

@Art4 Art4 commented Mar 18, 2021

Hey,

I would like to introduce a HTTP client agnostic client based on PSR-18 and PSR-17.

I'm not done yet, but would like to hear what you have to say about it.

It is important to me that all changes in this PR are backward compatible. If you find a BC break I would love to fix ist. But maybe this new client could also be a step to a v2 of this project.

Benefits of this PR

  1. Allows the client consuming an app-wide PSR-18 HTTP client (like Guzzle 🎉)
  2. Makes the client a lot more simpler, because all curl opt handling can be removed
  3. No need for port handling, ssl checks and host customization
  4. Allows custom API implementations for not implemented APIs

Switching to Psr18Client

As I said before all changes in this PR are backward compatible. However switching to the new Psr18Client will create some work on the client side.

  1. Requires a PSR-18 HTTP client (like Guzzle).
  2. Requires a PSR-17 ServerRequestFactory and ...
  3. ... a StreamFactory. This can be done with an anonymous class using guzzlehttp/psr7
  4. All curls options must be set to the PSR-18 client (if it is a curl based client) or to the PSR-7 Request (can be done with a middleware)

As you can see in the todo list im planning to create an "migrate guide" to explain the alternatives for features that will not exists in the Psr18Client (like port handling, ssl checks and host customization).

I also have to determine what this PR will mean for an installation without composer.

Todo

  • Create Client interface
  • Create PSR-18 consuming client
  • Move handling of JSON and XML decoding outside of client
  • Make Api implementation client agnostic
  • Implement Redmine Authentication
  • Implement Redmine User impersonation
  • Add tests for json and xml decoding
  • Add tests for file uploads
  • Update example.php
  • Check installation without composer (if necessary)

Cheers 🥂

Update: Creating the migrate guide will be made in another PR.

@Art4
Copy link
Collaborator Author

Art4 commented Mar 20, 2021

I've checked the possibilities for using this library without composer. Because new dependency to psr/http-client and psr/http-factory will been created, the way currently described will no longer work.

Fortunately, there are now online tools that can be used instead of doing this manually:
https://php-download.com/package/kbsali/redmine-api

I think, this should allow us to remove the src/autoload.php and simply refer to php-download.com

@kbsali
Copy link
Owner

kbsali commented Mar 22, 2021

cool @Art4 , I didn't know about this service! 👍

Copy link
Owner

@kbsali kbsali left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

too long to review till the end! 😅
amazing work!!! 💪
I would try to type hint everything (new at least) and get rid of most of the useless docblocks

But this can be done in another PR, up to you!

@Art4
Copy link
Collaborator Author

Art4 commented Mar 22, 2021

I would try to type hint everything (new at least) and get rid of most of the useless docblocks

But this can be done in another PR, up to you!

Typed properties where released with PHP 7.4. I wanted to keep the PHP 7.3 compatibility. I would leave typed properties to a future release to allow a smoother migration.

@Art4
Copy link
Collaborator Author

Art4 commented Mar 22, 2021

Thank you for your first review @kbsali. I will admit your suggestions as best as possible. While working on this PR I developed this possible roadmap:

Release 1.7

  • Introduce new Redmine\Client\Psr18Client, keep everything else as it is.
  • This will allow testing the new client in the wild.

Release 1.8

  • Bump required PHP version to 7.4, add typed properties
  • Create new Redmine\Client\NativeCurlClient that will introduce new curl options setter and forbid/remove some old features.
  • Let Redmine\Client extends from NativeCurlClient
  • Add deprecation warnings in Redmine\Client for a smoother upgrade experience.

Release 2.0

  • breaking Remove Redmine\Client
  • breaking Move Redmine\Client\Client interface to Redmine\Client
  • breaking Make NativeCurlClient final

What do you think?

@kbsali
Copy link
Owner

kbsali commented Mar 22, 2021

I like your plan! :)
one thing though: you are removing the $client->issue in favor of $client->get(issue'), I haven't tested... does it still work the "old way"? If not, this is quite a big breaking change ⚠️

@kbsali
Copy link
Owner

kbsali commented Mar 22, 2021

Also, why adding a new "usage.md" file? How about updating the README instead?

And final point : would you be interested in maintaining the package yourself?

@Art4
Copy link
Collaborator Author

Art4 commented Mar 22, 2021

I like your plan! :)

Thank you. 👍

one thing though: you are removing the $client->issue in favor of $client->get(issue'), I haven't tested... does it still work the "old way"? If not, this is quite a big breaking change warning

I introduced the method getApi() in the client interface. The old client still supports $client->issue and $client->get(issue'), but in Psr18Client one has to switch to getApi().

I now also added tests to guaranty the old behavior on the old client, see d419e1e

Also, why adding a new "usage.md" file? How about updating the README instead?

I thought about moving the example.php and examples from README into a new docs folder. There will be also the migration guide. This would blow up the README.md

But if you wish I could leave that in the README.md. Or we move this docs discussion into a separate PR.

And final point : would you be interested in maintaining the package yourself?

It is a honor for me that you consider me as a maintainer, but I'm afraid I have to decline. I'm already maintaining some projects myself. But I'd be happy to contribute code and bugfixes and reply to issues if I find some time (like in the last days).

@Art4
Copy link
Collaborator Author

Art4 commented Mar 22, 2021

Also, why adding a new "usage.md" file? How about updating the README instead?

I moved the usage docs into README.md. I will create the migration guide into another PR.

From my side this PR is ready for merge.

@Art4 Art4 changed the title [WIP] Introduce new PSR-18 based client Introduce new PSR-18 based client Mar 22, 2021
@kbsali
Copy link
Owner

kbsali commented Mar 22, 2021

awesome work! thanks a lot!
(and too bad for the maintaining offer, if you change your mind please do let me know! :))

@kbsali kbsali merged commit 238cef9 into kbsali:master Mar 22, 2021
@Art4 Art4 deleted the add-psr-18-client branch March 22, 2021 17:53
@Art4 Art4 mentioned this pull request Mar 23, 2021
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

Successfully merging this pull request may close these issues.

2 participants