(I will immediately hard-approve this lovely change as soon as we share whatever ACK like "Yeah let's log the planet" or "Yeah let's just show a user message")

OK the situation is a bit more clear now.

More complete test case added to D25472 which demonstrates the following error:

Interesting. I think this specific crash was not caused by a specific document, but it was probably caused by a specific nonsense request with a nonsense encoding.

Good points. This patch puts also the exception message into phlog.

In T15667#14211, @Sten wrote:

how many people use Phabricator/Phorge :-(

I guess the documentation needs an update on page https://we.phorge.it/book/phorge/article/arcanist_lint_unit/ and/or https://we.phorge.it/book/phorge/article/arcanist_new_project/

I basically completely trust speck if they say this works with TLS, and I verified this works without TLS. Tried with / without this configuration.

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

(I share the same question from Sten)

I'm 5% scared by this since in 8 months we may discover that $author could be an object and so an user may report our well-known exception in some cases. But, the method name is getAuthorString() so I'm quite sure that Evan Priestley already choosen that name for a reason, and so, probably the getAuthor() is always a string or null and nothing else, so it's nice to report alien things with an exception, as proposed here.

In D25428#12247, @avivey wrote:

• We don't use document.querySelector - we use JX.DOM.find, you can see an example in line 43

I love your change. Simple and effective and without impact on other parts.

• Fix double prompt marker

Nice catches, @valerio.bozzolan — will fix.

celerity

Hoping to be useful, proposing minor things. Feel free to say "AAAH NOPE" :D

I completely trust Speck and Evan Priestley that already reviewed this in secure dot. So...

Restoring original author :) Thanks speck

Hi @Cigaryno thanks for this change but we are causing some confusion since the mentioned commit 40b272fa is not in Phorge (as reported by the kind @jmeador from Mozilla). I will try editing a bit. Feel free to review then.

So I'm 99.99% sure that $old and $new always are PHID=>title array and always with count($that) === 1 - whenever you rename a single photo or multiple ones. So, I see this as totally OK.

Interestingly I was able to reproduce the issue with this nonsense patch, in the hope to emulate an email plain-text generation, even if it's not the case:

For interesting reasons, I cannot, in any way, get an email about somebody renaming a single Mockup photo. And, my email preferences are like "email me for everything from every application" and email delivery is "email me about myself".

I'm now quite sure that $old and $new always are array, and each one consists in a single element with key PHID and value => title.

(I also cannot see T15665)

Maybe this is a clue?

Also, I'm OK with the change also because of GDPR's principle of minimization. I mean, Phorge avoids to collect unnecessary data, and this is nice.

As a side note, it's possible that somebody in the world was using the Phone number feature in a way that was then integrated with their custom management system, accessing this information via plain SQL.

I've adopted PhabricatorPeopleUserEmailQuery as suggested.

Apply review tips

(Hide all inline comments or this is unreadable)

Small improvement thanks to review tip

In D25467#13458, @l2dy wrote:

I have verified the fix on Firefox. Are you sure phabricator/c0bdb5b4/core.pkg.css is loaded?

In D25467#13453, @valerio.bozzolan wrote:

Uh! Nice!

Interestingly this CSS change only has effects on Chromium 119 and it does not change anything in my Mozilla Firefox 118. Can you give a quick look?

Interestingly this CSS change only has effects on Chromium 119 and it does not change anything in my Mozilla Firefox 118. Can you give a quick look?

So, no PHP changes? Interesting.

This is related to https://secure.phabricator.com/T13546. "classic" Arcanist workflows were being upgraded to Toolsets, but the migration was not complete. See comments starting with TOOLSETS: , e.g. in ArcanistConfigurationSource.php.

• Wrap long line

• Rephrase Next Steps entry

Opening this up from draft if communication/reviews are happening

Phrasing

Okay I misunderstood the default value. I don’t think a security tag is necessary either.

In D25464#13372, @speck wrote:

This will require documentation of some sort, specifically for the upgrade notes to indicate that if someone relies on rendering PDFs currently then after upgrading they would need to update that configuration.

I think making a task to document the issue and linking in the release notes would be a good approach and what upstream would typically have done.

This will require documentation of some sort, specifically for the upgrade notes to indicate that if someone relies on rendering PDFs currently then after upgrading they would need to update that configuration.

Any problems with this fix?

Is switching to using phutil_nonempty_scalar a blocker?
I don't think it should be because otherwise, we could have just done a bulk change across the whole codebase converting

• Javascript updated to make collapsible containers more generic, and not specific to indices
• Removed unused 'active' as per review
• Moved setting the display of the container from Javascript to CSS as per review
• Added i18n support for the button text (no need for a TODO!)
• Extended variable/attribute names to make the code a bit more self-documenting