Problem/Motivation

Module install updates default user form display like a simple config 

  //rebuild user entity form display for new fields.
  $config = \Drupal::service('config.factory')->getEditable('core.entity_form_display.user.user.default');

  // Not sure why this isn't set, but it throws an error in simpletest.
  $config->set('targetEntityType', 'user');
  $config->set('bundle', 'user');

So that leads to broken user/X/edit form and admin/config/people/accounts/form-display

Fatal error: Call to a member function getName() on null in .../core/lib/Drupal/Core/Config/Entity/ConfigEntityStorage.php on line 214

Proposed resolution

Use form display API to add fields and enforce dependency on the module for shipped fields

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#14 user_edit_is_broken-2605086-14.patch6.3 KBandypost
#14 interdiff.txt5.01 KBandypost
#13 password_policy-reinstall_broken-2605086-13-8.x-3.x.patch5.78 KBnerdstein
#11 password_policy-reinstall_broken-2605086-11-8.x-3.x.patch4.54 KBnerdstein
#6 password_policy-reinstall_broken-2605086-6-8.x-3.x.patch4.74 KBnerdstein
#3 2605086-3.patch2.54 KBandypost

Comments

andypost created an issue. See original summary.

andypost’s picture

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

the error message is
Fatal error: Call to a member function getName() on null in ..../core/lib/Drupal/Core/Config/Entity/ConfigEntityStorage.php on line 214

andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Active » Needs review
StatusFileSize
new 2605086-3.patch2.54 KB

Also PR https://github.com/d8-contrib-modules/password_policy/pull/69

nerdstein’s picture

nerdstein
he/him
Primary language English
Location United States
commented
Assigned: Unassigned » nerdstein
nerdstein’s picture

nerdstein
he/him
Primary language English
Location United States
commented

Per the contrib tracking page:

