This is a child of #1775842: [meta] Convert all variables to state and/or config systems

In Drupal 7 install_profile was a variable - the magic of variable get working without a database connection protected us during the early install.

For Drupal 8 we need to do something different.

The patch attached in #61 converts the variable to a setting in settings.php. This is because:

  • this setting changes what modules are available to be installed
  • the only UI to select it is in the installer before settings.php has been made read only
  • there is no UI to change it
  • have different values in different environments makes no sense and it never changes at runtime
CommentFileSizeAuthor
#65 61-65-interdiff.txt4.77 KBalexpott
#65 drupal-1827112-65.patch10.3 KBalexpott
#61 59-61-interdiff.txt680 bytesalexpott
#61 drupal-1827112-61.patch10.32 KBalexpott
#59 57-59-interdiff.txt553 bytesalexpott
#59 drupal-1827112-59.patch10.01 KBalexpott
#57 54-57-interdiff.txt2.7 KBalexpott
#57 drupal-1827112-57.patch9.27 KBalexpott
#54 52-54-interdiff.txt3.26 KBalexpott
#54 drupal-1827112-54.patch6.81 KBalexpott
#52 drupal-1827112-52.patch6.36 KBalexpott
#50 drupal-1827112-50.patch6.16 KBalexpott
#46 drupal-1827112-46.patch4.52 KBalexpott
#44 42-44-interdiff.txt490 bytesalexpott
#44 drupal-1827112-44.patch4.5 KBalexpott
#42 40-42-interdiff.txt1.45 KBalexpott
#42 drupal-1827112-42.patch4.53 KBalexpott
#40 drupal-1827112-40.patch3.15 KBdawehner
#36 drupal-1827112-36.patch4.73 KBdawehner
#36 interdiff.txt4.73 KBdawehner
#33 1827112-convert-install_profile-variable-CMI-33.patch4.59 KBvijaycs85
#31 interdiff.txt1.96 KBdawehner
#31 drupal-1827112-31.patch4.59 KBdawehner
#30 drupal-1827112-30.patch3.04 KBdawehner
#28 1827112-convert-install_profile-variable-CMI-28.patch1.93 KBvijaycs85
#25 1827112-convert-install_profile-variable-CMI-25.patch1.92 KBvijaycs85
#20 1827112-convert-install_profile-variable-CMI-20.patch1.53 KBvijaycs85
#17 1827112-convert-install_profile-variable-CMI-17.patch1.84 KBvijaycs85
#15 1827112-convert-install_profile-variable-CMI-15.patch1.84 KBvijaycs85
#10 1827112-convert-install_profile-variable-CMI-10.patch1.9 KBcspitzlay
#3 1827112-convert-install_profile-variable-CMI-3.patch1.9 KBcspitzlay
#1 1827112-convert-install_profile-variable-CMI.patch747 bytesCameron Tod
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Cameron Tod’s picture

Status: Active » Needs review
FileSize
747 bytes

Status: Needs review » Needs work

The last submitted patch, 1827112-convert-install_profile-variable-CMI.patch, failed testing.

cspitzlay’s picture

I replaced a remaining variable_set and added an update function.

cspitzlay’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 1827112-convert-install_profile-variable-CMI-3.patch, failed testing.

cspitzlay’s picture

The test failure is triggered by the update function included in the patch.
But, IMHO, the bug is in update_variables_to_config.

I opened #1830424: update_variables_to_config loses data with a test.

cspitzlay’s picture

Status: Needs work » Postponed
cspitzlay’s picture

Status: Postponed » Needs review
Issue tags: -Configuration system

Status: Needs review » Needs work
Issue tags: +Configuration system

The last submitted patch, 1827112-convert-install_profile-variable-CMI-3.patch, failed testing.

cspitzlay’s picture

Status: Needs work » Needs review
FileSize
1.9 KB

updated the "N" in hook_update_N()

Status: Needs review » Needs work

The last submitted patch, 1827112-convert-install_profile-variable-CMI-10.patch, failed testing.

cspitzlay’s picture

Hu? Failed but 0 fails? What's the matter here?
Let's see if this is reproducible ...

cspitzlay’s picture

Status: Needs work » Needs review
Issue tags: -Configuration system

Status: Needs review » Needs work
Issue tags: +Configuration system

The last submitted patch, 1827112-convert-install_profile-variable-CMI-10.patch, failed testing.

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
1.84 KB

Re-rolling without config save.

Status: Needs review » Needs work

The last submitted patch, 1827112-convert-install_profile-variable-CMI-15.patch, failed testing.

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
1.84 KB

Fixing function name error.

Cameron Tod’s picture

Assigned: Cameron Tod » Unassigned

Status: Needs review » Needs work

The last submitted patch, 1827112-convert-install_profile-variable-CMI-17.patch, failed testing.

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
1.53 KB

Status: Needs review » Needs work

