Problem/Motivation

In #3112790-6: System and User modules are installed twice we tried to remove the user module installation from install_base_system() and let it be handled with the rest of the modules. This had two impacts:

  • 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.

Proposed resolution

Try remove it and fix the tests.

Remaining tasks

- detect why user module required at install time (probably session)
- remove user module install like #3112790-3: System and User modules are installed twice
- fix tests and document behavior of initial users' creation

User interface changes

None

API changes

TBD

Data model changes

TBD

Release notes snippet

TBD

CommentFileSizeAuthor
#23 3112866-23.patch3.2 KBandypost
#16 3112866-14.patch4.22 KBalexpott
#16 11-14-interdiff.txt771 bytesalexpott
#11 3112866-11.patch4.01 KBalexpott
#11 9-11-interdiff.txt1.83 KBalexpott
#9 3112866-9.patch2.76 KBalexpott
#9 7-9-interdiff.txt296 bytesalexpott
#7 3112866-7.patch1.44 KBalexpott

Comments

alexpott created an issue. See original summary.

xjm’s picture

xjm
she/her
Primary language English
commented
Version: 9.0.x-dev » 9.1.x-dev

This would be a minor-only change. Since 8.9.x and 9.0.x are now in beta, I'm moving this to 9.1.x. Thanks!

andypost’s picture

andypost
he/him
Primary language Russian
commented
Issue summary: View changes

The comment before the referenced issue was

Enable the user module so that sessions can be recorded during the
upcoming bootstrap step.

But the session storage not managed by user module
And I found no traces of usage \Drupal\user\Authentication\Provider\Cookie::getUserFromSession() at install

catch’s picture

catch
he/him
Primary language English
commented

The sessions table used to be created in user_schema() but that hasn't been the case during all of Drupal 8, would be great to remove this special-casing.

andypost’s picture

andypost
he/him
Primary language Russian
commented

@catch to you mean to move session schema from system_schema() to user's one? or maybe move it to \Drupal\Core\Session\SessionHandler as it already using it

catch’s picture

catch
he/him
Primary language English
commented

@andypost I mis-remembered where the table was defined (both now and previously), but yes we should move the table creation logic to \Drupal\Core\Session\SessionHandler and handle it the same way as the database cache backend does - I actually thought we had already done that but apparently not, and couldn't find an issue either.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Active » Needs review
StatusFileSize
new 3112866-7.patch1.44 KB

Let's see what breaks...

Status: Needs review » Needs work

The last submitted patch, 7: 3112866-7.patch, failed testing. View results

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Needs work » Needs review
StatusFileSize
new 7-9-interdiff.txt296 bytes
new 3112866-9.patch2.76 KB
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented 
+++ b/core/modules/file/file.info.yml
@@ -5,3 +5,4 @@ package: Field types
   - drupal:field
+  - drupal:user

So whilst this is true and results in a fix I'm not sure it is the correct fix. InstallerExistingConfigSyncDirectoryMultilingualTest fails in #7 because File is installed before the User module. So adding the dependency works but actually there are still modules like dynamic_page_cache that are still installed first. This is happening because \Drupal\Core\Config\ConfigImporter::createExtensionChangelist() is not taking the required-ness into account. It needs to do the same logic as install_profile_modules() with respect to installing required modules first.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
StatusFileSize
new 9-11-interdiff.txt1.83 KB
new 3112866-11.patch4.01 KB

Something like this.

andypost’s picture

andypost
he/him
Primary language Russian
commented

Awesome idea with #11 - would be great to delegate ordering to module installer to solve the same way

Status: Needs review » Needs work

The last submitted patch, 11: 3112866-11.patch, failed testing. View results

andypost’s picture

andypost
he/him
Primary language Russian
commented
Related issues: +#2345611: Load user entity in Cookie AuthenticationProvider instead of using manual queries

It also reminded me about user entity in session layer

andypost’s picture

andypost
he/him
Primary language Russian
commented

I can't find very old issue from early d8 days when user module used to unset required option in info.yml

EDIT something from "small core"

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Needs work » Needs review
StatusFileSize
new 11-14-interdiff.txt771 bytes
new 3112866-14.patch4.22 KB

Ah the old code removed modules that do not exist. We already have properly validation around that so we can ignore them here.

@andypost I don't think we can do that because the needs similar but not 100% the same. Like the code in install_profile_modules() adds in missing dependencies and we should not do that here. And we need to skip modules that don't exist in the ConfigImporter but that's a different concern in install_profile_modules().

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

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work
Issue tags: +Needs Review Queue Initiative, +Needs reroll

This issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request as a guide.

Tagging for a reroll.

andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Needs work » Needs review
Issue tags: -Needs reroll
StatusFileSize
new 3112866-23.patch3.2 KB

test already has the changes so re-roll required only for 2 files

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Thanks for the quick turnaround

Did a quick test of doing a fresh Drupal 10.1.x install and verifying that the user module installs and everything behaves as before.

  • catch committed 9dc5c66e on 10.1.x 
    Issue #3112866 by alexpott, andypost, smustgrave: Remove special case of...
catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Committed 9dc5c66 and pushed to 10.1.x. Thanks!

Status: Fixed » Closed (fixed)

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