In T15948#21505, @Cigaryno wrote:

Yeah I was also pretty sure Phab used mysqli? I have had to have it on all my installations, and the Phorge documentation still cites it. https://we.phorge.it/book/contrib/article/database/

Phorge neither uses PHP's mysqli extension nor calls PHP's htmlentities() function.

Are you sure about that? Phorge supports either the old mysql extension or mysqli - and mysql was deprecated in PHP 5.5, and removed in PHP 7.0. Notably, Phorge checks for the existence of mysqli in PhabricatorDatabaseRef, PhabricatorExtensionSetupCheck and PhabricatorPHPConfigSetupCheck.

Patch based on my understanding of taavi's comments in T15963 (as I fail to set up an LDAP server locally):

diff --git a/src/applications/auth/adapter/PhutilLDAPAuthAdapter.php b/src/applications/auth/adapter/PhutilLDAPAuthAdapter.php
index 14047c1761..e25659a4aa 100644
--- a/src/applications/auth/adapter/PhutilLDAPAuthAdapter.php
+++ b/src/applications/auth/adapter/PhutilLDAPAuthAdapter.php
@@ -305,7 +305,16 @@ final class PhutilLDAPAuthAdapter extends PhutilAuthAdapter {
           'port' => $this->port,
         ));

@valerio.bozzolan Could you remove the concern so this isn't listed on https://we.phorge.it/diffusion/commit/ for me? Thanks :)

In T15948#21503, @Cigaryno wrote:

There must be a function that allows Conduit methods to be used by logged-out users. It's just that there are hardly any methods using that function.

There must be a function that allows Conduit methods to be used by logged-out users. It's just that there are hardly any methods using that function.

add PHPDoc

Check if the comments are interesting - thanks for this Phorgi patchi that seems lovely

I'm still a bit shocked by how PHP is so weird. So:

Wow. 10 years of PHP, first time that I notice this

https://www.php.net/manual/en/language.types.array.php#language.types.array.syntax :

Strings containing valid decimal ints, unless the number is preceded by a + sign, will be cast to the int type.

yuuuuuuup

yuuuup

The culprit is $map[$branch] = $branch_head; in https://we.phorge.it/source/phorge/browse/master/src/applications/diffusion/data/DiffusionGitBranch.php$105.
Before $branch was a string, afterwards it is an integer.

Patch approved in 123123121231323μ asd

Yup and thanks that somebody already put "PHID" is the inline description.

Looks good to me, thanks!

Looks good, yeah.

In D25935#25149, @Cigaryno wrote:

Why would a cancel URI be needed?

@avivey does this look good to you?

In D25926#25147, @Cigaryno wrote:

But so far this is nothing meant to be hidden from users who can't edit the repo.

In D25935#25135, @aklapper wrote:

After these steps I get Unhandled Exception ("Exception"): This transaction group requires MFA to apply, but the Editor was not configured with a Cancel URI. This workflow can not perform an MFA check.

Why would a cancel URI be needed? Do you know a Cancel URI for an app with something that prompts for MFA (ie. exposing Passphrases, empowering users, signing comments with MFA, managing your VCS password and SSH keys)

In D25935#25144, @Cigaryno wrote:

In D25935#25135, @aklapper wrote:

Which "an application" exactly?

Any application were canUninstall is not set to false (thus not a required application).

That's what I tested (as the Files application can be uninstalled). Which exact application(s) did you test?
I'm surprised that you did not run into the same problem as I did described in my last comment...maybe it's related to not being an admin?

In D25926#25145, @aklapper wrote:

Socially I remain unconvinced about use cases. Implications are for example exposing hidden (or internal?) URIs under URIs or "Working Copy Status" stuff under Basics to the public. I just so far do not think it's a good idea.

Tested this locally; technically it looks correct to me.

In D25935#25135, @aklapper wrote:

Which "an application" exactly?

Any application were canUninstall is not set to false (thus not a required application).

As which type of user?

A user with the Can Configure Application capability (by default admins).

Fix typos reported by @aklapper.

In D25936#25132, @aklapper wrote:

@Cigaryno: Thanks! Could you elaborate why the change in .arcconfig is needed?

Clear Test Plans with URIs are welcome - the less others need to think "how/where to do that" the easier gets testing.

@Cigaryno: Thanks! Could you elaborate why the change in .arcconfig is needed?

So, this turns out to be a lot harder than I thought.

Should be fine after these two changes :)

There is a downstream patch "Add HTTP Body option in Harbormaster HTTP Build step" in https://github.com/mozilla-conduit/phabricator/commit/9bb9a12079b381ce14af97a8b2c511ae71be389c which might be related and could be lifted?

I think it's relevant to have a status site (likely at status.phorge.it). Uptimerobot sounds like a good option and is used by translatewiki.net.
One thing that's optional to have (likely not possible with Uptimerobot) is a Daemon status checker, as @avivey pointed out when they forgot to start the daemons at some point. For this, a custom status checker that calls a daemon that's not for making edits or changes can be used.

I don't know why, but O1 has to be manually added as a reviewer as it does not own R10.

Thank you both for the conversation here and further thanks @Cigaryno for the patch! :)

I don't know if what I done on D25935 is correct, but as usual, Request Changes if I did it wrong.
I relied on the code on PhabricatorUserEmpowerTransaction to try to make PhabricatorApplicationUninstallTransaction require MFA if enabled.

Make lint happy