The relation between AuthProvider and AuthProviderConfig and AuthAdapter confused me a bit

An AuthAdapter is the raw protocol bits that talk to some auth system and give Phabricator a consistent API for interacting with it. These are generic and have no Phabricator-specific code. Most of them subclass OAuth1Adapter or OAuthAdapter and just plug in all the endpoint URIs for Twitch/Facebook/Google/etc.

An AuthProvider represents a Phabricator-specific way for users to authenticate to an install. In most cases, these are pretty much 1:1 with Adapters (e.g., GitHubProvider is highly related to GitHubAdapter). The separation is partly "Phabricator-Specific Providers" vs "Generic Adapters", and partly because some auth (like "Password") is fairly special and has no meaningful "Adapter" behavior. Most of the code in most Providers is instructions for administrators on how to configure stuff.

These classes (Providers and Adapters) could reasonably be merged, and might be if they were written today. Long ago, there was more emphasis on providing as much code as possible in a generic way, but only a handful of people ever used libphutil as a standalone library and I walked away from this a while ago -- the increased installation complexity of Arcanist wasn't worth the handful of people using libphutil as a library.

A Config is just a storage object for a particular Provider an administrator has configured. You can plausibly configure multiple variants of many provider types (e.g., you could authenticate against two different LDAP servers), so there isn't just one "LDAP" config.

Were these fields migrated at some point? It doesn't look like these fields are ever set or read so maybe they can go away?

Phabricator has a lot of "storage objects", which map to some database table. You can identify these objects because they subclass LiskDAO (possibly several layers deep). DAO stands for "Data Access Object" in this context.

Lisk, the storage object subsystem, does a few bits of magic. The main ones are these:

• All protected properties are mapped to database storage.
• All protected properties have getters and setters generated automatically.

In this case, PhabricatorAuthProviderConfig subclasses LiskDAO (PhabricatorAuthProviderConfig → PhabricatorAuthDAO → PhabricatorDAO → LiskDAO), so it is a storage object. providerType and providerDomain are protected properties, so they'll be loaded from storage by load() and saved to storage with save(). You can inspect the database table to find them (the table name is based on the class name, column names are property names):

mysql> SHOW CREATE TABLE auth_providerconfig;
+---------------------+-------------------------------------------------------------
| Table               | Create Table                                                
+---------------------+-------------------------------------------------------------
| auth_providerconfig | CREATE TABLE `auth_providerconfig` (
  `id` int unsigned NOT NULL AUTO_INCREMENT,
  `phid` varbinary(64) NOT NULL,
  `providerClass` varchar(128) CHARACTER SET utf8mb4 COLLATE utf8mb4_bin NOT NULL,
  `providerType` varchar(32) CHARACTER SET utf8mb4 COLLATE utf8mb4_bin NOT NULL,
  `providerDomain` varchar(128) CHARACTER SET utf8mb4 COLLATE utf8mb4_bin NOT NULL,
  `isEnabled` tinyint(1) NOT NULL,
  `shouldAllowLogin` tinyint(1) NOT NULL,
  `shouldAllowRegistration` tinyint(1) NOT NULL,
  `shouldAllowLink` tinyint(1) NOT NULL,
  `shouldAllowUnlink` tinyint(1) NOT NULL,
  `shouldTrustEmails` tinyint(1) NOT NULL DEFAULT '0',
  `properties` longtext CHARACTER SET utf8mb4 COLLATE utf8mb4_bin NOT NULL,
  `dateCreated` int unsigned NOT NULL,
  `dateModified` int unsigned NOT NULL,
  `shouldAutoLogin` tinyint(1) NOT NULL DEFAULT '0',
  PRIMARY KEY (`id`),
  UNIQUE KEY `key_phid` (`phid`),
  UNIQUE KEY `key_provider` (`providerType`,`providerDomain`),
  KEY `key_class` (`providerClass`)
) ENGINE=InnoDB AUTO_INCREMENT=3 DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_bin |
+---------------------+-------------------------------------------------------------