The last submitted patch, 1827112-convert-install_profile-variable-CMI-20.patch, failed testing.

cspitzlay’s picture

Hi vijaycs85,

why is saving the config value no longer needed?
How will the profile be stored if a fresh installation with a profile != standard is made?

vijaycs85’s picture

Thought of variable_set doesn't need for config system as it is in yml.

cspitzlay’s picture

Well, the value in this yml file is just a default.

But that's not the only possible value (that's why this variable exists in the first place :) ).
The install_finished function stores away the actually used install profile.

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
1.92 KB

Re-rolling with saving profile and name change from install_profile to profile.install as simpletest look green locally.

Status: Needs review » Needs work

The last submitted patch, 1827112-convert-install_profile-variable-CMI-25.patch, failed testing.

vijaycs85’s picture

Getting below exception:

Symfony\Component\DependencyInjection\Exception\InvalidArgumentException: The service definition "config.factory" does not exist. in Symfony\Component\DependencyInjection\ContainerBuilder->getDefinition() 
(line 690 of /var/lib/drupaltestbot/sites/default/files/checkout/core/vendor/symfony/dependency-injection/Symfony/Component/DependencyInjection/ContainerBuilder.php).

">Symfony\Component\DependencyInjection\ContainerBuilder->getDefinition('config.factory')
Symfony\Component\DependencyInjection\ContainerBuilder->get('config.factory')
config('system.site')
drupal_get_profile()
Drupal\Core\SystemListingInfo->profiles('scripts')
Drupal\Core\SystemListing->scan('/^[a-zA-Z_\x7f-\xff][a-zA-Z0-9_\x7f-\xff]*\.script$/', 'scripts', 'name', 1)
drupal_system_listing('/^[a-zA-Z_\x7f-\xff][a-zA-Z0-9_\x7f-\xff]*\.script$/', 'scripts')
drupal_get_filename('script', 'test')
Drupal\system\Tests\Bootstrap\GetFilenameUnitTest->testDrupalGetFilename()
Drupal\simpletest\TestBase->run()
simpletest_script_run_one_test('39', 'Drupal\system\Tests\Bootstrap\GetFilenameUnitTest')

Might be related to http://drupal.org/node/1821312.

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
1.93 KB

Re-rolling with update_N change to check #27 problem still persist.

Status: Needs review » Needs work

The last submitted patch, 1827112-convert-install_profile-variable-CMI-28.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
3.04 KB

Added an upgrade path, sadly this patch still breaks.

The general problem is that drupal_system_listing() is called and needed to construct a container,
so the call to drupal_get_profile() in SystemListingInfo::profiles() fail.

dawehner’s picture

FileSize
4.59 KB
1.96 KB

Let's add an empty config factory into the container.

Status: Needs review » Needs work

The last submitted patch, drupal-1827112-31.patch, failed testing.

vijaycs85’s picture

vijaycs85’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 1827112-convert-install_profile-variable-CMI-33.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
4.73 KB
4.73 KB

The problem is that this upgrade function jumps in way too late, drupal_get_profile() is used really early during the upgrade path, so let's fix that in update_prepare_d8_bootstrap().

Status: Needs review » Needs work

The last submitted patch, drupal-1827112-36.patch, failed testing.

dawehner’s picture

+++ b/core/modules/system/config/system.site.ymlundefined
@@ -7,3 +7,5 @@ page:
+profile:
+  install: standard

I'm wondering whether we should use minimal here instead?

catch’s picture

Is this really configuration? It tracks the originally installed install profile, and can never be updated (unless you hack it). Seems more like state.

dawehner’s picture

Status: Needs work » Needs review
FileSize
3.15 KB

If you look at other similar stuff this really makes sense. There are examples like the state,
which you will never change.

Status: Needs review » Needs work

The last submitted patch, drupal-1827112-40.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
4.53 KB
1.45 KB

So by changing variable_get (which has no dependencies other than a global $conf) to state we introduce some problems for the installer...

Status: Needs review » Needs work

The last submitted patch, drupal-1827112-42.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
4.5 KB
490 bytes

Ok I should know this already...

dawehner’s picture

Oh gosh, all these globals. Your recent interdiffs are looking great.

alexpott’s picture

FileSize
4.52 KB

Rerolled

moshe weitzman’s picture

catch rightly points out that install_profile never changes. I would think then that it belongs in config, and not state. Not a big deal though.

olli’s picture

Is there a reason why this can't be a setting?

