Write install_profile value to configuration and only to settings.php if it is writeable

What about config directories Conf?

Also #2115533: Add InstallerInterface to provide a stable non-interactive installer
I don't see a problem with that although I note that the interactive installer test assumes you can skip at least the language and profile steps, not sure about the settings.

This is a dupe of #2451363: Ensure install_profile is exists in settings.php after installation which has patches.

This issue was not fixed by #2451363: Ensure install_profile is exists in settings.php after installation at least for the non-interactive installer (i.e. Drush). So I am reopening. To reproduce the problem, install first with minimal and then with standard (or vice versa). You end up with the old profile only in settings.php.

In Drush, we are going to just refuse to proceed with re-install in this situation and leave the fix to this issue - https://github.com/drush-ops/drush/pull/1345

Ah it was fixed by at one point. But then we backed off one of the changes for some sensible reason.

Actaully I've just run the test myself and the value is changed - are you sure about this?

And if I change settings.php to something unwritable I get an error if the profiles don't match.

Seems I was having trouble with this just last week. I'll re-run the tests and confirm here.

alexpott is right and this is working for me as designed.

1. We can still improve the UX when there is a mismatch and settings.php is non-writable. The error tells you to make settings.php writable. It says nothing about the install_profile mismatch. In Acquia's case, it is likely that the customer has to change the value of install_profile, commit to git, and deploy, since our settings.php is never writable. Lets repurpose this issue for this item.
2. Drush drops all your tables and then you rudely find out about the mismatch. Lets handle that in https://github.com/drush-ops/drush/pull/1345

Attached patch skips reporting an error when the distribution can be determined purely via code. This is a big help in environments where settings.php is usually read-only.

I still would like a better error message when we fail to write to settings due to a profile mismatch. I'm not sure yet how to reliably catch that exception. That can be a follow-up, IMO

