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

Replace HTTP Factory class #141

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jakubpilaralmamedia
Copy link

@jakubpilaralmamedia jakubpilaralmamedia commented Jan 4, 2024

Fix: Package php-http/message-factory is abandoned, you should avoid using it. Use psr/http-factory instead.

composer.json Outdated Show resolved Hide resolved
composer.json Outdated
"psr/http-message": "^1.0",
"ramsey/uuid": "^3.7 || ^4.0"
},
"require-dev": {
"ergebnis/composer-normalize": "^2.4",
"lmc/coding-standard": "^1.3 || ^2.0",
"http-interop/http-factory-guzzle": "^1.2",

Choose a reason for hiding this comment

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

Shouldn't be required oldest possible version with needed features?

Suggested change
"http-interop/http-factory-guzzle": "^1.2",
"http-interop/http-factory-guzzle": "^1.0",

I'm affraid that ^1.2 could be problem if some projects use 1.1.x or 1.0.x versions.

@@ -27,7 +28,7 @@ public function request(): RequestBuilderFactory
}

/** @return $this */
public function setHttpClient(HttpClient $client): self
public function setHttpClient(ClientInterface $client): self

Choose a reason for hiding this comment

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

All these new types look like breaking changes. Are you gonna release new major version? If so, there is no minor version with deprecation notices for easier transition?

Copy link
Member

Choose a reason for hiding this comment

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

I don't think it would be possible / effective to have this transition path with deprecation notices. IMHO releasing new version with new PHP verison dependencies would be enough...

Choose a reason for hiding this comment

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

I think, It's not breaking change, because old HttpClient can be still used, becuase it implements this interface - https://github.com/php-http/httplug/blob/2.x/src/HttpClient.php

CHANGELOG.md Outdated
@@ -5,6 +5,9 @@
<!-- There is always Unreleased section on the top. Subsections (Added, Changed, Fixed, Removed) should be added as needed. -->

## Unreleased
### Added
- Fix: Package php-http/message-factory is abandoned, you should avoid using it. Use psr/http-factory instead.
Copy link
Member

Choose a reason for hiding this comment

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

This is not a fix. It replaces underlying HTTP library interfaces, meaning the signatures of public methods will change. This is unfortunately a breaking change.

This also means there should be an UPGRADE document prepared showing a steps to upgrade (see root directory of the repository for previous upgrade document).

Choose a reason for hiding this comment

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

But there is no changes needed. It will works without changes 🤷

@jakubpilaralmamedia jakubpilaralmamedia force-pushed the replace-http-factory-class branch 5 times, most recently from bb6f769 to 0729bad Compare January 16, 2024 10:07
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