Page MenuHomePhorge

Ineffective instanceof check in PhabricatorMySQLSetupCheck:shouldUseMySQLSearchEngine()
Closed, ResolvedPublic

Description

Per PHPStan static code analysis,
Instanceof between PhabricatorSearchService and PhabricatorMySQLSearchHost will always evaluate to false
in https://we.phorge.it/source/phorge/browse/master/src/applications/config/check/PhabricatorMySQLSetupCheck.php;7aee92b5e95cfc11059e9ff8788e5e53dc88e84d$378

This code was introduced by rPe41c25de5050d69b720424dadbe3d8680362ceaf.

Event Timeline

Ah. Maybe PhabricatorSearchHost should extend PhabricatorSearchService? Boh

I think line 392 should be:

if ($service->engine instanceof PhabricatorFerretFulltextStorageEngine) {

Quick and dirty way to test: Insert this in a random place (output listed after the // ):

$services = PhabricatorSearchService::getAllServices();
foreach ($services as $service) {
  phlog(get_class($service));                   // 'PhabricatorSearchService'
  phlog($service->getDisplayName());            // 'MySQL'
  phlog($service->getEngine());                 // Object PhabricatorFerretFulltextStorageEngine
  if ($service->getEngine() instanceof
      PhabricatorFerretFulltextStorageEngine) {
    phlog('yesh');                              // 'yesh'
  }
}

Now that rP7aee92b5e95cfc11059e9ff8788e5e53dc88e84d is merged (plus realizing that shouldUseMySQLSearchEngine() is only called in https://we.phorge.it/source/phorge/browse/master/src/applications/config/check/PhabricatorMySQLSetupCheck.php;7aee92b5e95cfc11059e9ff8788e5e53dc88e84d$106, plus seeing rP48a34eced28d82b77eb840d05702daffb8e3ddbb), maybe PhabricatorSearchDocument::isInnoDBFulltextEngineAvailable() could be completely removed and this class could instead check the DB software version via mysqli::get_server_info and PDO::getAttribute(PDO::ATTR_SERVER_VERSION) ?
I guess that's a topic for a separate task though...?

PhabricatorSearchDocument::isInnoDBFulltextEngineAvailable() could be completely removed and this class could instead check the DB software version via mysqli::get_server_info and PDO::getAttribute(PDO::ATTR_SERVER_VERSION) ?

Seems like a good solution to me.

I guess that's a topic for a separate task though...?

You're probably right. But while we are here, I say we might as well spam this one with more tangentially related discussion:

Given that approximately nobody uses the elastic backend anymore, it might make sense to remove the check for that entirely. I'm tempted to suggest that we remove the entire scaffolding for configurable back-ends which is provided by rP src/infrastructure/cluster/search/PhabricatorSearchService.php. In theory, it does potentially enable flexibility in the way search traffic is allocated to database servers. For example, you could have one db host for full text search and another db host that only handles regular db queries. This flexibility is not actively harmful, however, I doubt it's being used in practice so it's starting to look more and more like tech debt. This saddens me a little since it was one of my first major upstream contributions to the Phabricator project back in 2017. But practically speaking, that is irrelevant to the usefulness of the code in 2025.