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

Do not imply having parsed the request body #27

Merged
merged 4 commits into from Jun 6, 2020

Conversation

Zegnat
Copy link
Collaborator

@Zegnat Zegnat commented Feb 3, 2019

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 Zegnat requested a review from Nyholm February 3, 2019 16:23
@Zegnat
Copy link
Collaborator Author

Zegnat commented Feb 3, 2019

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
Copy link
Collaborator Author

Zegnat commented Sep 14, 2019

@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
Copy link
Collaborator Author

Zegnat commented Sep 17, 2019

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.

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.
@Zegnat Zegnat mentioned this pull request Jun 6, 2020
Copy link
Owner

@Nyholm Nyholm left a comment

Choose a reason for hiding this comment

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

You are correct. Thank you

\strtolower(\trim(\explode(';', $headerValue, 2)[0])),
['application/x-www-form-urlencoded', 'multipart/form-data']
)) {
$post = $_POST;
Copy link
Owner

Choose a reason for hiding this comment

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

What will $_POST contain if it isnt a POST request with content type application/x-www-form-urlencoded or multipart/form-data?

Copy link
Owner

Choose a reason for hiding this comment

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

It is just an empty array.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes. From my reading of PSR-7, getParsedBody should return null when no (parseable) request body was provided. If we pass along the empty array from $_POST that would be the same as saying we successfully parsed the body content, which may not have been try at al. E.g. we make no attempt to parse JSON bodies.

That is why I wanted to establish a clear difference between [] and null 😄

Copy link
Owner

Choose a reason for hiding this comment

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

Thank you

@Nyholm Nyholm merged commit 4db66c8 into master Jun 6, 2020
@Nyholm
Copy link
Owner

Nyholm commented Jun 6, 2020

Thank you

@Nyholm Nyholm deleted the parsedbody-default-null branch June 6, 2020 11:57
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