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 a new PSR-17 factory to Psr17FactoryProvider #3120

Merged
merged 4 commits into from
Dec 2, 2021
Merged

Add a new PSR-17 factory to Psr17FactoryProvider #3120

merged 4 commits into from
Dec 2, 2021

Conversation

solventt
Copy link
Contributor

There is curious performance benchmark of the PSR-7 and PSR-17 implementations (Guzzle, HttpSoft, Laminas, Nyholm, Slim) - https://github.com/devanych/psr-http-benchmark.

The fastest is HttpSoft, the slowest - Slim.

So I've added HttpSoftPsr17Factory to the Slim's factory set to give users the possibility to use the HttpSoft implementation of the PSR-7.

What was generally done:

1) Adding HttpSoftPsr17Factory::class to the $factories property of the Slim\Factory\Psr17\Psr17FactoryProvider class

protected static $factories = [
    SlimPsr17Factory::class,
    HttpSoftPsr17Factory::class,
    NyholmPsr17Factory::class,
    LaminasDiactorosPsr17Factory::class,
    GuzzlePsr17Factory::class,
];

I placed the class for the second position because it's the fastest implementation of the PSR-7 according to the benchmark. But the Slim's implementation remains in first place because I think it's normal to give priority to packages from the Slim's ecosystem.

2) Adding the new HttpSoftPsr17Factory class to the Slim/Factory/Psr17 folder

3) Adding the httpsoft/http-message and httpsoft/http-server-request packages to the composer.json file (--dev section)

4) Adding [HttpSoftPsr17Factory::class, HttpSoftServerRequest::class] to the provideImplementations() method in the ServerRequestCreatorFactoryTest class

5) Adding [HttpSoftPsr17Factory::class, HttpSoftResponseFactory::class] to the provideImplementations() method in the AppFactoryTest class

6) Editing README.md (added a mention of the HttpSoft PSR-7 implementation along with other implementations)

About tests

  • There were: 420 tests, 779 assertions
  • Became: 422 tests, 781 assertions

@coveralls
Copy link

coveralls commented Oct 15, 2021

Coverage Status

Coverage remained the same at 100.0% when pulling fe7cab8 on solventt:new-psr17-factory into e208383 on slimphp:4.x.

@l0gicgate
Copy link
Member

l0gicgate commented Oct 15, 2021

This is a great contribution, I was not aware that our implementation was this slow. We should probably do some digging since the benchmarking code just seems to be instantiating the objects.

I also wasn't aware of the HttpSoft implementation!

We won't be able to merge this until we bump PHP lowest version support to 7.4 in about a month unfortunately

@l0gicgate
Copy link
Member

We can deprecate PHP 7.3 as of Today @solventt if you would like to add that to this PR

@solventt
Copy link
Contributor Author

@l0gicgate The conflict in composer.json has been resolved

Copy link
Member

@l0gicgate l0gicgate left a comment

Choose a reason for hiding this comment

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

Thank you for this contribution @solventt

@l0gicgate l0gicgate merged commit 0b6376b into slimphp:4.x Dec 2, 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.

None yet

3 participants