@alexpott suggested that we replace all calls to Settings::get('install_profile' with a new Settings::getInstallProfile() which implements the fallback to a Distribution. Its a good idea - the attached patch does just that.

1. +++ b/core/lib/Drupal/Core/Site/Settings.php
   @@ -150,6 +150,22 @@ public static function getHashSalt() {
   +   * Get the configured install profile, or fall back to the first Distribution
   +   * that was declared. If neither is available, return NULL.
   

   First part of a docblock should be < 80 characters and a single line.

   No need to have all this information here, the last sentence can be moved to the @return, and the second part also seems unecessary here (it's an implementation detail and not relevant for the caller IMHO)

2. +++ b/core/lib/Drupal/Core/Site/Settings.php
   @@ -150,6 +150,22 @@ public static function getHashSalt() {
   +  public static function getInstallProfile() {
   +    $install_profile = self::$instance->get('install_profile');
   +    if (empty($install_profile)) {
   +      $install_profile = drupal_get_distribution();
   +    }
   +
   +    return $install_profile;
   +  }
   

   Do we get multiple calls to this? Should we statically cache it, or put it inside settings when it's not set?

Should we deprecate drupal_get_profile() in favor of Settings::getProfile()? I also don't really like adding new functions...

Addressed all feedback except for getting rid of drupal_get_profile(). Instead I marked it as deprecated. I can't get rid of it without bringing along its special case for the installer. Since there are 18 callers and we are late in the cycle, I figure we can leave it as a wrapper for now.

I think that this is testable by creating an InstallerTestBase test and having it create the test distribution... oh we have this already - DistributionProfileTest. We should test this is that test.

Added an assertion to DistributionProfileTest. Thanks for the pointer.

So, am I missing something or is this going to do a file scan on every page load if you don't have the setting in place?

It also seems like it won't fall back to standard if nothing is set in settings.php?

Would be helpful to update the summary about what the current proposed resolution is and what problem it's trying to solve.

@pwolanin - i clicked around and can't find any regular pages that call Settings::getInstallProfile(). I think this only happens during Drupal heart attacks like cache rebuilds. During page building, Drupal relies on the config item core.extension which already includes your profile.

I don't think fallback to Standard is appropriate. For one thing, the installer would just pick Standard in that case, and not present a chooser.

I updated the Issue Summary and added Beta Eval

I tried applying this patch to Drupal 8.0-beta12, and installing on a system with a read-only settings.php file, and still received an error:

I tried running Drupal 8 with this patch installed, with a writable settings.php, and the install profile line was still written to settings.php. It seems like this is expected behavior for this patch -- we are only trying to make Drupal still install if the install profile line does not exist in settings.php.

However, I have not yet been successful in making this work when settings.php is not writable, even with a couple of experimental changes to try to help things along. I'll keep trying, though.

Ok, I now see that this patch is only for sites with distributions. Testing #23 on Drupal 8 beta 12 with a distribution works like a charm. This is a great improvement for hosting environments that want to set up their configuration in advance via a read-only settings.php.

Once this is committed, as a next step we could modify #1356276: Allow profiles to define a base/parent profile to favor the distribution that contains a 'base' attribute (inherits from some other distribution).

Note that #31 is a false negative. The bot tested an ancient patch.

Since alexpott looked at this once already, I'm assigning this to him. My apologies if that is bad form.

Thanks Moshe, and thanks Greg for the manual testing!

Since @alexpott most recently just asked for tests I don't think it specifically needs his feedback, so unassigning. However, each time I started to review this I got snagged on miscellaneous spinach that seemed a smidge too much to fix on commit, so attached cleans that up a little. I'd leave it at RTBC since these are utterly tedious coding standards fixes, but it also needs a change record since it includes a deprecation, so NW for the CR once the bot picks it up.

Also fixing a typo or two in the beta eval. It's listed not disruptive but would be good to say why; presumably because it's totally BC/API addition and only affects sites when they are first being installed.

Finally, I'm still a little concerned about #25. Extra file scans even just on cold cache sound worrisome. I think we should confirm that there isn't a performance issue being added here.

And NW for the CR.

1. Added justification for Not Disruptive in the CR.
2. A full cold cache rebuild is already horribly slow. These file scans are trivial compared to all the other cache calculating we need to do at this time. Sites under significant load are probably going to do a cache warming step before going live. Thats not affected by this patch.

Back to RTBC. Hope thats OK. There have been no changes to the patch itself.

It's not just cold cache that would need the additional disk access but going to a page like admin/modules would end up calling drupal_get_distribution() if the install profile is not written to settings.php.

So... I think we can actually make this work with container parameters - which means that the installer logic in install profile can be moved to InstallerKernel (hey this makes sense) - and the install profile ends up being written to the container.

An interdiff is attached but it might be best to just read the patch since it's a big change of approach.

Now with added tests that if we have an existing unwritable settings.php the installer works and the install_profile is correctly determined without being written to settings.php.

Look how the changes to GetFilenameUnitTest are so nice :) - no longer relying on the weird install_state global side effect for testing!

Renamed the container parameter from install.profile to install_profile cause it matches the existing setting. And tidied up a few things.

Looks really great, I would RTBC this - but I am not sure if alexpott does not want to work more on this.

1. +++ b/core/lib/Drupal/Core/DrupalKernel.php
   @@ -1079,6 +1080,7 @@ protected function compileContainer() {
        $container->setParameter('container.modules', $this->getModulesParameter());
   +    $container->setParameter('install_profile', $this->getInstallProfile());
    
   
   +++ b/core/lib/Drupal/Core/InstallProfileFactory.php
   @@ -0,0 +1,42 @@
   +  public function get() {
   +    return $this->drupalKernel->getInstallProfile();
   +  }
   

   I don't see the point of that, isn't the parameter enough?

2. +++ b/core/lib/Drupal/Core/DrupalKernelInterface.php
   @@ -134,4 +134,15 @@ public function preHandle(Request $request);
   +   * If multiple profiles are distributions, then the first discovered profile
   +   * will be selected. See https://www.drupal.org/node/2210443.
   

   That is a bit confusing, but I guess a distribution will never ship with two, I gues?

3. +++ b/core/lib/Drupal/Core/InstallProfileFactory.php
   @@ -0,0 +1,42 @@
   + * Contains \Drupal\Core\AppRootFactory.
   ...
   +class InstallProfileFactory {
   

   Needs the right class

+++ b/core/lib/Drupal/Core/InstallProfileFactory.php
@@ -0,0 +1,42 @@
+ */
+class InstallProfileFactory {
+

I don't see the point of that at all, why is the parameter not enough? At least the factory docs should explain that, and when to use what

I do... adding more testing around assumptions that Drush makes about being able to override profiles even if an existing Settings.php has one set in it. Also the test added for non writable settings.php and a distribution was not quite right. Patch adds plenty of tests for the various scenarios even one where the settings.php is not writable but we have a profile mistmatch and throw and exception in the installer.

We still need a change record, also.

Does the install profile really have to live in settings.php, could it live in state instead?

@catch - if nothing else, you are consistent :). See #1827112-39: Convert 'install_profile' variable to Settings where you asked same question. Then see #55 there where you declared settings.php to be the right place for this.

So, this looks RTBC to me as well.

I'm not sure what the CR would say here. install_profile was not written in D7. Further, any D8 sites in the wild are unharmed by removing the requirement to write this param.

No, this issue has two needs tags and things aren't addressed. Just because you want that in doesn't mean its automatically RTBC :)

+++ b/core/lib/Drupal/Core/DrupalKernelInterface.php
index 0000000..dc6b9af
--- /dev/null

--- /dev/null
+++ b/core/lib/Drupal/Core/InstallProfileFactory.php

+++ b/core/lib/Drupal/Core/InstallProfileFactory.php
+++ b/core/lib/Drupal/Core/InstallProfileFactory.php
@@ -0,0 +1,42 @@

This is totally unused

Sorry, I did not see those.

1. In my last post, I asked a CR would say for this issue. Any input?
2. As for profiling, what is imagined there? This is only in effect during container rebuild which is pretty much ignored from a profiling perspective AFAIK. Don't think this shallow file scan contributes in any significant way. I'm gonna be bold and remove this tag.

I agree that the newest approach does not need profiling, we do way worse things on container rebuild and/or module scan time.

No worries @moshe weitzman (it was late at night)

I agree that the newest approach does not need profiling, we do way worse things on container rebuild and/or module scan time.

I totally agree

I think the fact that the install profile should be injected into services is worthy of a CR (https://www.drupal.org/node/2538996). Very few services require this but doing this will allow new ways of unit testing them. In the patch attached I've used parameter injection in the ConfigInstaller so there is an example in core.

Patch attached also removes bogus InstallProfileFactory - sorry that crept in - it was a partial idea that I dropped because it was unnecessary complexity.

What I really like about that approach is that there is a clear state whether the install profile is determined.

Thanks for the PR, the patch looks really great now!

This changes the behaviour in subtle ways that make me uncomfortable, it might be just a documentation issue, or it might be something we need to account for.

Consider:

1. You install a site with one distribution.

2. Later on you add a different install profile to /profiles, because you're updating your site to be a multi-site.

3. Because nothing is in settings.php, now the file scan finds the different distribution instead of the original one. Fun ensues.

With the current behaviour, once the install profile is in settings.php, it's fixed and changes to the filesystem don't mess with it.

Also wonder if since it's going into a container parameter, whether we can't use services.yml for distributions and fall back to that instead of the filescan. Not sure if that fixes the use case this is intended for though (i.e. whether the installations that don't want to write to settings.php also don't want to write to services.yml).

We can throw an exception if this occurs after a site has been installed and if a site is installed with two distributions we can force the user to choose.

The new exception looks smart to me. I noticed the path core/lib/Drupal/Core/Installer and wondered if Installer is really the right place for this. getDistribution() is in core/lib/Drupal/Core/DrupalKernelInterface.php.

Also saw one typo: distrubtion

Reroll for one conflict and fixed the remaining typo from ##5.

Rerolled. No logic changes. Lets see what fails.

Hi,
Here is a rerolled #60
:)

The patch in #67 applies cleanly to HEAD.

Re-rolled against #57 at @moshe weitzman's request.

I know nothing about the install system, but let me try this on a hunch...

Re-rolled.

Patch failed to apply after commit:
Commit 41e882f on 8.0.x

Still failing testing.

Fixing whitespace issue. Oops!

Missed the new conditional in getProfileStorage(). Submitting re-roll for test again.

Anyone available to push this along? Only 4 fails.

This fixes most of the points.

Reroll against 8.1.x.

Because https://twitter.com/greg_1_anderson/status/721107714903011328 :-)

This patch fixes an error in InstallerExistingSettingsMismatchProfileTest

It was attempting to override getInstallationUrl to pass parameters via the query string which I didn't see in the parent class or anywhere else.

Instead I changed it to override visitInstaller:

 protected function visitInstaller() {
    // Provide profile and language in query string to skip these pages.
    $this->drupalGet($GLOBALS['base_url'] . '/core/install.php?langcode=en&profile=testing');
  }

I am still seeing an failing test in InstallerExistingSettingsMismatchProfileBrokenTest

I am not sure what is happening. I see:

 protected function error($message = '', $group = 'Other', array $caller = NULL) {
    if ($group == 'Exception' && $message == $this->exceptionMessage) {
      // Ignore the expected exception.
      return FALSE;
    }
    return parent::error($message, $group, $caller);
  }

This is the first time I have looked at installer tests but this seem to be trying to suppress the know error.
Through putting in debug messages I can confirm that the return false is being hit but I still see the error causing a failing test.

Anyways hopefully this patch will cause 1 less failing test.

Ok here is patch that fixes one of tests to use \Drupal::installProfile()

Should fix 1 fail. Still not sure why the known error is not being suppressed as I mentioned in #92

1. +++ b/core/includes/bootstrap.inc
   @@ -715,31 +715,19 @@ function drupal_installation_attempted() {
   + * @deprecated in Drupal 8.0.0, will be removed before Drupal 9.0.0.
   

   Let's point to 8.2.x

2. +++ b/core/includes/bootstrap.inc
   @@ -715,31 +715,19 @@ function drupal_installation_attempted() {
   + *   Use \Drupal::installProfile() or the install_profile container parameter
   + *   instead.
   

   Ideally we swap that around

3. +++ b/core/lib/Drupal/Core/Config/ExtensionInstallStorage.php
   @@ -78,15 +77,14 @@ protected function getAllFolders() {
   -      $install_profile = Settings::get('install_profile');
   -      $profile = drupal_get_profile();
   +      $profile = \Drupal::installProfile();
          $extensions = $this->configStorage->read('core.extension');
          // @todo Remove this scan as part of https://www.drupal.org/node/2186491
          $listing = new ExtensionDiscovery(\Drupal::root());
          if (!empty($extensions['module'])) {
            $modules = $extensions['module'];
            // Remove the install profile as this is handled later.
   -        unset($modules[$install_profile]);
   +        unset($modules[$profile]);
   

   We can inject the profile into this class as well.

4. +++ b/core/lib/Drupal/Core/DrupalKernelInterface.php
   @@ -140,4 +140,19 @@ public function preHandle(Request $request);
   +   * If multiple profiles are distributions, then the first discovered profile
   +   * will be selected. See https://www.drupal.org/node/2210443.
   ...
   +   * @throws \Drupal\Core\Installer\Exception\TooManyDistributionsException
   +   *   Thrown when a site has more than one distribution installation profile.
   

   Mh, don't those two lines disagree with each other?

5. +++ b/core/lib/Drupal/Core/Installer/Exception/TooManyDistributionsException.php
   @@ -0,0 +1,16 @@
   +
   +/**
   + * @file
   + * Contains \Drupal\Core\Installer\Exception\TooManyDistributionsException.
   + */
   
   +++ b/core/modules/system/src/Tests/Installer/DistributionProfileExistingSettingsTest.php
   @@ -0,0 +1,165 @@
   +
   +/**
   + * @file
   + * Contains \Drupal\system\Tests\Installer\DistributionProfileExistingSettingsTest.
   + */
   
   +++ b/core/modules/system/src/Tests/Installer/InstallerExistingSettingsMismatchProfileBrokenTest.php
   @@ -0,0 +1,133 @@
   +/**
   + * @file
   + * Contains \Drupal\system\Tests\Installer\InstallerExistingSettingsMismatchProfileBrokenTest.
   + */
   +
   
   +++ b/core/modules/system/src/Tests/Installer/InstallerExistingSettingsMismatchProfileTest.php
   @@ -0,0 +1,106 @@
   +
   +/**
   + * @file
   + * Contains \Drupal\system\Tests\Installer\InstallerExistingSettingsMismatchProfileTest.
   + */
   
   +++ b/core/modules/system/src/Tests/Installer/MultipleDistributionsProfileTest.php
   @@ -0,0 +1,106 @@
   +
   +/**
   + * @file
   + * Contains \Drupal\system\Tests\Installer\MultipleDistributionsProfileTest.
   + */
   

   Let's not add all of them here.

6. +++ b/core/modules/system/src/Tests/Bootstrap/GetFilenameUnitTest.php
   @@ -12,14 +13,18 @@
      /**
   +   * {@inheritdoc}
   +   */
   +  public function containerBuild(ContainerBuilder $container) {
   +    parent::containerBuild($container);
   +    // Use the testing install profile.
   +    $container->setParameter('install_profile', 'testing');
   +  }
   +
   

   Should we set this up in KernelTestBase itself?

Ok here is patch that addresses @dawehner's review and

In InstallerExistingSettingsMismatchProfileBrokenTest the last line is new here.

public function testBrokenInstaller() {
    $this->assertRaw(Html::escape($this->exceptionMessage));
    // The exceptions are expected. Do not interpret them as a test failure.
    // Not using File API; a potential error must trigger a PHP warning.
    unlink(\Drupal::root() . '/' . $this->siteDirectory . '/error.log');
  }

After chatting with @wimleers he point this out to me. It is used when a known error is written out to error.log. Errors in would then be read in simpletest_log_read and add false failing test.

Also in \Drupal\Core\Config\ExtensionInstallStorage::getAllFolders changed

-        if (isset($this->profile)) {
+        if ($this->profile) {

Because \Drupal::installProfile() will return back a false if no profile is set yet I believe. But I am clear whether that would ever happen in this point in the install.

Whoops! Think I see what I did wrong here. Another patch coming shortly.

Ok this patch fixes those test errors by adding \Drupal\system\Tests\Bootstrap\GetFilenameUnitTest::containerBuild and removing the logic KernelTestBase.

This caused a bunch of tests to fail. Not sure why.

I also fixed some dependency injection issues from my last patch.

This caused a bunch of tests to fail. Not sure why.

That honestly sounds like a problem for me. If the code which kind of works on production fails ... we should better figure out now, as otherwise it will bite us in the future.

Ok, @dawehner, I should have said I didn't have time to look into it yet.

I will.

Ok haven't figured this out yet but the exception that is being trigger comes from drupal_get_filename in bootstrap.inc

// If still unknown, create a user-level error message.
    if (!isset($files[$type][$name])) {
      trigger_error(SafeMarkup::format('The following @type is missing from the file system: @name', array('@type' => $type, '@name' => $name)), E_USER_WARNING);
    }

This problem doesn't show up in GetFilenameUnitTest because it doesn't set the $modules array.

If you add

/**
   * {@inheritdoc}
   */
  public static $modules = array('system');

to the class it gets the same problem.

So the problem is not which containerBuild you add

// Use the testing install profile.
    $container->setParameter('install_profile', 'testing');

to. but rather the problem shows up when you use that line AND the $modules array is NOT empty.

drupal_get_filename get called multiple types with $name == 'testing'
The first time it gets called the $files never gets filled out so this error is triggered.

On subsequent calls $files has been filled out and then this error doesn't happened.

I will take another look tomorrow.

Ok, here is the problem.
In 8.1.x \Drupal\Core\Extension\ExtensionDiscovery::setProfileDirectoriesFromSettings gets called from \Drupal\simpletest\KernelTestBase::enableModules
$module_list = $listing->scan('module');
setProfileDirectoriesFromSettings has
$profile = drupal_get_profile();
In 8.1.x returns null so further down

// In case both profile directories contain the same extension, the actual
    // profile always has precedence.
    if ($profile) {
      $this->profileDirectories[] = drupal_get_path('profile', $profile);
    }

Is skipped

with this patch
$profile = drupal_get_profile();
Returns 'testing.
So that if statement calls
drupal_get_path('profile', $profile);
Which ultimately causes the problem in drupal_get_filenamethat I mentioned in #103

My guess is something needs to be added to \Drupal\simpletest\KernelTestBase::containerBuild to make not through that error but maybe who is more familiar with KernelTestBase and ExtensionDiscovery would have better idea.

Ok, after chatting with @dawehner about what I found in #103 and #104 I am going to make separate issue to see if we should add

// Use the testing install profile.
    $container->setParameter('install_profile', 'testing');

To \Drupal\simpletest\KernelTestBase::containerBuild

So putting back to Need review for patch in #100

Regarding my comment there

This caused a bunch of tests to fail. Not sure why.

We do know why now explained in #103 and #104

@tedbow
Do you mind opening up the follow up issue, so we don't forget about it?

1. +++ b/core/includes/bootstrap.inc
   @@ -715,31 +715,19 @@ function drupal_installation_attempted() {
   + *   Use the install_profile container parameter or \Drupal::installProfile()
   + *   instead.
   

   I would just let people not know about \Drupal::installProfile(). Code that needs the installation profile is not really common, so let's just give them good practises

2. +++ b/core/lib/Drupal/Core/DrupalKernelInterface.php
   @@ -140,4 +140,18 @@ public function preHandle(Request $request);
   +   *
   +   * @throws \Drupal\Core\Installer\Exception\TooManyDistributionsException
   +   *   Thrown when a site has more than one distribution installation profile.
   ...
   +  public function getDistribution();
   

   Here is one question ... is this method useful in the public outside of the context of an installer? Also given the exception it sounds like the method be better just to available in the InstallerKernel. Any thoughts about that?

Here is one question ... is this method useful in the public outside of the context of an installer? Also given the exception it sounds like the method be better just to available in the InstallerKernel. Any thoughts about that?

Yeah it seems to me that this is only useful during install.

So if we moved this to InstallerKernel then we would also have to override getInstallProfile in InstallerKernel because it calls $this->getDistribution().

So if we moved this to InstallerKernel then we would also have to override getInstallProfile in InstallerKernel because it calls $this->getDistribution().

Well, we could keep the getDistribution, but keep it protected by default and just expose it publicly in the installer.

It seems to me that `DrupalKernel::getDistribution()` is needed outside of the context of the installer. If there is no install profile set in settings.php, then `DrupalKernel::getInstallProfile()` will fall back on `getDistribution()` to determine the installation profile to use. The result of `getInstallProfile()` is exposed by the `install_profile` parameter in the container, which is used by the config installer, the locale module, and potentially other sources. So, moving this behavior into `InstallerKernel::getInstallProfile()` would not work. I believe this is what @dawehner implicitly meant above.

It is easy enough to remove `getDistribution()` from DrupalKernelInterface, and place the public method in InstallerKernel, as suggested. However, `getDistribution()` is called in install.core.inc via `\Drupal::service('kernel')->getDistribution()`. If we removed `getDistribution()` from the interface, then this code would still happen to work, because the 'kernel' service is always an InstallerKernel at this point in the code. I've tried this, it works; I could provide the patch if we wanted to go this way. However, without having `getDistribution()` in the interface, there is no contract that guarantees that this method is available in this service, so this code change is perhaps not desirable.

I talked with @alexpott about this, and his feeling is that `getDistribution()` is similar to `getSitePath()` in terms of usage and requirements, and should therefore be fine to leave in the DrupalKernelInterface.

I tested #100 on Pantheon on a read-only filesystem with a distribution, and it works very well. Pending agreement on #109, I think this is good.

Re-roll

Failure using this patch, #2466197: Staging directory should not have to be writeable. and a hacked config_installer (https://gist.github.com/heddn/c8c1f423715fc78880773925d997d630) on platform.

Array to string conversion install.core.inc:2158                        [notice]
Drupal\Core\Installer\Exception\InstallerException: File system:         [error]
Writable (<em>public</em> download method)

Array

Settings file: The <em class="placeholder">Settings file</em> is not
writable.

The Drupal installer requires write permissions to <em
class="placeholder">./sites/default/settings.php</em> during the
installation process. The <a
href="https://www.drupal.org/server-permissions">webhosting
issues</a> documentation section offers help on this and other
topics. in /app/web/core/includes/install.core.inc:2162
Stack trace:
#0 /app/web/core/includes/install.core.inc(1019):
install_display_requirements(Array, Array)
#1 /app/web/core/includes/install.core.inc(653):
install_verify_requirements(Array)
#2 /app/web/core/includes/install.core.inc(531):
install_run_task(Array, Array)
#3 /app/web/core/includes/install.core.inc(116):
install_run_tasks(Array)
#4 /app/vendor/drush/drush/includes/drush.inc(726):
install_drupal(Object(Composer\Autoload\ClassLoader), Array)
#5 /app/vendor/drush/drush/includes/drush.inc(711):
drush_call_user_func_array('install_drupal', Array)
#6 /app/vendor/drush/drush/commands/core/drupal/site_install.inc(80):
drush_op('install_drupal', Object(Composer\Autoload\ClassLoader),

Is #115 related? This issue avoids writing to settings.php if a distribution provides an installation profile. It does nothing to allow installation to succeed when settings.php is written for some other reason, so I think that the problem above should be addressed in a follow-on issue -- either in #2466197, or some other issue, as appropriate.

re #116, I was not sure where to post my test result, so this might be in the wrong location. My local hack is some early work on my part (not ready for public use) for #2723499: drupal_rewrite_settings called in SiteConfigurationForm. May#115 should be directed at it.

I still disagree to add the method to the runtime interface, but well, I don't care that much. You know, its just sometimes helpful to keep the surface area reasonable.

+++ b/core/lib/Drupal/Core/DrupalKernelInterface.php
@@ -138,6 +138,20 @@ public function preHandle(Request $request);
+  /**
+   * Get the name of any discovered profile that is a distribution.
+   *

The documentation doesn't make it clear whether this distribution was actually used during install.

+++ b/core/lib/Drupal/Core/DrupalKernel.php
@@ -1471,6 +1474,42 @@ protected static function setupTrustedHosts(Request $request, $host_patterns) {
+   */
+  public function getDistribution() {
+    $listing = new ExtensionDiscovery($this->root);
+    $listing->setProfileDirectories(array());
+    $info_parser = new InfoParser();

Can we add to the documentation that this actually scans the filesystem or so?

@dawehner: I care more about getting this patch in than whether or not the method is part of the interface. Talk to @alexpott; I already have a patch that implements #108 if you guys decide that's the way you want to go. I'd just need to reroll it / add it to #114 to put it in -- not hard.

Documentation requests above are reasonable. I'll add that once there's a decision on #108, if someone else doesn't get to it first.

Discussed with @dawehner. Let's go for not exposing the getDistribution() method public on the regular run-time kernel. If we find the need later then we can open another issue to discuss it. However if we make it public now then going backwards is super hard. So let's get the fix that everyone wants and have the API discussions later if necessary.

@greg.1.anderson sorry I should have suggested this approach yesterday - at least you already have the patch :)

Removing getDistribution() from the interface. Let's see how this fares with the testbot.

We need to test getDistribution() when we have an InstallerKernel.

Update version and run the tests.

Thank you a lot! I really like how this is now working. Nothing is really added to the runtime surface

It's a little harder to test this way; getDistribution() must be called on the InstallerKernel, so the test must happen after the kernel is created, but before the installation is completed. I'll get it right eventually...

Actually, since the problem is just in the test, I just put the code back the way it was, but used reflection to make the method accessible so that it could be called indirectly in the test.

Here's the interdiff for 128.

This looks great for me!

A few coding standards issues that needed addressing.

.

Unrelated test fails

In a conversation with Greg, it sounded like you need to denote the install profile as a 'distribution'. Should we write some release notes to explain how to make Drupal install as a distribution and fool things so it will install on a RO filesystem? My testing has been with minimal, standard and config_installer. All of which aren't technically distributions.

@heddn It's not just about distributions - it is also if the install profile is already set in the settings.php and the chosen profile matches it - then, with this patch, it will no longer attempt to write it.

Patch Failed to apply
error: core/modules/system/src/Tests/Bootstrap/GetFilenameUnitTest.php: No such file or directory

This is different to the unrelated failing migrate test mentioned by @dawehner and @alexpott.

As far as I can see GetFileNameUntiTest has not been touched since May 8, and patch applied fine on May 23.

Does this need a reroll or is the fail still unrelated?

In order to port this to 8.1.x we would ideally

a) keep ConfigInstaller::getInstallationProfile (and call out to some other code)
b) Use underscores for new methdo names in DrupalKernel if possible

Rerolled - needed to change the GetFilenameTest:: containerBuild() to GetFilenameTest::register() to work in the new phpunit KernelTestBase environment.

Nits.

1. +++ b/core/lib/Drupal/Core/DrupalKernel.php
   @@ -1508,4 +1511,61 @@ protected function addServiceFiles(array $service_yamls) {
   +    return $install_profile;
   +
   +  }
   

   Extra empty line.

2. +++ b/core/lib/Drupal/Core/DrupalKernel.php
   @@ -1508,4 +1511,61 @@ protected function addServiceFiles(array $service_yamls) {
   +   * Scans the filesystem looking for all installation profiles.
   +   * Returns the one that has a 'distribution' entry in its info.yml
   +   * file.  If multiple  profiles are distributions, an exception will
   +   * be thrown.
   +   *
   +   * This is used in two places:
   +   *  1) During installation, if there is a single distribution, then
   +   *     the installer will not write the installation profile name
   +   *     to settings.php.
   +   *  2) Whenever DrupalKernel::getInstallProfile() is called, if there
   +   *     is no installation profile name noted in settings.php, then
   +   *     it will call this function to determine the distribution
   +   *     to use.
   

   The word "Returns" should continue on the first line. Extra space preceding "If multiple". More lines are wrapped too early.

3. +++ b/core/lib/Drupal/Core/DrupalKernel.php
   @@ -1508,4 +1511,61 @@ protected function addServiceFiles(array $service_yamls) {
   +    $listing->setProfileDirectories(array());
   
   +++ b/core/modules/system/src/Tests/Installer/DistributionProfileExistingSettingsTest.php
   @@ -0,0 +1,160 @@
   +    $this->info = array(
   
   +++ b/core/modules/system/src/Tests/Installer/InstallerExistingSettingsMismatchProfileTest.php
   @@ -39,6 +39,13 @@ protected function setUp() {
   +    $this->settings['settings']['install_profile'] = (object) array(
   
   +++ b/core/modules/system/src/Tests/Installer/MultipleDistributionsProfileTest.php
   @@ -0,0 +1,106 @@
   +      $info = array(
   ...
   +        'distribution' => array(
   ...
   +          'install' => array(
   

   Array short syntax?

Updated patch with the nits from #143 addressed.

In addition to the passing tests, I can give a +1 to this from my anecdotal in real world testing...

We've been using a backported version of the patch in #142 to install Lightning in an environment with a write protected settings file (and the config_sync/database values in an include file).

1. +++ b/core/lib/Drupal/Core/Config/ConfigInstaller.php
   @@ -632,20 +640,6 @@ protected function drupalGetPath($type, $name) {
   -  protected function drupalGetProfile() {
   -    // Settings is safe to use because settings.php is written before any module
   -    // is installed.
   -    return Settings::get('install_profile');
   -  }
   

   For less BC break we could return $this->installProfile; That seems to be an easy win.

2. +++ b/core/modules/system/src/Tests/Installer/InstallerExistingSettingsNoProfileTest.php
   @@ -65,7 +64,7 @@ protected function setUpSettings() {
   -    $this->assertEqual('testing', Settings::get('install_profile'));
   +    $this->assertEqual('testing', \Drupal::installProfile());
   
   +++ b/core/modules/system/src/Tests/Installer/InstallerExistingSettingsTest.php
   @@ -75,6 +76,7 @@ public function testInstaller() {
        $this->assertEqual('testing', drupal_get_profile(), 'Profile was changed from minimal to testing during interactive install.');
   +    $this->assertEqual('testing', Settings::get('install_profile'), 'Profile was correctly changed to testing in Settings.php');
   

   Should we use \Drupal::installProfile() in both places?

3. +++ b/core/modules/system/src/Tests/Installer/MultipleDistributionsProfileTest.php
   @@ -0,0 +1,106 @@
   +
   +  protected function setUp() {
   ...
   +  /**
   +   * Overrides InstallerTest::setUpLanguage().
   +   */
   ...
   +  /**
   +   * Overrides InstallerTest::setUpProfile().
   +   */
   

   Nitpick: Let's add {@inheritdoc}

Fixed all of @dawehner's comments in #146 except for #1, which seems to refer to code that no longer exists.

except for #1, which seems to refer to code that no longer exists.

Exactly that's my point. We are removing a function we don't actually have to remove.

+++ b/core/lib/Drupal/Core/Config/ConfigInstaller.php
@@ -585,7 +593,7 @@ protected function getEnabledExtensions() {
    */
   protected function getProfileStorages($installing_name = '') {
-    $profile = $this->drupalGetProfile();
+    $profile = $this->installProfile;
     $profile_storages = [];
     if ($profile && $profile != $installing_name) {
       $profile_path = $this->drupalGetPath('module', $profile);
@@ -632,20 +640,6 @@ protected function drupalGetPath($type, $name) {

@@ -632,20 +640,6 @@ protected function drupalGetPath($type, $name) {
   }
 
   /**
-   * Gets the install profile from settings.
-   *
-   * @return string|null $profile
-   *   The name of the installation profile or NULL if no installation profile
-   *   is currently active. This is the case for example during the first steps
-   *   of the installer or during unit tests.
-   */
-  protected function drupalGetProfile() {
-    // Settings is safe to use because settings.php is written before any module
-    // is installed.
-    return Settings::get('install_profile');
-  }
-
-  /**
    * Wrapper for drupal_installation_attempted().

This is the code we remove.

I think this patch, in its current form, has a major flaw: ExtensionDiscovery and its call to drupal_get_profile() starts to rely on a working container.
In the case of a working Drupal this is not much of a deal, but in a world of where the cached container is gone, this could become a problem.

In order to solve that problem, here is a possible solution (as discussed with alexpott and mpotter):

• On install time, write the current profile into core.extension
• On container build time, get this information into the container from the configuration
• This allows us to not retrieve the information statically from the container on runtime
• In case the container goes away, we can still fallback to the container, similar to the way how we fallback to the list of active modules from core.extension

An alternative approach could be to write the installation profile into a services.yml file on install time and then read from that, in case its needed.

Here is a patch on top of addressing the review in #148

Fixing #150 and re-rolling.

Oopsie... a better patch (against the right branch) and an upgrade path

+++ b/core/lib/Drupal/Core/DrupalKernel.php
@@ -1503,4 +1506,63 @@ protected function addServiceFiles(array $service_yamls) {
+  protected function getDistribution() {

I think if we write the profile to core.extension then we shouldn't add this to the kernel since it just isn't necessary.

Interesting...

I'm not sure what is going on with the BrowserTestBase failure. While these tests fail for me locally, they still return a proper response.
This fixes the other 2 failing tests.

1. +++ b/core/includes/install.core.inc
   @@ -2170,13 +2174,34 @@ function install_display_requirements($install_state, $requirements) {
   -  if (Settings::get('install_profile') !== $install_state['parameters']['profile']) {
   +  $settings_value = Settings::get('install_profile');
   +  // We need to write to settings.php if the value in settings.php does not
   +  // equal the selected profile.
   +  $need_to_write = $settings_value !== $install_state['parameters']['profile'];
   +  // However, if we're dealing with a distribution and the profile is not
   +  // writable do not write the value to settings.php if the current value is not
   +  // set.
   +  $distribution = FALSE;
   +  try {
   +    $distribution = \Drupal::service('kernel')->getDistribution();
   +  }
   +  catch (TooManyDistributionsException $e) {
   +    // The user will have chosen.
   +  }
   +
   +  if ($settings_value == '' && $distribution && !is_writable(\Drupal::service('site.path') . '/settings.php')) {
   +    $need_to_write = FALSE;
   +  }
   +
   +  if ($need_to_write) {
        // Remember the profile which was used.
        $settings['settings']['install_profile'] = (object) array(
          'value' => $install_state['parameters']['profile'],
   diff --git a/core/includes/install.inc b/core/includes/install.inc
   

   Shouldn't this function now also write to config, just from an API point of view? The name of the function doesn't point out that its just about settings.php to be honest.

2. +++ b/core/lib/Drupal/Core/DrupalKernel.php
   @@ -1503,4 +1506,63 @@ protected function addServiceFiles(array $service_yamls) {
   +    $install_profile = Settings::get('install_profile');
   +
   +    if (empty($install_profile) && ($config = $this->getConfigStorage()->read('core.extension')) && isset($config['profile'])) {
   +      $install_profile = $config['profile'];
   +    }
   +    elseif (empty($install_profile)) {
   +      $install_profile = $this->getDistribution();
   +    }
   

   I would try to reorganize this code to make it clear, that Settings::get('install_profile') is just a fallback and the config storage is the primary source of truth.

3. +++ b/core/modules/system/src/Tests/Installer/DistributionProfileTest.php
   @@ -68,6 +69,10 @@ public function testInstalled() {
   +    // Confirm that Drupal recognizes this distribution as the current profile.
   +    $this->assertEqual(\Drupal::installProfile(), 'mydistro');
   +    $this->assertEqual(Settings::get('install_profile'), 'mydistro', 'The install profile has been written to settings.php.');
   

   IMHO we should check also the config itself

Thanks for the review @dawehner
1. Deprecated in the current patch
2. Completely rewritten
3. Agreed

The current patch goes further and deprecates the install_profile setting. It also removes the getDistribution code because it just is not necessary anymore.