Found two more quick issues

• Adding search results to dashboard hit an error
• Viewing the prototype calendar event failed after creating an event

Looking at the rest of the UT failures I think those are due to updated/newer versions of linters emitting slightly different results.

Fix new exception message to be phtranslated

Correct the check for an empty username

Revert addition of custom error-types

One more simplification

Some simplifications

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

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.

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

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

Fixed another issue with user log entries

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.

Be consistent with usage/checks

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 😄

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

Would this be reasonable to address with a new server-side setup check, similar to the checks for post_max_size and op_cache values?

In T13588#256584, @epriestley wrote:

PHP 8.1 disallows strlen(null).
...
Broadly, Phabricator likely will not pass the empty string as a constraint in these cases...
...
D21762 introduces phutil_nonempty_string(), which is like this test but slightly stricter (it raises an exception for wonky values) and easier to revisit later (i.e., use of this function is a hint that the code can be made more strict about value types).
...
Upshot:

• if (strlen($maybe_null)) may be replaced in all cases with if (phutil_nonempty_string($maybe_null)) to work in PHP 8.1 and not break any code which wasn't already extremely suspicious (...stringlike() may be required if the value may be an object, usually a URI object).
• This is usually not the most desirable replacement, but probably 95% of these aren't realistically worth being surgical about.
• In new code, prefer increasing type strictness over these more-flexible tests.