@@ -26,6 +26,11 @@ public static function getInfo() {
+    // drupal_get_profile() is using obtaining the profile from state if the

Maybe just "is obtaining"?

alexpott’s picture

Status: Needs review » Needs work

I agree a setting makes a great deal of sense actually as this is used to change what code is included in you site. Having it in settings.php means that it has very few dependencies - which is a good thing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
6.16 KB

Had a little bit of fun with Simpletest but here's the install_profile variable moved in to Settings.

Status: Needs review » Needs work

The last submitted patch, drupal-1827112-50.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
6.36 KB

Lets try writing to settings.php earlier in the installer.

Status: Needs review » Needs work

The last submitted patch, drupal-1827112-52.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
6.81 KB
3.26 KB

How about this?

catch’s picture

Of all the options, $settings feels like the best. It's weird that we install a module then base the existence of other modules on that module, but that's a holdover from when install profiles became modules and the introduction of install profiles in general, and not something we can sort out here.

Status: Needs review » Needs work

The last submitted patch, drupal-1827112-54.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
9.27 KB
2.7 KB

I love simpletest

Status: Needs review » Needs work

The last submitted patch, drupal-1827112-57.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
10.01 KB
553 bytes

Drupal\system\Tests\Upgrade\BareMinimalNoConfigUpgradePathTest passes locally :(

No idea why it's failing. Still loving simpletest and pift/pifr :)

Status: Needs review » Needs work

The last submitted patch, drupal-1827112-59.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
10.32 KB
680 bytes

Maybe this will work?

tstoeckler’s picture

Title: Convert 'install_profile' variable to Configuration system » Convert 'install_profile' variable to Settings

Re-titling per the direction in the latest patches.

alexpott’s picture

#61: drupal-1827112-61.patch queued for re-testing.

dawehner’s picture

+++ b/core/includes/install.core.inc
@@ -1234,6 +1234,14 @@ function install_settings_form_submit($form, &$form_state) {
+      'value'    => $install_state['parameters']['profile'],
+      'required' => TRUE,

+++ b/core/includes/update.inc
@@ -439,6 +439,23 @@ function update_prepare_d8_bootstrap() {
+            'value'    => $old_variable,
+            'required' => TRUE,

+++ b/core/modules/simpletest/lib/Drupal/simpletest/WebTestBase.php
@@ -779,6 +779,22 @@ protected function setUp() {
+          'value'    => $this->profile,
+          'required' => TRUE,

+++ b/core/modules/system/lib/Drupal/system/Tests/Upgrade/BareMinimalAnonymousUpgradePathTest.php
@@ -28,9 +28,15 @@ public function setUp() {
+        'value'    => $this->profile,
+        'required' => TRUE,

+++ b/core/modules/system/lib/Drupal/system/Tests/Upgrade/BareMinimalNoConfigUpgradePathTest.php
@@ -35,6 +40,12 @@ public function setUp() {
+        'value'    => $this->profile,
+        'required' => TRUE,

+++ b/core/modules/system/lib/Drupal/system/Tests/Upgrade/UpgradePathTestBase.php
@@ -146,6 +146,21 @@ protected function setUp() {
+          'value'    => $install_profile,
+          'required' => TRUE,
+        ),

I have never seen an alignment of the arrow in proper Drupal core code, or in other words, that just looks odd.

+++ b/core/includes/common.inc
@@ -269,11 +270,17 @@ function drupal_get_region_content($region = NULL, $delimiter = ' ') {
+    $profile = Settings()->get('install_profile') ?: 'standard';

Let's keep it a lowercase settings() to be safe if PHP just decides to not be case-insensitive.

alexpott’s picture

Thanks dawehner - changes made

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Thank you

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 8.x. Thanks.

tstoeckler’s picture

+++ b/core/includes/common.inc
@@ -269,11 +270,17 @@ function drupal_get_region_content($region = NULL, $delimiter = ' ') {
 function drupal_get_profile() {
...
-  if (isset($install_state['parameters']['profile'])) {
-    $profile = $install_state['parameters']['profile'];
+  if (drupal_installation_attempted()) {
+    // If the profile has been selected return it.
+    if (isset($install_state['parameters']['profile'])) {
+      $profile = $install_state['parameters']['profile'];
+    }
+    else {
+      $profile = '';
+    }
   }

I think this change to drupal_get_profile() is great in principal but I think returning NULL instead of an empty string would make more sense, as at this point there simply is no profile.

This wasn't discussed anywhere above (unless I missed it) so I thought I'd ask for some feedback before opening a follow-up for that.

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

Anonymous’s picture

Issue summary: View changes

Updating summary

sun’s picture

Unfortunately, the changes to the installer weren't sufficient here.

If there is an existing settings.php already, and if both $databases + $config_directories are set up correctly, then settings.php will not be touched or rewritten in any way by the installer.

As a consequence, if you re-install your Drupal site, the resulting settings.php will not contain the install_profile setting. On /admin/modules, you get the following error message:

Notice: Undefined index: distribution_name in drupal_install_profile_distribution_name() (line 106 of core\includes\install.inc).

Likewise, installation of Drupal halts with a WSOD in the installer, if you choose any other installation profile than the Standard profile.

Created follow-up issue for that: #2156401: Write install_profile value to configuration and only to settings.php if it is writeable