Read "$_POST" before hooking the profiler, and remove "aphront.default-application-configuration-class"
ClosedPublic
Actions

Authored by epriestley on Jan 28 2019, 7:09 PM.

Details

Reviewers

Maniphest Tasks

T4369: Phabricator HTTP repository hosting has fairly severe scalability limits
T12297: Make Conduit API calls on `admin.phacility.com` reasonably easy to profile
T13242: 2019 Week 5 Bonus Content

Commits

rPa24e6deb5798: Read "$_POST" before hooking the profiler, and remove "aphront.default…

Summary

Ref T4369. Ref T12297. Ref T13242. Ref PHI1010. I want to take a quick look at transaction.search and see if there's anything quick and obvious we can do to improve performance.

On secure, the __profile__ flag does not survive POST like it's supposed to: when you profile a page and then submit a form on the page, the result is supposed to be profiled. The intent is to make it easier to profile Conduit calls.

I believe this is because we're hooking the profiler, then rebuilding POST data a little later -- so $_POST['__profile__'] isn't set yet when the profiler checks.

Move the POST rebuild a little earlier to fix this.

Also, remove the very ancient "aphront.default-application-configuration-class". I believe this was only used by Facebook to do CIDR checks against corpnet or something like that. It is poorly named and long-obsolete now, and AphrontSite does everything we might reasonably have wanted it to do.

Test Plan

Poked around locally without any issues. Will check if this fixes the issue on secure.

Diff Detail

Repository

rP Phabricator

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

epriestley created this revision.Jan 28 2019, 7:09 PM

Harbormaster completed remote builds in B21725: Diff 47871.Jan 28 2019, 7:10 PM

epriestley requested review of this revision.Jan 28 2019, 7:10 PM

epriestley mentioned this in D20053: Replace manual query string construction with "phutil_build_http_querystring()".Jan 29 2019, 3:45 AM

I would kind of like to see these changes split into two diffs, but there's nothing wrong with reverting the whole thing if this causes problems.

I've read enough of the linked context to claim that I mostly understand this, but still:

itsmagic

src/aphront/configuration/AphrontApplicationConfiguration.php
784
src/aphront/configuration/AphrontDefaultApplicationConfiguration.php
4

This revision is now accepted and ready to land.Jan 30 2019, 6:13 AM

Closed by commit rPa24e6deb5798: Read "$_POST" before hooking the profiler, and remove "aphront.default… (authored by epriestley). · Explain WhyJan 30 2019, 2:22 PM

This revision was automatically updated to reflect the committed changes.

epriestley mentioned this in T12297: Make Conduit API calls on `admin.phacility.com` reasonably easy to profile.Jan 30 2019, 7:56 PM

Looks like this patch broke serving Phabricator via php built-in server as __path__ variable is not available on $_POST after readHTTPPOSTData method is executed.