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

Fallback for getallheaders() missing #101

Closed
moritz-h opened this issue Jul 8, 2019 · 11 comments · Fixed by #111
Closed

Fallback for getallheaders() missing #101

moritz-h opened this issue Jul 8, 2019 · 11 comments · Fixed by #111

Comments

@moritz-h
Copy link

moritz-h commented Jul 8, 2019

Slim-Psr7 does only show the Host header on systems where getallheaders() is missing. My setup is Apache + PHP 7.2 via FPM. But I think there are many other cases, for example using nginx.
Slim3 has a fallback to parse the headers from environment in this case. And so do other PSR7 implementations like Nyholm/PSR7: https://github.com/Nyholm/psr7-server/blob/0.3.0/src/ServerRequestCreator.php#L52

Slim 3 example

composer require slim/slim
<?php
use Psr\Http\Message\ResponseInterface as Response;
use Psr\Http\Message\ServerRequestInterface as Request;
use Slim\App;

require 'vendor/autoload.php';

$app = new App();

$app->get('/', function (Request $request, Response $response, array $args) {
    var_dump($request->getHeaders());
    return $response;
});

$app->run();

Result:

array(10) {
  ["HTTP_UPGRADE_INSECURE_REQUESTS"]=>(...)
  ["HTTP_DNT"]=>(...)
  ["HTTP_ACCEPT_ENCODING"]=>(...)
  ["HTTP_ACCEPT_LANGUAGE"]=>(...)
  ["HTTP_ACCEPT"]=>(...)
  ["HTTP_USER_AGENT"]=>(...)
  ["HTTP_CONNECTION"]=>(...)
  ["HTTP_X_ACCEL_INTERNAL"]=>(...)
  ["HTTP_X_REAL_IP"]=>(...)
  ["HTTP_HOST"]=>(...)
}

Slim 4 example

composer require slim/slim:4.0.0-beta
composer require slim/psr7
<?php
use Psr\Http\Message\ResponseInterface as Response;
use Psr\Http\Message\ServerRequestInterface as Request;
use Slim\Factory\AppFactory;

require 'vendor/autoload.php';

$app = AppFactory::create();

$app->get('/', function (Request $request, Response $response, array $args) {
    var_dump($request->getHeaders());
    return $response;
});

$app->run();

Result:

array(1) {
  ["Host"]=>(...)
}
@l0gicgate
Copy link
Member

@adriansuter should we include the getallheaders() polyfill in the main package? Then this wouldn't happen I think.

@adriansuter
Copy link
Contributor

Not sure about that. This would add a dependency that is actually only needed by some users. Probably we could give a hint in the docs.

@moritz-h
Copy link
Author

I think only write a hint in the docs is not a good solution. The problem is, getallheaders() is an Apache only function, and it works not even in all Apache/PHP configurations (namely PHP via FPM for PHP <7.3)
So i think this will affect more than only a few users. Correct headers are also a requirement for #97, otherwise you do not have a "Content-Type" header to parse.

@adriansuter
Copy link
Contributor

Well then we should add that dependency.

@l0gicgate
Copy link
Member

Yeah let's add it as a dep instead of just a dev dep.

@adriansuter
Copy link
Contributor

@moritz-h Do you write a PR?

@adriansuter
Copy link
Contributor

@l0gicgate Is there a reason why Slim PSR-7 uses ralouphie/getallheaders in version 2?

@moritz-h
Copy link
Author

@adriansuter I can do a PR, but earliest mid of next week. Im on holiday without computer until then.
Do you just want to move the getallheaders polyfill to dependencies or do you want an own implementation without additional install dependency?
(I can try to build something based on current slim 3 fallback code if you want)

@danopz
Copy link
Member

danopz commented Aug 1, 2019

@l0gicgate Is there a reason why Slim PSR-7 uses ralouphie/getallheaders in version 2?

@adriansuter actually you added it 3 months ago - #76 😀
But yea since v2 is used for PHP<5.6 we should update to version 3

@l0gicgate
Copy link
Member

@danopz the problem is it's added as a dev-dependency and not a regular dependency which means that it doesn't get downloaded when someone requires this package.

@danopz
Copy link
Member

danopz commented Aug 2, 2019

@l0gicgate for sure I know, what I just meant is #112

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 a pull request may close this issue.

4 participants