Correct the check for an empty username

Revert addition of custom error-types

One actual logical issue inline.

Fine as-is, but see inlines about Conduit error behavior.

One more simplification

Some simplifications

This version looks correct to me in all non-pathological cases, thanks!

I tried creating, updating, and landing a diff revision with the change to throw an exception and none of the workflows were interrupted/error'd.

Switch to using coalesce(), revert json parsing behavior change and throw exception instead, update .editorconfig

Maybe better querystring parsing

Clean up loop logic

Remove calls to phlog

PhutilJSONParserException is sometimes handled specifically:

My preferred behavior is that the error is reported to the user, but not sent to the error log.

If you want to try to handle these cases, I think it would be reasonable to make AphrontApplicationConfiguration store the raw strings somewhere before it sends them to PhutilQueryStringParser so you don't have to copy/paste 100 lines out of readHTTPPOSTData().

But presumably we're more in "fix urlencode() and call it a day" territory here than "rewrite the whole stack to support pure passthrough for a case that probably never occurs in the wild".

I will take the fix urlencode route for now but this is good to know

In D21864#279131, @epriestley wrote:

The two phlog() commands introduced in error handling seem unrelated, can we strip those out of this change? Everything else looks good.

This is a big mess, but here's some general guidance (see also inline):

I think the JSON change is suspicious, everything else looks good.

The two phlog() commands introduced in error handling seem unrelated, can we strip those out of this change? Everything else looks good.

I probably have missed some code paths but this revision has touched a lot of files as-is

Updating to verify before landing that I haven't included further changes

I didn't catch anything that looks suspicious or hazardous. Thanks!

Revert change that doesn't distinguish between invalid vs. inaccessible dashboards

Fixed another issue with user log entries

(Haven't forgotten about this stuff, but the kids got sick and family is flying in soon.)

Remove unnecessary function

I changed the approach from what I initially had in D21862, which was to add the PhabricatorProfileMenuItem::getDefaultName() as an abstract function and make the default implementation of getDisplayName() to read the 'name' menu property from the config.

My session expired and ran into a few more issues

I jumped the gun and updated my local arcanist to using php 8.2 which introduced the UT errors

Remove changes to profile menu files

Thank you for the thorough explanation. Yesterday I tried searching for how to dynamically add functions to classes but apparently my searching today is better~

Made suggested changes except I haven't pulled out the profile menu stuff yet.

The relation between AuthProvider and AuthProviderConfig and AuthAdapter confused me a bit

Be consistent with usage/checks

I think the string typehint isn't supported until recent-ish PHP, so its availability will depend on your minimum supported version.

As an example, given this code

private $name;

I was ready to suggest that after further investigation was made, recalling the "recent" refactor work of the mercurial command-with-extensions changes we looked at 😄

For https://we.phorge.it/T15281, consider modifying DiffusionGitCommandEngine->newFormattedCommand() to pass explicit configuration to Git (as we do in Mercurial) rather than requiring administrators correctly configure Git via .gitconfig via $HOME.

Gotcha. I'm going to take a swing at updating past 8.0 and see what crops up. Also thanks for the tip with PHPAST. I haven't looked too much into it other than trying to get it working on Windows a year or so back