@andypost - also... the module does not allow reinstall (uninstall and there's no way install again)

My response:

I am guessing this is because the install hook config exists upon reinstall and we're trying to install it again. We can solve within the uninstall hook. And, I am going to explore CMI - because I feel like we're trying to solve CMI related issues. I'll debug.

nerdstein’s picture

nerdstein
he/him
Primary language English
Location United States
commented
StatusFileSize
new password_policy-reinstall_broken-2605086-6-8.x-3.x.patch4.74 KB

For some reason, the configuration "dependencies" being set for password policies' installed user profile fields are not being observed. As such, these fields have not been removed upon uninstall. This causes the aforementioned error where uninstall leaves password policy artifacts and it should not. Because it doesn't properly clean things up, re-installing breaks because the configuration already exists. The previous patch did not address the root of the issue, so I developed a new one that leverages the configuration API.

The guts of the patch is an updated uninstall hook, which removes all of the added and altered config. Why does this need done? I have no idea. I expected "dependencies" defined in the config/install and the "dependencies" in the install hook to be respected when password_policy would be uninstalled. This may suggest another issue with this config (or even core), but the patch seems to work now.

Can someone review?

andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Needs review » Needs work

Final patch should be combined mine + password_policy_uninstall() from core's forum module

  1. +++ b/password_policy.install
@@ -16,28 +14,18 @@ function password_policy_install() {
+  ¶

@@ -46,9 +34,39 @@ function password_policy_install() {
\ No newline at end of file

    should be fixed 

  2. +++ b/password_policy.install
@@ -46,9 +34,39 @@ function password_policy_install() {
+  $conf = \Drupal::service('config.factory')->getEditable('core.entity_form_display.user.user.default.content.field_last_password_reset');
...
+  $conf = \Drupal::service('config.factory')->getEditable('core.entity_form_display.user.user.default.content.field_password_expiration');
...
+  $conf = \Drupal::service('config.factory')->getEditable('field.field.user.user.field_last_password_reset');
...
+  $conf = \Drupal::service('config.factory')->getEditable('field.field.user.user.field_password_expiration');
...
+  $conf = \Drupal::service('config.factory')->getEditable('field.storage.user.field_last_password_reset');
...
+  $conf = \Drupal::service('config.factory')->getEditable('field.storage.user.field_password_expiration');

    That's totally wrong
    Proper way see core's forum module uninstall
    forum does the same

andypost’s picture

andypost
he/him
Primary language Russian
commented

Additions to #3

+++ b/password_policy.install
@@ -15,40 +15,28 @@ function password_policy_install() {
+  $storage = \Drupal::entityManager()->getStorage('entity_form_display');

suppose this should be entityTypeManager() service now

nerdstein’s picture

nerdstein
he/him
Primary language English
Location United States
commented

Forum's uninstall hook only accounts for field storage. This may be the proper convention for removing the field storage configuration. But, CMI is used to install it. I don't see why this isn't acceptable and more generally applicable even outside of the field storage (as you can see). There is more than one way to skin a cat... I'd rather do it the correct way, I'm just not convinced by one example in core. Are there other examples?

Plus, this approach can't be used for the other config -- adjustments to the the user edit form display and the field settings. This use case seems a bit different from the forum module.

andypost’s picture

andypost
he/him
Primary language Russian
commented

@nerdstein the difference in CMI and EntityAPI - fields and storage are entities that use CMI as storage, the same applies to entity displays - they must be managed with own api. Basically you can swap storage for this entities but install and uninstall will not break

nerdstein’s picture

nerdstein
he/him
Primary language English
Location United States
commented
StatusFileSize
new password_policy-reinstall_broken-2605086-11-8.x-3.x.patch4.54 KB

I was able to catch up with @timplunkett (again, thanks for your help).

The following patch addresses the issue. The uninstall hook can go away entirely. The dependencies, as noted previously, were not "enforced". Adding this key tied all of the config to the install/uninstall of the module. No uninstall hook needed...

andypost’s picture

andypost
he/him
Primary language Russian
commented 
  1. +++ b/config/install/field.field.user.user.field_last_password_reset.yml
@@ -1,11 +1,11 @@
 dependencies:
-  config:
-    - field.storage.user.field_last_password_reset
...
+  enforced:
+    config:
+      - field.storage.user.field_last_password_reset

+++ b/config/install/field.field.user.user.field_password_expiration.yml
@@ -1,10 +1,11 @@
 dependencies:
-  config:
-    - field.storage.user.field_password_expiration
...
+  enforced:
+    config:
+      - field.storage.user.field_password_expiration

    not sure that needed 

  2. +++ b/config/install/field.storage.user.field_password_expiration.yml
@@ -1,9 +1,9 @@
 dependencies:
-  module:
-    - user

    user should be there 

  3. +++ b/password_policy.install
@@ -16,28 +14,20 @@ function password_policy_install() {
+  /** @var $config \Drupal\Core\Config\Config */
   $config = \Drupal::service('config.factory')->getEditable('core.entity_form_display.user.user.default');

    once again, this is a entity, should be managed as #3

nerdstein’s picture

nerdstein
he/him
Primary language English
Location United States
commented
Status: Needs work » Needs review
StatusFileSize
new password_policy-reinstall_broken-2605086-13-8.x-3.x.patch5.78 KB

OK, the following patch should do the trick. The "enforced" config is needed to perform the appropriate cleanup when password policy is uninstalled.

andypost’s picture

andypost
he/him
Primary language Russian
commented
Issue summary: View changes
StatusFileSize
new interdiff.txt5.01 KB
new user_edit_is_broken-2605086-14.patch6.3 KB

Module should enforce only the module dependency, user form display is updated automatically.
Cleaned-up code and comments.
Field storage should be locked to prevent fields deleted via UI.

andypost’s picture

andypost
he/him
Primary language Russian
commented 
  1. +++ b/password_policy.install
@@ -1,54 +1,49 @@
+  $users = \Drupal::entityTypeManager()->getStorage('user')->loadMultiple();
+  // @todo Get rid of updating all users.
   foreach ($users as $user) {
...
+    $user
+      ->set('field_last_password_reset', $timestamp)
+      ->set('field_password_expiration', '0')
+      ->save();

    Default value for timestamp is now.
    And field already defines default value as 0 

  2. +++ b/password_policy.install
@@ -1,54 +1,49 @@
+  $user_form_display = $storage->load('user.user.default');
+  if (!$user_form_display) {
+    $user_form_display = $storage->create([
...
+    ->setComponent('field_last_password_reset', [
...
+    ->setComponent('field_password_expiration', [

    Actually I'm not sure that module should do that.
    It's a install profile task to ship display and set weight

nerdstein’s picture

nerdstein
he/him
Primary language English
Location United States
commented

@andypost - I'll test this out tonight and see how it works. In previous testing, I had to have "enforced" to ensure it was properly removed. I'll get back to you shortly once I pull this down and test.

nerdstein’s picture

nerdstein
he/him
Primary language English
Location United States
commented

@andypost - Oddly, the patch is not applying cleanly for me. I'll try to get it to apply.

nerdstein’s picture

nerdstein
he/him
Primary language English
Location United States
commented
Status: Needs review » Reviewed & tested by the community

The patch has been reviewed and is working great. I'll push.

  • nerdstein committed be10dd8 on authored by andypost 
    Issue #2605086 by nerdstein, andypost: User edit is broken after install
nerdstein’s picture

nerdstein
he/him
Primary language English
Location United States
commented
Status: Reviewed & tested by the community » Fixed

Pushed to the 8.x-3.x branch

andypost’s picture

andypost
he/him
Primary language Russian
commented
Assigned: nerdstein » Unassigned

@nerdstein Thanx a lot, looks we need follow-up to discus #15.2 #2650192: Properly place fields into entity display on install

Status: Fixed » Closed (fixed)

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