Remove \Drupal\FunctionalTests\Update\UpdatePathTestBase::$installProfile, use the inherited $profile property instead

Hmm. Ok, so UpdatePathTestBase::$installProfile is written to ['settings']['install_profile'] in UpdatePathTestBase::prepareSettings().

So we search around for how the install_profile setting is used, and discover that install_write_profile() was deprecated in 8.3.0 along with this setting, but neither has a change record:

/**
 * Installation task; writes profile to settings.php if possible.
 *
 * @param array $install_state
 *   An array of information about the current installation state.
 *
 * @see _install_select_profile()
 *
 * @deprecated in Drupal 8.3.0 and will be removed before Drupal 9.0.0. The
 *    install profile is written to core.extension.
 */
function install_write_profile($install_state) {
  // Only write the install profile to settings.php if it already exists. The
  // value from settings.php is never used but drupal_rewrite_settings() does
  // not support removing a setting. If the value is present in settings.php
  // there will be an informational notice on the status report.
  $settings_path = \Drupal::service('site.path') . '/settings.php';
  if (is_writable($settings_path) && array_key_exists('install_profile', Settings::getAll())) {
[...]

It seems as though we’re testing whether a given database dump can be updated. Since that’s the case, we still have to write out the install_profile setting for older versions of core.

Is it functionally the same as the test profile, though? In the case of NoDependenciesUpdateTest we open the gzip fixture (drupal-8.6.0.bare.testing.php.gz) and search for profile information. Hey look, core.extension says: s:7:"profile";s:7:"testing"; which matches NoDependenciesUpdateTest saying: protected $installProfile = 'testing'; This suggests that we want to do an install using the same profile as the fixture. Too bad no one wrote that down in the docblock of UpdatePathTestBase. Regardless, for this particular fixture it shouldn’t actually matter whether we write out the install_profile setting.

In fact, if we change the patch in #2 to say NoDependenciesUpdateTest::$installProfile = NULL; we get a passing test, since the fixture is newer than core 8.3.0. We can even comment out the part that writes that setting from UpdatePathTestBase and get a passing test.

However, if we quit using $installProfile, the patch in #2 gives us basically 200+ fails with the same error message: “Exception: User warning: The following module is missing from the file system: standard”. So where does the standard profile come from? It comes from the fixture.

The answer to the question: ‘Does this need to be separate from BTB::$profile?’ is yes. It represents a different configuration, because it will be written out to the settings file. And there are circumstances where it probably shouldn’t be written out because we're testing a newer version of core.

I think BTB::$profile ends up being ignored for the installed fixture site, in favor of either the install_profile setting, or the core.extension config within the fixture, depending on the fixture.

In my ideal world UpdatePathTestBase::$installProfile should default to NULL, to be overridden and set by tests which explicitly need it. It should also get a docblock update so you know you might not need to write it out to the settings file.

Also, UpdatePathTestBase::prepareSettings() should conditionally write the setting if it’s not NULL.

Here’s a patch that does this, and it should have the same fails as #2. But the reason it has the same fails is because the tests are using fixtures that were generated for core versions < 8.3.0, and so they need the install_profile setting in the settings file. The fix to that is that they explicitly declare that they need that $installProfile.

Note that we also journey down a path where we get to DrupalKernel::getInstallProfile(), which says this:

    // @todo https://www.drupal.org/node/2831065 remove the BC layer.
    else {
      // If system_update_8300() has not yet run fallback to using settings.
      $settings = Settings::getAll();
      $install_profile = isset($settings['install_profile']) ? $settings['install_profile'] : NULL;
    }

#2831065: Remove BC layer from \Drupal\Core\DrupalKernel::getInstallProfile() wants us to remove this BC layer, and when we do, these tests will probably explode again.

Thanks for opening this, @Wim Leers.

Re: #4: Wow. Just wow. ;) @Mile23++ What a tangled web we weave...

Heh, so #2 fails 201 times, but #4 only fails 197 times (yet has 11 more successful tests?). Fun!

Okay, back to $day_job... I can't afford to go down this rabbit hole much further with y'all today. ;)

Similar, different, related.

The answer to the question: ‘Does this need to be separate from BTB::$profile?’ is yes. It represents a different configuration, because it will be written out to the settings file. And there are circumstances where it probably shouldn’t be written out because we're testing a newer version of core.

In this case, could we renaming it to $siteSettingsInstallProfile or something similar.

Since the value should really be the same between $profile and $installProfile, could we introduce a new variable that indicates if the value can be written to the settings file and still get rid of the $installProfile variable.

Oops.

Since the value should really be the same between $profile and $installProfile [...]

What I found in #4 was that it's safe to say that $profile is always overridden by the fixture. $profile is supposed to be the profile you need for the test to run within, but we never actually need to set that for UpdatePathTestBase tests.

$installProfile is a setting that's written out to the settings file, and we only need to do that for some fixtures. If the fixture db dump is from Drupal 8.3.0 or newer, we don't need to (and maybe shouldn't).

So we should:

• Set UpdatePathTestBase::$profile to a profile that doesn't exist (as in #4), so that if the fixture somehow doesn't have a profile we'll get a fail.
• Set UpdatePathTestBase::$installProfile to either NULL or default to standard. If we set it to NULL, then the tests that need to write the settings file will have to explicitly set it, which is good for the test. If we set it to standard the current tests will pass like they have in the past.
• We shouldn't remove $installProfile because we need it for some tests.
• Document these things in the docblocks. :-)

Renaming to $siteSettingsInstallProfile is totally OK, just as long as we document what it does.

I think in a Drupal 9 world this issue has moved on as we expect all database dumps to start from 8.8.0 or greater. So I think once #3109433: Set install profile correctly in the 8.8 database dumps is done we can remove $installProfile completely. It has no effect on dumps created after system_update_8601(). And all update path tests in Drupal 9 need to start from Drupal 8.8.0.

+++ b/core/tests/Drupal/FunctionalTests/Update/UpdatePathTestBase.php
@@ -28,6 +28,9 @@
+ * - If your fixture site is an older version than Drupal 8.3.0, override the
+ *   $installProfile property so that the 'install_profile' setting is written
+ *   out.

@@ -70,9 +75,24 @@ abstract class UpdatePathTestBase extends BrowserTestBase {
   /**
    * The install profile used in the database dump file.
    *
+   * This property will be written out to a fixture settings.php file as the
+   * 'install_profile' setting. This is only necessary for fixture db dumps that
+   * come from Drupal core versions before 8.3.0.
+   *
    * @var string
    */
-  protected $installProfile = 'standard';
+  protected $installProfile = NULL;

@@ -258,11 +278,13 @@ protected function initFrontPage() {
-    // Remember the profile which was used.
-    $settings['settings']['install_profile'] = (object) [
-      'value' => $this->installProfile,
-      'required' => TRUE,
-    ];
+    // Write out the 'install_profile' setting if it's needed.
+    if (!empty($this->installProfile)) {
+      $settings['settings']['install_profile'] = (object) [
+        'value' => $this->installProfile,
+        'required' => TRUE,
+      ];
+    }

+++ b/core/tests/Drupal/FunctionalTests/Update/UpdatePathTestBaseTest.php
@@ -19,6 +19,11 @@ class UpdatePathTestBaseTest extends UpdatePathTestBase {
+  /**
+   * {@inheritdoc}
+   */
+  protected $installProfile = 'standard';

We can remove all of this. And if we like add a deprecation to UpdatePathTestBase that if $this->installProfile is set it is ignored. Note we cannot make these changes in Drupal 8 because dumps older that 8.0.0 are supported.

I think this is all gone now - removed in #2831065: Remove BC layer from \Drupal\Core\DrupalKernel::getInstallProfile()