Problem/Motivation

We're still performing a manual db_insert() in user_install() to install user 0 and user 1. We should explore leveraging an entity save for that.

Proposed resolution

Remaining tasks

User interface changes

API changes

CommentFileSizeAuthor
#8 user-install-user-api-2249113-8.patch4.77 KBjessebeach
#8 interdiff.txt2.58 KBjessebeach
#5 2249113-5-user-install-use-api.patch6.28 KBtstoeckler
#5 2249113-3-5-interdiff.txt3.05 KBtstoeckler
#3 2249113-3-user-install-use-api.patch4.54 KBtstoeckler
#3 2249113-1-3-interdiff.txt4.28 KBtstoeckler
#1 2249113-1-user-install-use-api.patch1.39 KBtstoeckler
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler commented
Status: Active » Needs review
FileSize
2249113-1-user-install-use-api.patch1.39 KB

Let's see what breaks,

Status: Needs review » Needs work

The last submitted patch, 1: 2249113-1-user-install-use-api.patch, failed testing.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler commented
Status: Needs work » Needs review
FileSize
2249113-1-3-interdiff.txt4.28 KB
2249113-3-user-install-use-api.patch4.54 KB

Let's see how far this one goes:

    DEV 2249113: Patch #3
    
    - Add field as dependency of user
    - Install user module with dependencies in installer
    - Check for NULL explicitly when checking for the next ID in UserStorage
    - Remove pointless condition in ModuleHandlerInstall
    - Avoid pointless local variables in user_install()

Status: Needs review » Needs work

The last submitted patch, 3: 2249113-3-user-install-use-api.patch, failed testing.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler commented
Status: Needs work » Needs review
FileSize
2249113-3-5-interdiff.txt3.05 KB
2249113-5-user-install-use-api.patch6.28 KB

This should be green.

Btw, as I realize that that's not entirely obvious from the code:

The dependency of User module on Field module is due to ContentEntityDatabaseStorage - and by extension UserStorage - requiring the FieldInfo service which is provided by Field module.

This will be fixed with #2144263: Decouple entity field storage from configurable fields (or a follow-up thereof) at which point the dependency can probably be removed again, but for now it is a dependency, so we should declare it.

Also changed back to using $enable_dependencies = FALSE for performance in installer.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler commented

5: 2249113-5-user-install-use-api.patch queued for re-testing.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented

#2116363: Unified repository of field definitions (cache + API) should take care of the dependency.

jessebeach’s picture

jessebeach CreditAttribution: jessebeach commented
FileSize
interdiff.txt2.58 KB
user-install-user-api-2249113-8.patch4.77 KB

I've reverted the change to ModuleHandler::install(). It's erroneous to the issue in this patch.

Added two @todos: one for determining the langcode and one for removing the field dependency in User. Both have associated issues.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented 
+++ b/core/modules/user/user.install
@@ -220,28 +220,27 @@ function user_schema() {
-      'status' => 1,

What's the default status for new user accounts?

cf. #2257761: Interactive installer tests cause emails to be sent out

jessebeach’s picture

jessebeach CreditAttribution: jessebeach commented

Active, with a note in the User Entity base field definition:

$fields['status'] = FieldDefinition::create('boolean')
  ->setLabel(t('User status'))
  ->setDescription(t('Whether the user is active or blocked.'))
  // @todo As the status has access implications users should be created as
  //   blocked by default and activated explicitly if needed. See
  //   https://drupal.org/node/2248969.
  ->setSetting('default_value', TRUE);
Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Status: Needs review » Reviewed & tested by the community

Thanks this looks good. We're not changing the default and I don't think the e-mail is related to the default status, as it is only sent later after you filled out the final configuration form where the user already exists AFAIK. drush has a setting to prevent that, see drush si --help.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Fixed

Committed 902098c and pushed to 8.x. Thanks!

  • Commit 902098c on 8.x by alexpott: 
    Issue #2249113 by tstoeckler, jessebeach: Use an entity save instead of...
tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler commented

@jessebeach: Thanks for the patch update. Extracted the ModuleHandler change into #2267911: Remove pointless condition in ModuleHandler::install().

Status: Fixed » Closed (fixed)

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