...and note that these are basically the same fields as the protected properties on the class, plus some extras (id, PHID) added by Lisk:

protected $providerClass;
protected $providerType;
protected $providerDomain;

protected $isEnabled;
protected $shouldAllowLogin         = 0;
protected $shouldAllowRegistration  = 0;
protected $shouldAllowLink          = 0;
protected $shouldAllowUnlink        = 0;
protected $shouldTrustEmails        = 0;
protected $shouldAutoLogin          = 0;

protected $properties = array();

private $provider;

The provider property is private, so it doesn't interact with storage. The id, phid, dateCreated, and dateModified columns are automatic default Lisk behavior. The column type configuration is in getConfiguration():

protected function getConfiguration() {
  return array(
    self::CONFIG_AUX_PHID => true,
    self::CONFIG_SERIALIZATION => array(
      'properties' => self::SERIALIZATION_JSON,
    ),
    self::CONFIG_COLUMN_SCHEMA => array(
      'isEnabled' => 'bool',
      'providerClass' => 'text128',
      'providerType' => 'text32',
      'providerDomain' => 'text128',
      'shouldAllowLogin' => 'bool',
      'shouldAllowRegistration' => 'bool',
      'shouldAllowLink' => 'bool',
      'shouldAllowUnlink' => 'bool',
      'shouldTrustEmails' => 'bool',
      'shouldAutoLogin' => 'bool',
    ),
    self::CONFIG_KEY_SCHEMA => array(
      'key_provider' => array(
        'columns' => array('providerType', 'providerDomain'),
        'unique' => true,
      ),
      'key_class' => array(
        'columns' => array('providerClass'),
      ),
    ),
  ) + parent::getConfiguration();
}

...but note that getConfiguration() does not (normally) "create" columns -- it just adds information (usually type information) to existing columns. The authoritative source of the columns is the list of protected properties on the class.

Since getters and setters are automatically generated and the properties are protected, you'll usually need to look for calls to a getter to identify an unused/unread property. For example, the providerDomain property is accessed via getProviderDomain() (the automatic getter generated by Lisk).

In this specific case, I think providerClass and providerType convey the same information. Using class names is the older way; using type constants is the newer way. Historically, we renamed classes often enough to make using type constants instead of class names seem worth it. I think this got set up for migration, but providerClass hasn't actually been migrated away from. I'd guess you can find some support for this if you dig through blame, but couldn't find a task offhand.

Both providerType/providerClass and providerDomain are actively used (providerDomain isn't meaningful for auth with exactly one valid remote, like GitHub, but is required for anything you can run your own server for, like LDAP, JIRA, etc).

This pretty much all looks fine to me. Per one inline, it would be "nice" to use phutil_nonempty_string() in more cases, but this increases risk a little bit (if some caller is doing some nonsense with setName(567) or whatever, phutil_nonempty_string() will break but just adding a !== null test won't) and the hypothetical upside of making highly-theoretical rework some day marginally easier feels likely-imaginary, so I feel like the case for going back in here and reworking these to use the phutil_nonempty_*() calls is pretty weak.

To make this a bit more manageable, can we do something like this?

• Pull all the profile menu stuff out into a separate diff, since that seems like the largest single piece. I'll look that over a little more carefully but I think we can just land it as-is.
• Get rid of the DatabaseRef change from this diff since I think that's either incorrect (and potentially high-impact) or, at least, outside the scope of PHP8 type correctness. If you can recreate the context that lead to it there's probably some less-invasive fix available. I'm fine with "default host", the ref injection just shouldn't be there unless there are bigger problems somewhere else.
• Leave everything else here and I'll take one more look through it when I'm less tired, but I don't think I caught anything suspicious/fragile/etc.

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~

I'll look at splitting out all the profile menu stuff into a separate diff.