I can also propose another small change to adopt a sigil for the button label, so we don't need an ID.

We need more help on this. Never approved anything similar, I don't have enough experience.

Tried to add a lovely icon but still keeping the label toggling and the whole button selectable

Renamed butt to btn as per PR. Much more common abbrieviation!

Minor, just suggestion

Happy to take your suggestions with a couple of changes

• Remove index from the config labels - we've made collapsible-container.js generic, and not just for an index.
• The reason for the if/else block was to enable long variable/attribute names so as to have self-documenting code. I've done away with it, but we do need a comment instead so as to show why we associate the 'push this to close' text with the open state and vice-versa.

I'm a bit confused (!) I thought there were no PHP changes on Rev 1414 and I approved it, but instead I now see that we have again PHP changes. It's unclear to me whenever the Diff I approved was "updated to reflect changes", or maybe I'm just totally crazy because of real life. Please share your opinion.

• Use dynamically generated platform path where possible

Hoping to be useful we can self-document this joining this group:

(This was already approved but I wait for another "Yeah land" since I changed a thing)

To fix this error:

Btw, there are some similar commands in src/applications/config/check/PhabricatorManualActivitySetupCheck.php. Should I change them as well? How can I "break Phorge" to see this page rendered?

I think there’s still a chunk to do here, including additional documentation during setup

• Actually the path is not needed here.

• Fix missing arguments to hsprintf; re-add csprintf to string using the %R conversion

• Improve prompts: use hsprintf instead of cprintf, add missing <tt> markers, simplify syntax

For future reference, I went digging for the difference between csprintf() and hsprintf(), and found the following:

This might be a duplicate of T15636

As an aside, what do you think of https://github.com/campbsb/example-phorge-php-project - should we put a selection of example projects on https://we.phorge.it, either as individual repos, or one repo containing a bunch of examples, or somewhere in the arcanist repo ?

(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?