support content-type of application/json on AJAX requests #30678
Conversation
|
🤖 Thank you for contributing to CiviCRM! ❤️ We will need to test and review this PR. 👷 Introduction for new contributors...
Quick links for reviewers...
|
|
test this please |
| static $post = NULL; | ||
| if (!isset($post)) { | ||
| $rawPost = file_get_contents('php://input'); | ||
| $post = json_decode($rawPost, TRUE) ?? []; |
There was a problem hiding this comment.
Does this give reasonable performance? The input JSON is decoded every time a value is requested, instead of only once per request.
There was a problem hiding this comment.
In practice, I don't think this is called very many times per request.
There was a problem hiding this comment.
Yes, I agree. Normally this is only called a couple of times, and json_decode is pretty fast.
However, this loop can become problematic. It will decode JSON again for every key in the request JSON. This quadratic complexity could be abused in a denial-of-service attack.
There was a problem hiding this comment.
@Sjord Shouldn't the static keyword on line 137 (static $post = NULL) prevent this from being decoded multiple times?
There was a problem hiding this comment.
Yes, you are right. Thanks for pointing that out. So it is decoded only once per request, I was mistaken about that.
|
test this please I think the test fails are unrelated, hopefully this will provide some insight. |
|
This is only failing on a test that looks like it's failing generally, |
|
That test was fixed already in master so a rebase would get rid of the failure. Or you can just ignore it. |
Overview
This is #29886, but I've addressed Coleman's concern by fixing the non-deprecated function as well.
I force-pushed my change and tried reopening that PR, but apparently you have to reopen before force-pushing or not at all.
Before
Can't submit JSON-formatted POST requests.
After
You can.
Technical Details
While I don't love the static variable, my profiling of Civi shows that we lose a ridiculous amount of time to serializing/deserializing data (including JSON), even though it uses internal functions. So we only decode the POST request once.
I didn't think this necessary on the deprecated function because it seems far less likely to get called multiple times (it retrieves all values, not just one) but I could be swayed to add it to both (or leave it off of both).