Exception, when viewing previous revision from blame


#1

Observed Behavior:
Using Skip Past This Commit functionality, when viewing a file content (with blame) in Diffusion causes “InvalidArgumentException” with this message:

Value provided to "replaceQueryParam()" for key "renamed" is NULL. Use "removeQueryParam()" to remove a query parameter.

Expected Behavior:
Show previous revision of a file.

Phabricator Version:
On the https://secure.phabricator.com/ , Week 7, year 2019 published changes.

Reproduction Steps:

  1. open https://secure.phabricator.com/source/phabricator/browse/master/src/applications/differential/view/DifferentialRevisionListView.php URL (or any other file in Diffusion)
  2. click on double back arrow in 1st column for any line (the Skip Past This Commit functionality)
  3. see the exception page from (e.g. https://secure.phabricator.com/source/phabricator/browse/master/src/applications/differential/view/DifferentialRevisionListView.php;dc9aaa0fc2bf41ed01e4beeeca98c442c486fe8e?before=e40aec0210dc1f611b7a2346dab26502c2ab8d33)

#2

So uh, I have no idea how to actually troubleshoot Phabricator at all. It appears that the error actually occurs here. I was thinking calling removeQueryParam($key) instead of replaceQueryParam($key, null) would be good but that evidently wasn’t good. So it’s probably just null at the caller when it shouldn’t be, but I’m not sure. This is the caller (in this stack trace).

Btw this reproduces for me on

  • phabricator 5d88eef26b72623be528f6e448b5615a7cb166c2 (Sat, Feb 23) (branched from 701a9bc339b9d419326a62e85ef13666b08046cd on origin)
  • phutil 813a26a2d09758f3c407a0c99c6761f11f62cb90 (Sat, Feb 23) (branched from 671ec8fb8f7aa74e9e825ca95fd96ce4bbf79160 on origin)

edit: I don’t write PHP, but the syntax is close enough to C# for me to understand it, and stack traces are a big help imo.

This is evidently where the exception is actually thrown. Makes sense. Didn’t realize that was a local method. stupid php :stuck_out_tongue:

Dark console stack trace:

Value provided to "replaceQueryParam()" for key "renamed" is NULL. Use "removeQueryParam()" to remove a query parameter.
Stack trace:
PhutilURI::replaceQueryParam called at [/var/www/phab/libphutil/src/parser/PhutilURI.php:489]
PhutilURI::alter called at [/var/www/phab/phabricator/src/applications/diffusion/controller/DiffusionBrowseController.php:712]
DiffusionBrowseController::buildBeforeResponse called at [/var/www/phab/phabricator/src/applications/diffusion/controller/DiffusionBrowseController.php:108]
DiffusionBrowseController::browseFile called at [/var/www/phab/phabricator/src/applications/diffusion/controller/DiffusionBrowseController.php:46]
DiffusionBrowseController::handleRequest called at [/var/www/phab/phabricator/src/aphront/configuration/AphrontApplicationConfiguration.php:286]
phlog called at [/var/www/phab/phabricator/src/aphront/handler/PhabricatorDefaultRequestExceptionHandler.php:41]
PhabricatorDefaultRequestExceptionHandler::handleRequestThrowable called at [/var/www/phab/phabricator/src/aphront/configuration/AphrontApplicationConfiguration.php:744]
AphrontApplicationConfiguration::handleThrowable called at [/var/www/phab/phabricator/src/aphront/configuration/AphrontApplicationConfiguration.php:298]
AphrontApplicationConfiguration::processRequest called at [/var/www/phab/phabricator/src/aphront/configuration/AphrontApplicationConfiguration.php:209]
AphrontApplicationConfiguration::runHTTPRequest called at [/var/www/phab/phabricator/webroot/index.php:35]

#3

Thanks, see https://secure.phabricator.com/D20222.

Previously, PhutilURI had ->setQueryParam(key, value) and ->alter(key, value) methods which were convenient and usually matched intended usage, but ambiguous given that query strings may repeat parameters (i.e., for the query string ?a=1&a=2, it is not obvious what set(a, 3) or alter(a, 3) will do).

See also: https://secure.phabricator.com/T13251.


#4

Thanks for the quick commit evan, im going to try that out to make sure it works for me.

I’m not sure why but that basically looks like the code i had modified locally at one point (if null, remove, else, alter) but when I tried using that I just constantly got redirected to pages that said ‘can’t continue, this commit added the line’ or something similar, even though it was definitely not the commit that added it.

Edit: Upon further examination it appears that that issue was actually caused because although the file wasn’t added in the previous commit, no commits before it modified the same line so there was probably nothing to blame.

I have this patch applied locally and it LGTM.


closed #5