In the future it may be nice to also have a getArgStr() that always return a string, never NULL. So, we can just if ( $to === '' )

I guess this is still waiting for someone to review it, and not for me to "land" it?

I had no idea that I am supposed to "land" these changes. Surely it should be the responsibility of the maintainers to decide into which release a change will go and when?

Whitelisting both vs and id sound good.

id, vs and /new/ in URL all affect content of the generated file.

In D25485#14022, @speck wrote:

I was thinking we might want to better enable multi-line messages and remove the Enter = submit for both desktop and mobile (on mobile making a send button like most other chat apps).

I spent only 5 minutes playing around on a diff to see what might make the url change. The diff id is probably the key one I’d be interested in keeping. I don’t know what the other params are for (even vs).

In D25478#13957, @speck wrote:

What do you think about keeping the current naming scheme but whitelisting a handful of query params to use instead of using them all? The timestamp adds noise and it's probably nice to see the ID of the diff in the filename, e.g. D25478.id1541.diff? Maybe just allow the the id parameter and regex that it's value is just a string of numbers.

add PHP version information to the view.

Hmm that stack trade doesn’t seem to contain the origin of the issue, likely getting lost through error handling or maybe it’s an incomplete stack trace.

After reverting my patch, I have the following backtrace:

This seems fine, though I wonder if we should introduce a phutil_empty_string() or phutil_is_empty_string function to avoid the double-negative logic. I think this reads more easily:

if (phutil_empty_string($from)) {
  throw new Exception()
}

Is there a stacktrace to work from here? I suspect nothing should really be passing null into phutil_encode_log() and there's likely another issue here. I prefer preventing passing null values into this rather than papering over the issue here in the depths of util functions.

Thank you so much for the header fix that’s been bugging me every time I use conpherence lol

In T15225#13714, @waldyrious wrote:

It's also misleading that the text box is multiline (at least in desktop) which suggests that line breaks are expected, but pressing Enter instead sends the message.

Add maniphest task to the commit message

Fix warning raised by the linter.

Okay, thanks. I've created T15681: PHP8 error running "ssh vcs-user@phorge.yourcompany.com conduit conduit.ping", commits via arcanist should follow soon :)

I'm just glossing over this - is ArcanistPhpunitTestResultParser now learning to parse generic "junit style xml" format?

Testing, using https://github.com/campbsb/example-phorge-php-project.git and breaking it so as to cause a test failure, we get:

• Add LIBXML_NONET option
• Remove $last_test_finished and the !last_test_finished block, as that is always true

Just as a reference point, a few years ago, I created a version of this as well - It supports readCoverage coverage reports, etc.

Thanks for staging this @valerio.bozzolan

6 year old in-your-face unreported bugs like these make me wonder how many people use Phabricator/Phorge :-(

It's not many, but this issue would be specific to anyone using Phabricator/Phorge for PHP development, which is likely Phab/Phorge itself is close to 100% of that market. And Phab/Phorge also itself has its own unit testing framework instead of PHPUnit.

What do you think about keeping the current naming scheme but whitelisting a handful of query params to use instead of using them all? The timestamp adds noise and it's probably nice to see the ID of the diff in the filename, e.g. D25478.id1541.diff? Maybe just allow the the id parameter and regex that it's value is just a string of numbers.

Sorry for the delayed review

Oh interesting

This already has been accepted. I think @Matthew 's old request for changes is what is keeping this from being landable right now.

As I just ran into this again spinning up another machine and had to look this up again, I'd really appreciate if a Blessed Committer could give an "Accepted" state. Thanks in advance!

so something like this might work?

can't repro anymore, looks like the regression regressed

In D25479#13937, @avivey wrote:

maybe use an PhabricatorSearchIntField with a default provided in the buildCustomSearchFields() ?

In D25482#13939, @speck wrote:

Was this the result of a recent change?

Was this the result of a recent change?

If you can hop on IRC/Conpherence in about 4 hours (around 20:00 UTC), I can try debugging this interactively.

maybe use an PhabricatorSearchIntField with a default provided in the buildCustomSearchFields() ?

mmm, ok.

In T15676#14475, @avivey wrote:

What kind of operation are you doing that involves loading all tasks in a browser?

What kind of operation are you doing that involves loading all tasks in a browser?

I'd really prefer not to do that. I pretty often need to work with complete search results (e.g. batch-editing), not with only the first 100 and then repeat all steps for the next 100.

If this is the only place where we have a "limit"/"page size" field, maybe just remove it?