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
Files: 
CommentFileSizeAuthor
#65 61-65-interdiff.txt4.77 KBalexpott
#65 drupal-1827112-65.patch10.3 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 58,398 pass(es).
[ View ]
#61 59-61-interdiff.txt680 bytesalexpott
#61 drupal-1827112-61.patch10.32 KBalexpott
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#59 57-59-interdiff.txt553 bytesalexpott
#59 drupal-1827112-59.patch10.01 KBalexpott
FAILED: [[SimpleTest]]: [MySQL] 58,118 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#57 54-57-interdiff.txt2.7 KBalexpott
#57 drupal-1827112-57.patch9.27 KBalexpott
FAILED: [[SimpleTest]]: [MySQL] 58,128 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#54 52-54-interdiff.txt3.26 KBalexpott
#54 drupal-1827112-54.patch6.81 KBalexpott
FAILED: [[SimpleTest]]: [MySQL] 58,083 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
#52 drupal-1827112-52.patch6.36 KBalexpott
FAILED: [[SimpleTest]]: [MySQL] 57,840 pass(es), 68 fail(s), and 1,509 exception(s).
[ View ]
#50 drupal-1827112-50.patch6.16 KBalexpott
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#46 drupal-1827112-46.patch4.52 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 57,914 pass(es).
[ View ]
#44 42-44-interdiff.txt490 bytesalexpott
#44 drupal-1827112-44.patch4.5 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 57,030 pass(es).
[ View ]
#42 40-42-interdiff.txt1.45 KBalexpott
#42 drupal-1827112-42.patch4.53 KBalexpott
FAILED: [[SimpleTest]]: [MySQL] 13,340 pass(es), 597 fail(s), and 1 exception(s).
[ View ]
#40 drupal-1827112-40.patch3.15 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 56,991 pass(es), 8 fail(s), and 2 exception(s).
[ View ]
#36 drupal-1827112-36.patch4.73 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 52,236 pass(es), 0 fail(s), and 17 exception(s).
[ View ]
#36 interdiff.txt4.73 KBdawehner
#33 1827112-convert-install_profile-variable-CMI-33.patch4.59 KBvijaycs85
FAILED: [[SimpleTest]]: [MySQL] 52,175 pass(es), 1 fail(s), and 2,262 exception(s).
[ View ]
#31 interdiff.txt1.96 KBdawehner
#31 drupal-1827112-31.patch4.59 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 50,141 pass(es), 1 fail(s), and 1,707 exception(s).
[ View ]
#30 drupal-1827112-30.patch3.04 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 50,087 pass(es), 3 fail(s), and 1,674 exception(s).
[ View ]
#28 1827112-convert-install_profile-variable-CMI-28.patch1.93 KBvijaycs85
FAILED: [[SimpleTest]]: [MySQL] 49,616 pass(es), 1 fail(s), and 1,504 exception(s).
[ View ]
#25 1827112-convert-install_profile-variable-CMI-25.patch1.92 KBvijaycs85
FAILED: [[SimpleTest]]: [MySQL] 49,355 pass(es), 0 fail(s), and 1,504 exception(s).
[ View ]
#20 1827112-convert-install_profile-variable-CMI-20.patch1.53 KBvijaycs85
FAILED: [[SimpleTest]]: [MySQL] 45,633 pass(es), 25 fail(s), and 67,890 exception(s).
[ View ]
#17 1827112-convert-install_profile-variable-CMI-17.patch1.84 KBvijaycs85
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/system/system.install.
[ View ]
#15 1827112-convert-install_profile-variable-CMI-15.patch1.84 KBvijaycs85
FAILED: [[SimpleTest]]: [MySQL] Setup environment: failed to clear checkout directory.
[ View ]
#10 1827112-convert-install_profile-variable-CMI-10.patch1.9 KBcspitzlay
FAILED: [[SimpleTest]]: [MySQL] 48,182 pass(es), 0 fail(s), and 1,417 exception(s).
[ View ]
#3 1827112-convert-install_profile-variable-CMI-3.patch1.9 KBcspitzlay
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/system/system.install.
[ View ]
#1 1827112-convert-install_profile-variable-CMI.patch747 bytescam8001
FAILED: [[SimpleTest]]: [MySQL] 44,605 pass(es), 31 fail(s), and 12,805 exception(s).
[ View ]

