Skip to content
This repository was archived by the owner on Sep 8, 2021. It is now read-only.

fix #14 send post data to backend when request method is post #17

Merged
merged 1 commit into from
Dec 27, 2018

Conversation

cm-iwata
Copy link
Contributor

@cm-iwata cm-iwata commented Dec 24, 2018

bootstrap must post data to backend when request method is post

@cm-iwata cm-iwata changed the title bootstrap don't send post data to backend even though request method is post send post data to backend when request method is post Dec 24, 2018
@cm-iwata cm-iwata changed the title send post data to backend when request method is post fix #14 send post data to backend when request method is post Dec 24, 2018
Copy link
Member

@txase txase left a comment

Choose a reason for hiding this comment

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

Thanks for figuring this out! I don't really understand why setting the CURLOPT_READFUNCTION doesn't take care of this already, but oh well.

Please test non-form-encoded POST bodies to be sure this doesn't break them. Then this looks ready to be merged!

@@ -156,6 +156,9 @@ while (true) {
}

if (strlen($body) > 0) {
if($event['httpMethod'] === 'POST'){
Copy link
Member

Choose a reason for hiding this comment

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

I think this works if the content type is application/x-www-form-urlencoded. But I'm not sure it works if the body is instead application/json. The docs make it sound like the body will be incompatible with what CURLOPT_POSTFIELDS expects (see http://php.net/manual/en/function.curl-setopt.php).

Can you verify what happens if you post a JSON payload with Content-Type: application/json set? If this change breaks posting non-form-urlencoded bodies, then please add a condition here that checks the content type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I verified some test case.

Verification code

<?php
$input = file_get_contents("php://input");
echo $input;

verify at local machie

prepare builtin server

php -S localhost:8080 index.php

send request

 curl -X POST http://localhost:8080 -H 'Content-Type: application/json' -H 'cache-control: no-cache' -d '{"key": "val"}'

result

{"key": "val"}

verify at AWS using arn:aws:lambda:${AWS::Region}:887080169480:layer:php71:5

send request

 curl -X POST https://xxxxx.execute-api.ap-northeast-1.amazonaws.com/Prod/index.php -H 'Content-Type: application/json' -H 'cache-control: no-cache' -d '{"key": "val"}'

result

empty response body returned

verify at AWS using my layer that patched the issue

send request

 curl -X POST https://xxxxx.execute-api.ap-northeast-1.amazonaws.com/Prod/index.php -H 'Content-Type: application/json' -H 'cache-control: no-cache' -d '{"key": "val"}'

result

{"key": "val"}

I think, It was not send Json Payload from before

I'm not good at English.

Copy link
Member

Choose a reason for hiding this comment

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

Awesome! Looks great, thanks for confirming the body still comes through!

@txase txase merged commit 3e6cbbe into stackery:development Dec 27, 2018
@cm-iwata cm-iwata deleted the add_post_support branch December 28, 2018 04:41
@txase
Copy link
Member

txase commented Jan 4, 2019

This fix is now published in version 6 of the layer!

🙏 @cm-iwata!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants