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

feat(php): Add Retry Strategy & cache for the PHP client #95

Merged
merged 9 commits into from
Jan 24, 2022

Conversation

damcou
Copy link
Contributor

@damcou damcou commented Jan 18, 2022

🧭 What and Why

🎟 JIRA Ticket: APIC-238

Changes included:

  • Add php 8.0 and composer to the Dockerfile
  • Update post gen script for php client
  • Added needed files for the retry strategy and cache (coming from the current PHP client : https://github.com/algolia/algoliasearch-client-php/tree/master/src )
  • Update instanciation of the Search Client and the Configuration
  • Call the new Api Wrapper instead of the default Guzzle Client coming from the OpenApi generation for each method
  • Cleanup the classes (should be some unused ones)

@damcou damcou marked this pull request as draft January 18, 2022 16:59
@damcou damcou marked this pull request as ready for review January 20, 2022 16:43
Copy link
Collaborator

@millotp millotp left a comment

Choose a reason for hiding this comment

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

I did not review all the Retry Strategy because it's code from for the existing client, but the rest is looking good !
Can you add php to playground.sh pls ?

Comment on lines +27 to +30
"guzzlehttp/psr7": "^2.0",
"psr/http-message": "^1.0",
"psr/log": "^1.0 || ^2.0 || ^3.0",
"psr/simple-cache": "^1.0 || ^2.0 || ^3.0"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you pin the last version for all deps pls ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, this has been recently set this way on the PHP repo to ensure the compatibility with latest PHP versions : https://github.com/algolia/algoliasearch-client-php/pull/696/files . So I'd keep it this way for now.

Copy link
Collaborator

Choose a reason for hiding this comment

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

If we keep having issue with support for old version of software maybe now is a good time to get rid of them and force clients to update.

Copy link
Contributor Author

@damcou damcou Jan 24, 2022

Choose a reason for hiding this comment

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

With PHP, multiple version (from 7.4 to 8.1) are coexisting, that's why we have such config in the composer.json . We can discuss it later with some PHP experts but for now, as it's not really related to the retry strategy itself, I would postpone this potential decision.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added to APIC-271

clients/algoliasearch-client-php/lib/Api/SearchApi.php Outdated Show resolved Hide resolved
/** @var array Hash of readable and writable stream types */
private static $readWriteHash = [
'read' => [
'r' => true, 'w+' => true, 'r+' => true, 'x+' => true, 'c+' => true,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can this be factorized to an array converted to an associative array ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Comment on lines +29 to +39
'http' => 80,
'https' => 443,
'ftp' => 21,
'gopher' => 70,
'nntp' => 119,
'news' => 119,
'telnet' => 23,
'tn3270' => 23,
'imap' => 143,
'pop' => 110,
'ldap' => 389,
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is overkill, we only need http and https

Copy link
Contributor Author

Choose a reason for hiding this comment

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

scripts/post-gen/php.sh Outdated Show resolved Hide resolved
@damcou damcou requested a review from millotp January 24, 2022 15:25
@damcou damcou requested a review from millotp January 24, 2022 15:52
Copy link
Collaborator

@millotp millotp left a comment

Choose a reason for hiding this comment

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

Working nicely ! A next step could be to add dotenv, like the other playgrounds

@damcou damcou merged commit 0fc486f into feat/APIC-238/add-php-client Jan 24, 2022
@damcou damcou deleted the feat/APIC-238/add-retry-strategy branch January 24, 2022 16:49
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