Comments

cam8001’s picture

Status:Active» Needs review
StatusFileSize
new747 bytes
FAILED: [[SimpleTest]]: [MySQL] 44,605 pass(es), 31 fail(s), and 12,805 exception(s).
[ View ]

Status:Needs review» Needs work

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

cspitzlay’s picture

StatusFileSize
new1.9 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/system/system.install.
[ View ]

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
StatusFileSize
new1.9 KB
FAILED: [[SimpleTest]]: [MySQL] 48,182 pass(es), 0 fail(s), and 1,417 exception(s).
[ View ]

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
StatusFileSize
new1.84 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: failed to clear checkout directory.
[ View ]

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
StatusFileSize
new1.84 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/system/system.install.
[ View ]

Fixing function name error.

cam8001’s picture

Assigned:cam8001» 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
StatusFileSize
new1.53 KB
FAILED: [[SimpleTest]]: [MySQL] 45,633 pass(es), 25 fail(s), and 67,890 exception(s).
[ View ]

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
StatusFileSize
new1.92 KB
FAILED: [[SimpleTest]]: [MySQL] 49,355 pass(es), 0 fail(s), and 1,504 exception(s).
[ View ]

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
StatusFileSize
new1.93 KB
FAILED: [[SimpleTest]]: [MySQL] 49,616 pass(es), 1 fail(s), and 1,504 exception(s).
[ View ]

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
StatusFileSize
new3.04 KB
FAILED: [[SimpleTest]]: [MySQL] 50,087 pass(es), 3 fail(s), and 1,674 exception(s).
[ View ]

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

StatusFileSize
new4.59 KB
FAILED: [[SimpleTest]]: [MySQL] 50,141 pass(es), 1 fail(s), and 1,707 exception(s).
[ View ]
new1.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

StatusFileSize
new4.59 KB
FAILED: [[SimpleTest]]: [MySQL] 52,175 pass(es), 1 fail(s), and 2,262 exception(s).
[ View ]

Re-rolling...

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
StatusFileSize
new4.73 KB
new4.73 KB
FAILED: [[SimpleTest]]: [MySQL] 52,236 pass(es), 0 fail(s), and 17 exception(s).
[ View ]

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
StatusFileSize
new3.15 KB
FAILED: [[SimpleTest]]: [MySQL] 56,991 pass(es), 8 fail(s), and 2 exception(s).
[ View ]

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
StatusFileSize
new4.53 KB
FAILED: [[SimpleTest]]: [MySQL] 13,340 pass(es), 597 fail(s), and 1 exception(s).
[ View ]
new1.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
StatusFileSize
new4.5 KB
PASSED: [[SimpleTest]]: [MySQL] 57,030 pass(es).
[ View ]
new490 bytes

Ok I should know this already...

dawehner’s picture

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

alexpott’s picture

StatusFileSize
new4.52 KB
PASSED: [[SimpleTest]]: [MySQL] 57,914 pass(es).
[ View ]

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
StatusFileSize
new6.16 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

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
StatusFileSize
new6.36 KB
FAILED: [[SimpleTest]]: [MySQL] 57,840 pass(es), 68 fail(s), and 1,509 exception(s).
[ View ]

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
StatusFileSize
new6.81 KB
FAILED: [[SimpleTest]]: [MySQL] 58,083 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
new3.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
StatusFileSize
new9.27 KB
FAILED: [[SimpleTest]]: [MySQL] 58,128 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
new2.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
StatusFileSize
new10.01 KB
FAILED: [[SimpleTest]]: [MySQL] 58,118 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
new553 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
StatusFileSize
new10.32 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
new680 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

StatusFileSize
new10.3 KB
PASSED: [[SimpleTest]]: [MySQL] 58,398 pass(es).
[ View ]
new4.77 KB

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: Avoid writing install_profile value to settings.php when provided by a Distribution