Skip to content

feat(php): Restore Guzzle as a suggestion #888

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 6 commits into from
Aug 3, 2022

Conversation

damcou
Copy link
Contributor

@damcou damcou commented Aug 3, 2022

🧭 What and Why

🎟 JIRA Ticket: APIC-600

Changes included:

OpenAPI default generated client introduced Guzzle as a dependency in the composer file, which is different from the current PHP client where this library is just suggested.
The goal of this PR is to mimic the current behaviour where Guzzle is suggested and where the logic fallbacks on a custom HTTP Client in case Guzzle is not installed on the project.

(PR also contains a fix on the Husky exclusion pattern)

🧪 Test

  • CI / playground

@damcou damcou requested a review from shortcuts August 3, 2022 08:45
@netlify
Copy link

netlify bot commented Aug 3, 2022

Deploy Preview for api-clients-automation canceled.

Name Link
🔨 Latest commit bbc4a84
🔍 Latest deploy log https://app.netlify.com/sites/api-clients-automation/deploys/62ea4512ebedc80008ea4edd

@algolia-bot
Copy link
Collaborator

algolia-bot commented Aug 3, 2022

✗ The generated branch has been deleted.

If the PR has been merged, you can check the generated code on the main branch.
You can still access the code generated on main via this commit.

Copy link
Member

@shortcuts shortcuts left a comment

Choose a reason for hiding this comment

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

Noice

@@ -0,0 +1,151 @@
<?php
Copy link
Member

Choose a reason for hiding this comment

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

It makes me think that our CTS should include tests for all of the requesters we expose, could you please add a task for it?

Copy link
Member

Choose a reason for hiding this comment

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

Also for the JS client, I've imported tests from the previous major to ensure it still work properly, do we have tests for this one? It could be a great addition in the meantime of the CTS

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Created task : APIC-607

@damcou damcou requested a review from shortcuts August 3, 2022 13:05
Copy link
Member

@shortcuts shortcuts left a comment

Choose a reason for hiding this comment

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

Since it introduces a new HTTP client I'd mark it as a feat to be clearer in the changelog

@damcou damcou changed the title fix(php): Restore Guzzle as a suggestion feat(php): Restore Guzzle as a suggestion Aug 3, 2022
@damcou damcou requested a review from shortcuts August 3, 2022 13:15
@damcou damcou merged commit 22f75c4 into main Aug 3, 2022
@damcou damcou deleted the fix/APIC-600/change-composer-guzzle branch August 3, 2022 13:19
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.

3 participants