Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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
Comment | File | Size | Author |
---|---|---|---|
#65 | 61-65-interdiff.txt | 4.77 KB | alexpott |
#65 | drupal-1827112-65.patch | 10.3 KB | alexpott |
#61 | 59-61-interdiff.txt | 680 bytes | alexpott |
#61 | drupal-1827112-61.patch | 10.32 KB | alexpott |
#59 | 57-59-interdiff.txt | 553 bytes | alexpott |
Comments
Comment #1
Cameron Tod CreditAttribution: Cameron Tod commentedComment #3
cspitzlayI replaced a remaining variable_set and added an update function.
Comment #4
cspitzlayComment #6
cspitzlayThe 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.
Comment #7
cspitzlayOk, here's already some work regarding this:
#1825466: Allow NestedArray::mergeDeepArray() to preserve integer keys
(I closed #1830424: update_variables_to_config loses data as duplicate)
Comment #8
cspitzlay#3: 1827112-convert-install_profile-variable-CMI-3.patch queued for re-testing.
Comment #10
cspitzlayupdated the "N" in hook_update_N()
Comment #12
cspitzlayHu? Failed but 0 fails? What's the matter here?
Let's see if this is reproducible ...
Comment #13
cspitzlay#10: 1827112-convert-install_profile-variable-CMI-10.patch queued for re-testing.
Comment #15
vijaycs85Re-rolling without config save.
Comment #17
vijaycs85Fixing function name error.
Comment #18
Cameron Tod CreditAttribution: Cameron Tod commentedComment #20
vijaycs85Comment #22
cspitzlayHi 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?
Comment #23
vijaycs85Thought of variable_set doesn't need for config system as it is in yml.
Comment #24
cspitzlayWell, 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.
Comment #25
vijaycs85Re-rolling with saving profile and name change from install_profile to profile.install as simpletest look green locally.
Comment #27
vijaycs85Getting below exception:
Might be related to http://drupal.org/node/1821312.
Comment #28
vijaycs85Re-rolling with update_N change to check #27 problem still persist.
Comment #30
dawehnerAdded 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.
Comment #31
dawehnerLet's add an empty config factory into the container.
Comment #33
vijaycs85Re-rolling...
Comment #34
vijaycs85Comment #36
dawehnerThe 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().
Comment #38
dawehnerI'm wondering whether we should use minimal here instead?
Comment #39
catchIs this really configuration? It tracks the originally installed install profile, and can never be updated (unless you hack it). Seems more like state.
Comment #40
dawehnerIf you look at other similar stuff this really makes sense. There are examples like the state,
which you will never change.
Comment #42
alexpottSo by changing variable_get (which has no dependencies other than a global $conf) to state we introduce some problems for the installer...
Comment #44
alexpottOk I should know this already...
Comment #45
dawehnerOh gosh, all these globals. Your recent interdiffs are looking great.
Comment #46
alexpottRerolled
Comment #47
moshe weitzman CreditAttribution: moshe weitzman commentedcatch 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.
Comment #48
olli CreditAttribution: olli commentedIs there a reason why this can't be a setting?
Maybe just "is obtaining"?
Comment #49
alexpottI 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.
Comment #50
alexpottHad a little bit of fun with Simpletest but here's the install_profile variable moved in to Settings.
Comment #52
alexpottLets try writing to settings.php earlier in the installer.
Comment #54
alexpottHow about this?
Comment #55
catchOf 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.Comment #57
alexpottI love simpletest
Comment #59
alexpottDrupal\system\Tests\Upgrade\BareMinimalNoConfigUpgradePathTest passes locally :(
No idea why it's failing. Still loving simpletest and pift/pifr :)
Comment #61
alexpottMaybe this will work?
Comment #62
tstoecklerRe-titling per the direction in the latest patches.
Comment #63
alexpott#61: drupal-1827112-61.patch queued for re-testing.
Comment #64
dawehnerI have never seen an alignment of the arrow in proper Drupal core code, or in other words, that just looks odd.
Let's keep it a lowercase settings() to be safe if PHP just decides to not be case-insensitive.
Comment #65
alexpottThanks dawehner - changes made
Comment #66
dawehnerThank you
Comment #67
Dries CreditAttribution: Dries commentedCommitted to 8.x. Thanks.
Comment #68
tstoecklerI 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.
Comment #69.0
(not verified) CreditAttribution: commentedUpdating summary
Comment #70
sunUnfortunately, 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:
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