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

Add generic Psr17Factory #209

Merged
merged 1 commit into from
Feb 9, 2023
Merged

Add generic Psr17Factory #209

merged 1 commit into from
Feb 9, 2023

Conversation

nicolas-grekas
Copy link
Collaborator

@nicolas-grekas nicolas-grekas commented Jan 27, 2023

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Related tickets -
Documentation php-http/documentation#303
License MIT

This PR ports WellKnownPsr17Factory to this repository.

This approach should make it even simpler to write generic PSR17-consuming code.
It would also make the package compatible with factories declared as default values.

Last but not least, the implementation in Psr17Factory provides two additional methods

  • Psr17Factory::createServerRequestFromGlobals()
  • Psr17Factory::createUriFromGlobals()

Providing these two methods provides true portability of PSR-7 implementations.
An example that shows this is missing is Sentry's RequestFetcher, which is currently coupled to guzzlehttp/psr7 because of the call to ServerRequest::fromGlobals().

Example Usage

Inline:

$factory = new Psr17Factory();

// Now we can call any PSR-17 method, e.g.
$factory->createRequest();

As default value on PHP8+:

class Foo
{
    public function __construct(
        private RequestFactoryInterface $requestFactory = new Psr17Factory(),
    ) {
        //...
    }
}

To Do

  • Updated CHANGELOG.md
  • Documentation pull request created
  • Fix tests

Copy link
Contributor

@dbu dbu left a comment

Choose a reason for hiding this comment

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

interesting. it does not directly have to do with discovery, not convinced if it fits into this repository.

looking at what we currently provide, i don't see a clear home for this. but like we have php-http/message about psr 7 things, we could do a php-http/factory repository for it?

i think i would prefer that over having it as part of discovery. wdyt? i could set up that repository and give you commit rights if you want.

src/Psr17Factory.php Outdated Show resolved Hide resolved
src/Psr17Factory.php Show resolved Hide resolved
src/Psr17Factory.php Show resolved Hide resolved
@nicolas-grekas
Copy link
Collaborator Author

it does not directly have to do with discovery, not convinced if it fits into this repository.

Arf. To me, discovery is the package where this makes most sense, because that class is using discovery, and because it fills the same need - decoupling to any specific PSR-17 implem. Making a separate package would just make maintenance harder and slow down adoption to me.

@dbu
Copy link
Contributor

dbu commented Feb 8, 2023

thanks for the discussion in slack @nicolas-grekas , now i got what it does. its a decorator around the findX methods that provides you with an all-of-psr17 factory that autodiscovers its things if needed but accepts constructor arguments when autodiscovery is not wanted.

yep, that fits into this package then.

@nicolas-grekas
Copy link
Collaborator Author

I just pushed tests for the *FromGlobals() methods. PR is ready on my side.

Copy link
Contributor

@dbu dbu left a comment

Choose a reason for hiding this comment

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

great, thanks!

@dbu dbu merged commit c1a1061 into php-http:1.x Feb 9, 2023
@nicolas-grekas nicolas-grekas deleted the global-request branch February 10, 2023 10:46
nicolas-grekas added a commit to symfony/symfony that referenced this pull request Feb 10, 2023
…mplementations" (nicolas-grekas)

This PR was merged into the 6.2 branch.

Discussion
----------

[HttpClient] Revert support for "friendsofphp/well-known-implementations"

| Q             | A
| ------------- | ---
| Branch?       | 6.2
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | -
| License       | MIT
| Doc PR        | -

This reverts PR #47950

The major features of "friendsofphp/well-known-implementations" are now available in "php-http/discovery"  [v1.15.0](https://github.com/php-http/discovery/releases/tag/1.15.0), see:
- php-http/discovery#209
- php-http/discovery#208

Given the extremely low download stats of "friendsofphp/well-known-implementations", I propose to just remove support for it as a bugfix:
https://packagist.org/packages/friendsofphp/well-known-implementations

This will save us from maintaining this integration, which is unused code in practice anyway.

Commits
-------

081aa00 Revert "[HttpClient] Add support for "friendsofphp/well-known-implementations""
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