Refine multisite detection: many aliases for a single site is fine!

The test failure are because of the database which gets added while running tests. Should I delete at the start and create that database(simpletest_original_default) at the end of all the failing tests?

This looks great, but:

1. we should keep the sites.php check
2. we should allow configuring which database connections to ignore: simpletest_default_original is one, but there could be e.g. database connections to e-commerce systems, warehouse systems, ERPs, and whatever else — those database connections are not Drupal sites!
3. additionally, we could instead try querying each of the databases, and verify which ones are an actual Drupal site, and only complain about those …

I think, No databases are added during build and functional tests so ignoring simpletest_original_defaultfor multisite detection
in kernelTestBase will pass the tests.

Bunch of nits, and one very scary thing that definitely has to be addressed before this can land! 😄

Need help with config inspector, See screen shot on the MR.

🏓

@yash.rode is on vacation, unassigning.

yash.rode → closed merge request !684

I would have fixed the nits myself if the remaining critical bug had been fixed: https://git.drupalcode.org/project/automatic_updates/-/merge_requests/70....

I noticed a couple of minor things.

99.9% done!

Yay!

🚢

Sorry, I should have pointed this out before -- but this really needs a comment to explain what's going on. It's not clear how the presence of multiple databases relates to the presence of a multisite, especially since (AFAIK) a single site can have multiple database connections...defined in settings.php.

I'm happy to add that so there's not more back-and-forth, but I think this needs to be clarified.

Please do.

How did we get the name <code>simpletest_original_default? Is that from upstream? If so, what if it changes?

It's documented right there:

+    // Ignore the backed up original database connection.
+    // @see \Drupal\Core\Test\TestSetupTrait::changeDatabasePrefix()
+    $this->config('package_manager.settings')
+      ->set('multisite_check_ignored_database_connections', ['simpletest_original_default'])
+      ->save();

😅

Welp, that's what I get for reviewing before coffee. ☕️

So I still don't feel comfortable merging (or even modifying) this, because I absolutely do not understand the proposed resolution.

As far as I know, database connection info, even for multiple databases, is resolved from settings.php. That means the database stuff is figured out after the current site path is resolved. I don't see how checking for multiple databases helps us detect a multisite, and I don't think the test coverage necessarily shows us that. Database connection info is not defined in sites.php.

This is something I need to discuss with Wim and/or Ted to confirm, but I strongly suspect that this is not the right fix. For what I can tell, all we're detecting here is a single site that has multiple database connections, which is not what our goal is here.

#37: Isn't the answer right in the issue summary you wrote?

1. In case of a single site, there is no risk in using AU.
2. In case of a multi site with a single DB (i.e. a single uniquesite: many sites from a single DB, with multiple domains served), there is no risk in using AU. 👈 That's what this issue is about.
3. In case of a multi site with multiple DBs (i.e. many unique sites), there is a risk in using AU. 👈 That's what this issue is about.

In HEAD, we're conflating 2 & 3. With this MR, we aren't anymore.

Perhaps you're getting hung up on the "multisite" nomenclature? Perhaps we ought to rename it to SingleSiteOrMultiSiteWithSingleDatabaseValidator or just SingleDrupalDatabaseValidator?

Isn't the answer right in the issue summary you wrote?

It is not. Here's the original issue summary: https://www.drupal.org/node/3267646/revisions/12573129/view →

Okay, looks like @tedbow added that in #10: https://www.drupal.org/node/3267646/revisions/view/12891482/12903165 → .

Still, where is the problem in the more detailed explanation I provided in #38?

Discussed in detail yesterday with @yash.rode, @phenaproxima and @tedbow.

Conclusion: we should stop checking the database altogether, that was a wrong path we went down. The only thing that can sa(f)(n)ely be tested is if there's >1 unique site directory that is actively being used.

Per #41.

I could find nothing to complain about, and believe me, I tried. Merged! Nice to see this one bite the dust.

Hah! 😁

Automatically closed - issue fixed for 2 weeks with no activity.