Nyholm / psr7-server

Helper classes to use any PSR7 implementation as your main request and response
MIT License
90 stars 21 forks source link

Version 1.0? #11

Closed Nyholm closed 4 years ago

Nyholm commented 6 years ago

Are we happy with this interface?

@Zegnat ?

Zegnat commented 6 years ago

I was trying to set this up and got to wondering whether it is right to limit ServerRequestCreatorInterface::fromGlobals to getallheaders for the HTTP headers?

That method is, generally, only available on Apache servers. Anyone currently using this implementation who cannot guarantee their code will run in an Apache environment will have to do something like I had to do:

$requestCreator = $injector->make(Nyholm\Psr7Server\ServerRequestCreator::class);
$request = $requestCreator->fromArrays(
    $_SERVER,
    function_exists('getallheaders') ? getallheaders() : $requestCreator->getHeadersFromServer($_SERVER),
    $_COOKIE,
    $_GET,
    $_POST,
    $_FILES
);

As the interface is already defining a method for getting headers from globals, why not use that?

On that note, if ServerRequestCreatorInterface::getHeadersFromServer is supposed to fill the polyfill-for-non-Apache role, would it be better as a static method? That way it could truly be used as a polyfill in other circumstances too without always having to create a new ServerRequestCreatorInterface instance.

Nyholm commented 6 years ago

It makes sense to fall back on getHeadersFromServer if getallheaders() is not available đź‘Ť

Sure, we can declare it static. Would you mind creating a PR?

Zegnat commented 6 years ago

PRs filed, new issue opened. Basically detailing things I personally would have expected ServerRequestCreatorInterface::fromGlobals to do but found it didn’t do.

mindplay-dk commented 6 years ago

@Nyholm if I may suggest, just a minor change, but "creator" isn't really the normal terminology - it's a factory, right? I'd rename the class with a Factory suffix, which is consistent with your PSR-17 factory used in the example anyhow. Since upgrading to 1.0.0 is a manual chore anyhow, might as well fix this?

mindplay-dk commented 6 years ago

oh, wait, I just realized that would conflict with ServerRequestFactoryInterface in PSR-17.

still, "creator" is unusual terminology - since this isn't actually creating the request, and isn't really a factory either since it depends on the factory, what is it doing? building the server-request maybe? ServerRequestBuilder then?

Maybe just bikeshedding.

mindplay-dk commented 6 years ago

Are there any pending issues blocking a 1.0 release?

If so, I'm happy to help. If not... :-)