Problem/Motivation

The user module is installed by install_base_system() but it is also in the list of profiles to be installed by install_profile_modules(). System module is removed from this list by install_base_system() but user is not. However the system module removal doesn't actually work.

So both System and User are passed twice to \Drupal\Core\Extension\ModuleInstaller::install() during a Drupal install. This doesn't have much impact because the second time we ignore the requests to install but it is still confusing when debugging and doing unnecessary work.

Proposed resolution

  • The removal of the system module in install_base_system() does not work because System module is required by other modules (including User) so it is added back in.
  • The use of state to track the list of modules is completely unnecessary we already have this information in the $install_state global and this information is maintained already through out the install process
  • system and user modules should be removed in install_profile_modules()

Do less things during the installation process for the win.

#3 tried to remove the special case for the User module but this ending up breaking:

  • Installation from configuration for DrupalCI (couldn't reproduce this fail locally)
  • Changing the language that user 0 and user 1 are created in - this is probably a bug in HEAD but changing this behaviour shouldn't happen here.

Remaining tasks

User interface changes

None

API changes

None

Data model changes

None

Release notes snippet

N/a

CommentFileSizeAuthor
#14 3112790-9.1.x-14.patch2.06 KBalexpott
#7 3112790-7.patch2.06 KBalexpott
#7 3-7-interdiff.txt1.25 KBalexpott
#3 3112790-3.patch1.89 KBalexpott
#3 2-3-interdiff.txt1.72 KBalexpott
#2 3112790-2.patch781 bytesalexpott
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexpott created an issue. See original summary.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Thunder commented
Issue summary: View changes
Status: Active » Needs review
FileSize
3112790-2.patch781 bytes

Here it goes for option 1.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Thunder commented
Title: User module is installed twice » System and User modules are installed twice
Issue summary: View changes
FileSize
2-3-interdiff.txt1.72 KB
3112790-3.patch1.89 KB

Arrghhh... the code in HEAD to exclude the system module from being installed twice is also not working!

The last submitted patch, 2: 3112790-2.patch, failed testing. View results

Status: Needs review » Needs work

The last submitted patch, 3: 3112790-3.patch, failed testing. View results

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Thunder commented

So the patches in #2 and #3 have an interesting affect. It means that when the anonymous and admin user are created they are done so in the language selected by the installer. I think this might make sense but it is a bigger change than I anticipated. So I think first up here we should make a more limited change and file a follow-up to discuss the user creation and in what language issue.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Thunder commented
Issue summary: View changes
Status: Needs work » Needs review
FileSize
3-7-interdiff.txt1.25 KB
3112790-7.patch2.06 KB

Here's a more minimal patch.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented

Interesting, ++ to get follow-up because at least drush allows to install without EN language

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Thunder commented

Created #3112866: Remove special case of User module install as a follow-up

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented
Status: Needs review » Reviewed & tested by the community

Patch and explanation makes sense, this is a small but useful cleanup.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented 
+++ b/core/includes/install.core.inc
@@ -1113,15 +1113,10 @@ function install_base_system(&$install_state) {
-  // Enable the user module so that sessions can be recorded during the
-  // upcoming bootstrap step.
+  // Install the User module so that installing from configuration works and to
+  // ensure the anonymous user is created with a langcode of 'en'.

btw now comment has different meaning

It was like session service is not enough for next bootstrap, and the user entity should be created before \Drupal\user\Authentication\Provider\Cookie::getUserFromSession() can use it

but that is not true for installer kernel, so follow-up is more about why user required in install time

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

Needs a re-roll.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Thunder commented
Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs reroll
FileSize
3112790-9.1.x-14.patch2.06 KB

Rerolled.

  • catch committed 4e62462 on 9.1.x 
    Issue #3112790 by alexpott, andypost, longwave: System and User modules...
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Fixed

Committed 4e62462 and pushed to 9.1.x. Thanks!

Status: Fixed » Closed (fixed)

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