search

Nyholm / psr7-server

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

Do not imply having parsed the request body #27

Closed Zegnat closed 4 years ago

Zegnat commented 5 years ago

Strictly follow the PSR-7 guidance where $_POST is used for a request’s parsed body only for very specific requests. All other requests should result in a null value and leave other parsing up to application logic.

The current practice of putting an empty array in may imply we have done parsing and found an empty request body. See also Nyholm/psr7#100.

Zegnat commented 5 years ago

Note: this didn’t break any tests, probably because we aren’t doing a lot of testing on POST requests. Do we need a better way to test for the results of specific HTTP requests? How would we introduce testing for the changes I propose here?

Zegnat commented 5 years ago

@yaskhan thanks again for the review! I think I have now addressed your points, as well as added some tests to try and keep future contributions from breaking anything!

Now I just need to figure out why styleci is breaking even after running php-cs-fixer. Are the configurations correct, @Nyholm?

Zegnat commented 5 years ago

Turns out StyleCI’s Symfony presets include a bunch of “blank line before” rules, while PHP CS Fixer actually disables all of them except for before return when you follow the Symfony presets.

Changed the rules to match StyleCI. @Nyholm let me know if you’d rather change StyleCI settings than PHP CS Fixer settings. I am not sure which one is seen as leading here—or by Symfony.

This will always be BC breaking. Although on the parameter side I merely widened the allowed input, which in itself will not break BC (and is in line with LSP), there may be minor breakage in the output as ServerRequestInterface::getParsedBody() may now return null where it previously would’ve returned an empty array.

Although this is a BC break from the PSR7 Server perspective, the ServerRequestInterface always allowed for null to be returned there so I am not sure this is likely to affect anyone. And we aren’t on a stable 1.0.0 semver API anyway.

Nyholm commented 4 years ago

Thank you