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

With the caveat that I mostly dig holes in the dirt now and no longer remember how computers work:

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?

I favor dealing with them on a case-by-case basis since they don't seem especially pervasive and I think in most of the cases where I've fixed the issue, the fix I chose wasn't just to swap the call. I think these strlen() errors are often a correct/useful symptom of undesirable slop in type handling.

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.

Aha! Looks good, then. Thanks!

smoosh the commits?

I am messing something up with git and arc?

Fix the implementation of using pk-zip compressed figlet font files

Okay I figured out what was happening.

1. We observed the issue with using {} as index operator on our production server
2. I updated the Figlet.php on our production server to fix the indexing issue
3. This resulted in getting the zip_open issue, which this error result was cached (return raiseError() vs. a 500 php error)
4. I applied the index issue change and the zip_open change to our test environment and was able to load the diff that was failing in production
5. Someone had previously installed a bunch of figlet fonts on our production server but it was after the last time we duplicated production to our test server - so the tests I was running in our test environment never hit the zip issue directly.
6. On the production server many of the fonts are PK-zip compressed however have the .flf extension. I had been looking for .zip or .tar.gz files so I had assumed none of the fonts installed were compressed.

I'm a little confused about how we ever hit the test for zip_open() (or whatever prompted the original change in D21860) if you don't have any custom compressed fonts and Phabricator doesn't ship with any custom compressed fonts? This change looks good to me, but how did we run into a problem in the first place?

I'm not sure why the change D21860 did not work to decompress the tar/gzip figlet file

Sorry for jumping the gun on landing that previous one. To my surprise we didn't even have any custom figlet font files installed previously. I did some more thorough testing with this change to ensure that custom figlet font files still work properly, unless they're compressed. I'm not sure why the change D21860 did not work to decompress the tar/gzip figlet file but if it's not worth supporting I prefer to remove the complexity.

Oh whoops. I didn't realize figlet font file format wasn't compressed

(It would also probably be fine to just remove support for compressed figlet fonts, they're probably saving like 5KB?)

Thanks!

Thank you it’s very appreciated!

I also cherry-picked this to stable, since I don't have any specific plans around when I'm going to cut the next release right now.

If the small pipe needs to drain into the big one, how about taking a large pvc end-cap, drilling a 1/4" hold and forcing the smaller pipe in it?

If the small pipe needs to drain into the big one, how about taking a large pvc end-cap, drilling a 1/4" hold and forcing the smaller pipe in it? Maybe seal the whole thing with some silicone.

I've been using Fusion 360, which I'd say is "not bad". (See also T13697, perhaps.)

I'm curious what 3d modeling software you use, I've had success making simple boxes and things like that with sketchup but I've been a little intimidated by more advanced modeling software.

Thanks!

...I suspect that's a difficult geometry for FDM in PLA.

These sewing machine shaft collars are kind of tricky: thread unspools around them, so the inward face must have a completely smooth edge that can't catch or snag. This rules out a simple clamping-style shaft collar with a slit in it.

Spool Cap (Small)