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

Use HTTP POST vars rather than body to avoid PHP 5.6 warning #48

Closed
chillu opened this issue Jan 11, 2017 · 11 comments
Closed

Use HTTP POST vars rather than body to avoid PHP 5.6 warning #48

chillu opened this issue Jan 11, 2017 · 11 comments

Comments

@chillu
Copy link
Member

chillu commented Jan 11, 2017

http://php.net/manual/en/reserved.variables.httprawpostdata.php

This feature was DEPRECATED in PHP 5.6.0, and REMOVED as of PHP 7.0.0. $HTTP_RAW_POST_DATA contains the raw POST data. See always_populate_raw_post_data. In general, php://input should be used instead of $HTTP_RAW_POST_DATA

@robbieaverill
Copy link
Contributor

This kills asset admin for me.

@sminnee
Copy link
Member

sminnee commented Jan 11, 2017

Are we actually using RAW_POST_DATA?

@robbieaverill
Copy link
Contributor

I can't find it anywhere, but it only seems to happen on the graphql endpoints (for me anyway)

@chillu
Copy link
Member Author

chillu commented Jan 12, 2017

@robbieaverill Can you please dig into this a bit?

@sminnee
Copy link
Member

sminnee commented Jan 12, 2017

The SilverStripe GraphQL module uses HTTPRequest::getBody(), which is populated by Director::direct() line 165 using @file_get_contents('php://input'). It's used that since 2008.

@sminnee
Copy link
Member

sminnee commented Jan 12, 2017

I suspect that it's some weird PHP config, either on Robbie's dev environment, or on the production environment he's deploying to.

@chillu
Copy link
Member Author

chillu commented Jan 12, 2017

At its core, this is a very bad deprecation process in PHP which will hit anybody using POST body: https://www.bram.us/2014/10/26/php-5-6-automatically-populating-http_raw_post_data-is-deprecated-and-will-be-removed-in-a-future-version/. To make matters worse, the generated warning is misleading: It also applies for using php://input with always_populate_raw_post_data=0

Background:

  • The warning was introduces some time after PHP 5.6.10, and obviously only triggers with display_errors=1, so on dev enviornments
  • PHP's default php.ini in 5.6 has the setting commented out, so defaults to 0 (and exhibits the issue), so this is widespread
  • The setting has been removed in PHP 7.0, so this is only a 5.6 issue
  • The POST body is consumed via file_get_contents('php://input') in Director::direct(), at which point the warning is generated - so we can't make any use-case specific switches here
  • The GraphQL spec recommends supporting GET query params, POST query params and POST body, which we implement in SilverStripe\GraphQL\Controller already.

Recommendation:

  • Use the Apollo network layer to set convert the request to HTTP POST vars (see issue).
  • Change dev/graphiql to HTTP POST vars
  • Accept that with other GraphQL clients (e.g. the OSX GraphiQL app) and PHP 5.6 default config, requests will fail.

@chillu chillu added this to the CMS 4.0.0-alpha4 milestone Jan 12, 2017
@chillu chillu changed the title Use php://input instead of RAW_POST_DATA Use HTTP POST vars rather than body to avoid PHP 5.6 warning Jan 12, 2017
@chillu
Copy link
Member Author

chillu commented Jan 12, 2017

Also created a ticket for adding a warning to the installer: silverstripe/silverstripe-installer#147

@kinglozzer
Copy link
Member

Heh, I should’ve probably left my ticket open #38 🙃. Recommendation looks good 👍

@flamerohr
Copy link
Contributor

Pull request created silverstripe/silverstripe-framework#6500

@chillu
Copy link
Member Author

chillu commented Jan 15, 2017

Merged Chris' PR, and split out the GraphiQL one to a new issue on the separate repo: silverstripe/silverstripe-graphql-devtools#1

@chillu chillu closed this as completed Jan 15, 2017
unclecheese pushed a commit to open-sausages/silverstripe-graphql that referenced this issue Jan 9, 2018
unclecheese pushed a commit to unclecheese/silverstripe-graphql that referenced this issue Jan 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants