Problem/Motivation

Because of the issues #2977362: Revision user incorrectly appears as anonymous user when node author is canceled and #3089747: Content moderation can wrongly set old revisions as default when they're resaved and our necessity of restrict the cancel options to disable the user instead of delete, it is a good moment to add the ability to choose which options show when cancel an account.

Similar contrib modules:

Steps to reproduce

Delete a user from /user/[UID]/cancel

Proposed resolution

Add the ability to restrict the options cancel user account from the account settings /admin/config/people/accounts

Account settings:
Account settings

Account cancel page:
Account user cancel page

CommentFileSizeAuthor
#57 3384789-nr-bot_1c4m1ocq.txt6.67 KBneeds-review-queue-bot
#35 core-user-cancel-3384789-10_3_x.patch8.29 KBeduardo morales alberti
#32 requirements_user_duplication.png18.47 KBlpeidro
#20 3384789_screenshot.png69.91 KBeduardo morales alberti
#11 core-3384789-cancel-options-11.patch10.13 KBeduardo morales alberti
#6 core-3384789-cancel-options-6.patch5.74 KBeduardo morales alberti
#5 account_cancel_settings.png114.93 KBeduardo morales alberti
#5 account_cancel_page.png56.5 KBeduardo morales alberti
#4 core-3384789-cancel-options-4.patch4.65 KBeduardo morales alberti

Issue fork drupal-3384789

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

Eduardo Morales Alberti created an issue. See original summary.

Eduardo Morales Alberti opened merge request !4683

Eduardo Morales Alberti opened merge request !4684

eduardo morales alberti’s picture

eduardo morales alberti
Primary language Spanish
Location Spain, 🇪🇺
commented
Status: Active » Needs review
Issue tags: +Needs tests
Related issues: +#2977362: Revision user incorrectly appears as anonymous user when node author is canceled
StatusFileSize
new core-3384789-cancel-options-4.patch4.65 KB

Ready to review, It needs tests, but we would like to confirm if this solution is good for Drupal and then we will add the coverage.

eduardo morales alberti’s picture

eduardo morales alberti
Primary language Spanish
Location Spain, 🇪🇺
commented
Issue summary: View changes
StatusFileSize
new account_cancel_page.png56.5 KB
new account_cancel_settings.png114.93 KB
eduardo morales alberti’s picture

eduardo morales alberti
Primary language Spanish
Location Spain, 🇪🇺
commented
StatusFileSize
new core-3384789-cancel-options-6.patch5.74 KB

Upload patch with the right Schema

The last submitted patch, 4: core-3384789-cancel-options-4.patch, failed testing. View results

Status: Needs review » Needs work

The last submitted patch, 6: core-3384789-cancel-options-6.patch, failed testing. View results

cilefen’s picture

cilefen commented
Version: 9.5.x-dev » 11.x-dev
eduardo morales alberti’s picture

eduardo morales alberti
Primary language Spanish
Location Spain, 🇪🇺
commented

Tests fixed, pending to cover with tests the new functionality.

eduardo morales alberti’s picture

eduardo morales alberti
Primary language Spanish
Location Spain, 🇪🇺
commented
Status: Needs work » Needs review
StatusFileSize
new core-3384789-cancel-options-11.patch10.13 KB

Created test and solved failed PHPUnit tests, ready to review.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work
Issue tags: -Needs tests +Needs upgrade path tests

Tests are looking good.

Think we will need a simple update test though. Something simple like

Check values are null
Run updates
Check values are false.

Rest looks good!

eduardo morales alberti’s picture

eduardo morales alberti
Primary language Spanish
Location Spain, 🇪🇺
commented
Status: Needs work » Needs review

Added upgrade test checking the new values on user.settings.

eduardo morales alberti’s picture

eduardo morales alberti
Primary language Spanish
Location Spain, 🇪🇺
commented
Status: Needs review » Needs work

Reviewing failed tests on Drupal 11.x

eduardo morales alberti’s picture

eduardo morales alberti
Primary language Spanish
Location Spain, 🇪🇺
commented
Status: Needs work » Needs review
Issue tags: -Needs upgrade path tests

@smustgrave Both MR tests passed.

poker10’s picture

poker10 commented

Thanks for working on this @Eduardo Morales Alberti!

I am not sure, that the code is doing what the field description says:

Choose the method that will be available for the user when canceling the account

Looking at the changed code:

@@ -765,7 +766,11 @@ function user_cancel_methods() {
-    if (isset($method['access'])) {
+
+    if (isset($cancel_methods_settings[$name]) && !($cancel_methods_settings[$name])) {
+      $form[$name]['#access'] = $cancel_methods_settings[$name];
+    }
+    elseif (isset($method['access'])) {
       $form[$name]['#access'] = $method['access'];
     }

It seems to me, that the new field can only restrict access in addition to the already set #access property. It cannot explicitelly allow an access to the option if #access is already disabled (so the option will not be displayed). Therefore the field name and description seem to me a bit confusing.

Other thing is that in case you have disabled some options (via hook_user_cancel_methods_alter), these options will still display in the new field because of this:

+    // Do not hide options on user cancel methods.
+    foreach (array_keys($form['registration_cancellation']['user_cancel_methods']['#options']) as $method_name) {
+      if (isset($form['registration_cancellation']['user_cancel_methods'][$method_name]['#access'])) {
+        unset($form['registration_cancellation']['user_cancel_methods'][$method_name]['#access']);
+      }
+    }

I think, that this is not good from the usability point of view (and it has no point to allow a user to check/uncheck them, because these will not display regardless of this setting).

Other point is, what will happen if you select a default option in the user_cancel_method, but you will not enable that method in user_cancel_methods? Should a new validation be added?

Available options can be altered via contrib modules, so I would consider if this change would fit to the core. In case yes, I think that we need a Usability review to check the new field (and its title / description), so we do not create a confusion among users. Thanks!

eduardo morales alberti’s picture

eduardo morales alberti
Primary language Spanish
Location Spain, 🇪🇺
commented
Status: Needs review » Needs work
eduardo morales alberti’s picture

eduardo morales alberti
Primary language Spanish
Location Spain, 🇪🇺
commented

So after reading the comment from @poker10

The pending tasks are:

  • If by settings the option is disabled, do not allow to alter the access to other modules.
  • Do not show the cancel options in the checkbox list if some modules have already disabled them.
  • Set the available cancel options before the default options, and set the states to hide the disabled options, also add a validation to check that the user does not choose a disabled option as a default cancel option.

@poker10 Did I forget any point?

eduardo morales alberti’s picture

eduardo morales alberti
Primary language Spanish
Location Spain, 🇪🇺
commented

If we change the description of the field to "Disable the following cancel methods:", does this fit better for you?

Changing from:


    if (isset($cancel_methods_settings[$name]) && !($cancel_methods_settings[$name])) {
      $form[$name]['#access'] = $cancel_methods_settings[$name];
    }
    elseif (isset($method['access'])) {
      $form[$name]['#access'] = $method['access'];
    }

To 


    if ($cancel_methods_settings[$name]) {
      $form[$name]['#access'] = FALSE;
    }
    elseif (isset($method['access'])) {
      $form[$name]['#access'] = $method['access'];
    }
eduardo morales alberti’s picture

eduardo morales alberti
Primary language Spanish
Location Spain, 🇪🇺
commented
Status: Needs work » Needs review
StatusFileSize
new 3384789_screenshot.png69.91 KB

Ready to review.

Now the options to disable the cancel options are like this:
admin/config/people/accounts
Screenshot account settings cancel

We also added a validation to avoid choosing a disabled user cancel method.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs followup

Probably can close MR 4683 I don't think non bugs are being backported to D9 anymore.

Reviewing 4684
Applied the patch
Ran updb which ran without issue
Verified the config changes appear when doing a config export
Verified test coverage for post_update is there

Was checking the settings and think a good follow up would be to add states to the cancel methods. For example if I try to check "Disable the account and keep its content." I get an error in "When cancelling a user account" with message "The default user cancel method can not be a disabled method." Tagging for follow up if someone could open that.

But configuration updates work.

Was also still able to cancel an account.

xjm’s picture

xjm
she/her
Primary language English
commented

Hiding file attachments since the MR work seems most recent. @Eduardo Morales Alberti, for future reference, it's not necessary to post patches on an issue that has a merge request, and mixing the two adds noise and makes the issue harder to review. Thanks!

Edit: I also closed the D9 MR version as suggested.

xjm closed merge request !4683

xjm’s picture

xjm
she/her
Primary language English
commented
Title: Allow to choose the cancel options » Allow user admins to choose the user cancellation method on the user deletion confirmation form (rather than having a single sitewide setting)
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs framework manager review

Thanks for your work on this!

It took me a couple reads to understand what this is actually proposing. Posted a few remarks on the MR.

I think #21 needs to be fixed here (with test coverage), not in a followup. Otherwise, we're closing a feature request but opening a bug instead.

Do users see these options when canceling their own accounts, or do they just follow the default option? And if so, should the user be told that?

I've more concerns aside from those I already expressed but out of time for the moment. I think this issue needs framework manager feedback on the correct approach, or whether this is safe or a good idea.

xjm’s picture

xjm
she/her
Primary language English
commented
Issue tags: +Needs product manager review

I was thinking about the architectural implications of whether core should allow this configuration, but we could also use product manager feedback on whether this is a suitable feature for core. There's a usability cost to having an extra field every time a user is canceled, as well.

eduardo morales alberti’s picture

eduardo morales alberti
Primary language Spanish
Location Spain, 🇪🇺
commented

Working on xjm comments

eduardo morales alberti’s picture

eduardo morales alberti
Primary language Spanish
Location Spain, 🇪🇺
commented
Status: Needs work » Needs review

All issues were solved, added more testing coverage and validations, ready to review.

poker10’s picture

poker10 commented

@Eduardo Morales Alberti yes, I think the descriptions / labels are better now, thanks!

The MR proposes to hardcode the original four cancellation methods in the cancel_method_options (in user.settings.yml and user.schema.yml).

But it is still possible to modify these methods via hook_user_cancel_methods_alter, so it is possible to add a new method for example. What would happen in that case - would it be possible to disable also such a new method added via that hook?

eduardo morales alberti’s picture

eduardo morales alberti
Primary language Spanish
Location Spain, 🇪🇺
commented

@poker10 Maybe the property should be a sequence instead of a mapping, reading the documentation about schema I saw this https://www.drupal.org/docs/drupal-apis/configuration-api/configuration-...

Mappings and sequences

There are two base list types defined by
configuration schema. If all keys can be predefined
in the schema structure, then you should use a
mapping, otherwise, you should use a sequence (even
if the list’s keys are strings).

#3384789-28: Allow restriction of available User Cancellation Methods on deletion confirmation form

would it be possible to disable also such a new method added via that hook?

The Account Settings form will also load the custom methods because it calls to user_cancel_methods, if they are not configured, will be shown as disabled, in those cases, we can show them as "Available" to avoid lost methods on the form save, and then let the user chose if checks or not those methods. What do you think about it?
https://git.drupalcode.org/project/drupal/-/merge_requests/4684/diffs#0f...

$form['registration_cancellation']['user_cancel_method_options'] += user_cancel_methods();

If the custom method is not configured by the user.settings then the method will be available.
https://git.drupalcode.org/project/drupal/-/merge_requests/4684/diffs#b2...

    // Allow other modules to alter the access if the method is available.
    if (isset($cancel_methods_settings[$name]) && !($cancel_methods_settings[$name])) {
      $form[$name]['#access'] = FALSE;
    }
    elseif (isset($method['access'])) {
      $form[$name]['#access'] = $method['access'];
    }
smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Can this be rebased? Gitlab is saying this is unmergable

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

Ran into this on two separate sites this week, where content has been deleted in error because users with "administer users" did not understand the difference between the options. Allowing this to be configured in core would be a good step.

However I think the title of this issue is incorrect? Admins can already choose the cancellation method on the user cancel form; this issue proposes a separate option to restrict the set of available options?

lpeidro’s picture

lpeidro commented
StatusFileSize
new requirements_user_duplication.png18.47 KB

Hello:

I have found two main problems in the patch:

  • In a Drupal 9.5 I would be able to install the patch, but the four cancel options are still shown.
  • In a Drupal 10.3 I have problems with the patch application due the function user_requirements are instanced twice.

Requirements

eduardo morales alberti’s picture

eduardo morales alberti
Primary language Spanish
Location Spain, 🇪🇺
commented
Title: Allow user admins to choose the user cancellation method on the user deletion confirmation form (rather than having a single sitewide setting) » Allow restriction of available User Cancellation Methods on deletion confirmation form

@longwave Title updated.

Fixing merge request issues.

eduardo morales alberti’s picture

eduardo morales alberti
Primary language Spanish
Location Spain, 🇪🇺
commented
Status: Needs work » Needs review

Ready to review

eduardo morales alberti’s picture

eduardo morales alberti
Primary language Spanish
Location Spain, 🇪🇺
commented
StatusFileSize
new core-user-cancel-3384789-10_3_x.patch8.29 KB

@lpeidro

  1. Drupal 9.x is not supported by Drupal
  2. If the function user_requirements is duplicated, the patch did not apply properly, take into account the MR is over Drupal 11.X not 10.3.x, here is the patch that we have applied on Drupal 10.3.x
eduardo morales alberti’s picture

eduardo morales alberti
Primary language Spanish
Location Spain, 🇪🇺
commented

Errors on MR:
PHPUnit:

    PHPUnit Test failed to complete; Error: PHPUnit 10.5.35 by Sebastian
    Bergmann and contributors.
    
    Runtime:       PHP 8.3.12
    Configuration: /builds/issue/drupal-3384789/core/phpunit.xml.dist
    
    F                                                                   1 / 1
    (100%)
    
    Time: 00:03.085, Memory: 8.00 MB
    
    There was 1 failure:
    
    1)
    Drupal\Tests\config_translation\Kernel\Migrate\d6\MigrateUserConfigsTranslationTest::testUserMail
    Schema errors for user.settings with the following errors: 0 []
    'cancel_method_options' is a required key.
    (/builds/issue/drupal-3384789/core/lib/Drupal/Core/Config/Development/ConfigSchemaChecker.php:98)
    Failed asserting that two strings are equal.
    --- Expected
    +++ Actual
    @@ @@
    -'status'
    +'error'
    
    /builds/issue/drupal-3384789/core/modules/migrate/tests/src/Kernel/MigrateTestBase.php:203
    /builds/issue/drupal-3384789/core/modules/migrate/src/MigrateExecutable.php:515
    /builds/issue/drupal-3384789/core/modules/migrate/src/MigrateExecutable.php:273
    /builds/issue/drupal-3384789/core/modules/migrate/tests/src/Kernel/MigrateTestBase.php:179
    /builds/issue/drupal-3384789/core/modules/migrate/tests/src/Kernel/MigrateTestBase.php:192
    /builds/issue/drupal-3384789/core/modules/config_translation/tests/src/Kernel/Migrate/d6/MigrateUserConfigsTranslationTest.php:32

---- Drupal\Tests\standard\FunctionalJavascript\StandardJavascriptTest ----
Status    Group      Filename          Line Function                            
--------------------------------------------------------------------------------
Exception Other      phpunit-68.xml       0 Drupal\Tests\standard\FunctionalJav
    PHPUnit Test failed to complete; Error: PHPUnit 10.5.35 by Sebastian
    Bergmann and contributors.
    
    Runtime:       PHP 8.3.12
    Configuration: /builds/issue/drupal-3384789/core/phpunit.xml.dist
    
    E                                                                   1 / 1
    (100%)
    
    Time: 00:02.126, Memory: 8.00 MB
    
    There was 1 error:
    
    1)
    Drupal\Tests\standard\FunctionalJavascript\StandardJavascriptTest::testBigPipe
    Drupal\Core\Config\Schema\SchemaIncompleteException: Schema errors for
    user.settings with the following errors: 0 [] 'cancel_method_options' is a
    required key.
    
    /builds/issue/drupal-3384789/core/lib/Drupal/Core/Config/Development/ConfigSchemaChecker.php:98
    /builds/issue/drupal-3384789/vendor/symfony/event-dispatcher/EventDispatcher.php:206
    /builds/issue/drupal-3384789/vendor/symfony/event-dispatcher/EventDispatcher.php:56
    /builds/issue/drupal-3384789/core/lib/Drupal/Core/Config/Config.php:230
    /builds/issue/drupal-3384789/core/lib/Drupal/Core/Config/ConfigInstaller.php:396
    /builds/issue/drupal-3384789/core/lib/Drupal/Core/Config/ConfigInstaller.php:149
    /builds/issue/drupal-3384789/core/lib/Drupal/Core/ProxyClass/Config/ConfigInstaller.php:75
    /builds/issue/drupal-3384789/core/lib/Drupal/Core/Extension/ModuleInstaller.php:333
    /builds/issue/drupal-3384789/core/lib/Drupal/Core/ProxyClass/Extension/ModuleInstaller.php:83
    /builds/issue/drupal-3384789/core/includes/install.core.inc:1916
    /builds/issue/drupal-3384789/core/includes/batch.inc:296
    /builds/issue/drupal-3384789/core/includes/form.inc:977
    /builds/issue/drupal-3384789/core/includes/install.core.inc:654
    /builds/issue/drupal-3384789/core/includes/install.core.inc:572
    /builds/issue/drupal-3384789/core/includes/install.core.inc:121
    /builds/issue/drupal-3384789/core/lib/Drupal/Core/Test/FunctionalTestSetupTrait.php:315
    /builds/issue/drupal-3384789/core/tests/Drupal/Tests/BrowserTestBase.php:546
    /builds/issue/drupal-3384789/core/tests/Drupal/Tests/BrowserTestBase.php:363

--------------------------------------------------------------------------------
Exception Other      phpunit-53.xml       0 Drupal\Tests\content_translation\Fu
    PHPUnit Test failed to complete; Error: PHPUnit 10.5.35 by Sebastian
    Bergmann and contributors.
    
    Runtime:       PHP 8.3.12
    Configuration: /builds/issue/drupal-3384789/core/phpunit.xml.dist
    
    E                                                                   1 / 1
    (100%)
    
    Time: 00:02.117, Memory: 8.00 MB
    
    There was 1 error:
    
    1)
    Drupal\Tests\content_translation\FunctionalJavascript\ContentTranslationConfigUITest::testContentTranslationConfigUI
    Drupal\Core\Config\Schema\SchemaIncompleteException: Schema errors for
    user.settings with the following errors: 0 [] 'cancel_method_options' is a
    required key.
    
    /builds/issue/drupal-3384789/core/lib/Drupal/Core/Config/Development/ConfigSchemaChecker.php:98
    /builds/issue/drupal-3384789/vendor/symfony/event-dispatcher/EventDispatcher.php:206
    /builds/issue/drupal-3384789/vendor/symfony/event-dispatcher/EventDispatcher.php:56
    /builds/issue/drupal-3384789/core/lib/Drupal/Core/Config/Config.php:230
    /builds/issue/drupal-3384789/core/lib/Drupal/Core/Config/ConfigInstaller.php:396
    /builds/issue/drupal-3384789/core/lib/Drupal/Core/Config/ConfigInstaller.php:149
    /builds/issue/drupal-3384789/core/lib/Drupal/Core/ProxyClass/Config/ConfigInstaller.php:75
    /builds/issue/drupal-3384789/core/lib/Drupal/Core/Extension/ModuleInstaller.php:333
    /builds/issue/drupal-3384789/core/lib/Drupal/Core/ProxyClass/Extension/ModuleInstaller.php:83
    /builds/issue/drupal-3384789/core/includes/install.core.inc:1916
    /builds/issue/drupal-3384789/core/includes/batch.inc:296
    /builds/issue/drupal-3384789/core/includes/form.inc:977
    /builds/issue/drupal-3384789/core/includes/install.core.inc:654
    /builds/issue/drupal-3384789/core/includes/install.core.inc:572
    /builds/issue/drupal-3384789/core/includes/install.core.inc:121
    /builds/issue/drupal-3384789/core/lib/Drupal/Core/Test/FunctionalTestSetupTrait.php:315
    /builds/issue/drupal-3384789/core/tests/Drupal/Tests/BrowserTestBase.php:546
    /builds/issue/drupal-3384789/core/tests/Drupal/Tests/BrowserTestBase.php:363

---- Drupal\Tests\demo_umami\FunctionalJavascript\OpenTelemetryFrontPagePerformanceTest ----
Status    Group      Filename          Line Function                            
--------------------------------------------------------------------------------
Exception Other      phpunit-6.xml        0 Drupal\Tests\demo_umami\FunctionalJ
    PHPUnit Test failed to complete; Error: PHPUnit 10.5.35 by Sebastian
    Bergmann and contributors.
    
    Runtime:       PHP 8.3.12
    Configuration: /builds/issue/drupal-3384789/core/phpunit.xml.dist
    
    E                                                                   1 / 1
    (100%)
    
    Time: 00:01.718, Memory: 8.00 MB
    
    There was 1 error:
    
    1)
    Drupal\Tests\demo_umami\FunctionalJavascript\OpenTelemetryFrontPagePerformanceTest::testFrontPagePerformance
    Drupal\Core\Config\Schema\SchemaIncompleteException: Schema errors for
    user.settings with the following errors: 0 [] 'cancel_method_options' is a
    required key.
    
    /builds/issue/drupal-3384789/core/lib/Drupal/Core/Config/Development/ConfigSchemaChecker.php:98
    /builds/issue/drupal-3384789/vendor/symfony/event-dispatcher/EventDispatcher.php:206
    /builds/issue/drupal-3384789/vendor/symfony/event-dispatcher/EventDispatcher.php:56
    /builds/issue/drupal-3384789/core/lib/Drupal/Core/Config/Config.php:230
    /builds/issue/drupal-3384789/core/lib/Drupal/Core/Config/ConfigInstaller.php:396
    /builds/issue/drupal-3384789/core/lib/Drupal/Core/Config/ConfigInstaller.php:149
    /builds/issue/drupal-3384789/core/lib/Drupal/Core/ProxyClass/Config/ConfigInstaller.php:75
    /builds/issue/drupal-3384789/core/lib/Drupal/Core/Extension/ModuleInstaller.php:333
    /builds/issue/drupal-3384789/core/lib/Drupal/Core/ProxyClass/Extension/ModuleInstaller.php:83
    /builds/issue/drupal-3384789/core/includes/install.core.inc:1916
    /builds/issue/drupal-3384789/core/includes/batch.inc:296
    /builds/issue/drupal-3384789/core/includes/form.inc:977
    /builds/issue/drupal-3384789/core/includes/install.core.inc:654
    /builds/issue/drupal-3384789/core/includes/install.core.inc:572
    /builds/issue/drupal-3384789/core/includes/install.core.inc:121
    /builds/issue/drupal-3384789/core/lib/Drupal/Core/Test/FunctionalTestSetupTrait.php:315
    /builds/issue/drupal-3384789/core/tests/Drupal/Tests/BrowserTestBase.php:546
    /builds/issue/drupal-3384789/core/tests/Drupal/Tests/BrowserTestBase.php:363
    /builds/issue/drupal-3384789/core/tests/Drupal/FunctionalJavascriptTests/PerformanceTestBase.php:28
eduardo morales alberti’s picture

eduardo morales alberti
Primary language Spanish
Location Spain, 🇪🇺
commented

We are not able to reproduce the PHPUnit errors on a Drupal 11.0-x-dev locally.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

appears to have a number of test failures.

eduardo morales alberti’s picture

eduardo morales alberti
Primary language Spanish
Location Spain, 🇪🇺
commented

@smustgrave What should we change on the MR?

The errors are:

Drupal\Core\Config\Schema\SchemaIncompleteException: Schema errors for
    user.settings with the following errors: 0 [] 'cancel_method_options' is a
    required key.

But it is defined by the user.settings install config https://git.drupalcode.org/project/drupal/-/merge_requests/4684/diffs#1e...

cancel_method_options:
  user_cancel_block: true
  user_cancel_block_unpublish: true
  user_cancel_reassign: true
  user_cancel_delete: true

We could not reproduce it locally.

eduardo morales alberti’s picture

eduardo morales alberti
Primary language Spanish
Location Spain, 🇪🇺
commented
Related issues: +#3482107: Introduce it on core
smustgrave’s picture

smustgrave commented

Maybe something in the migration tests need to be updated to be aware of the new keys

eduardo morales alberti’s picture

eduardo morales alberti
Primary language Spanish
Location Spain, 🇪🇺
commented

@smutgrave it is happening on all types of tests not only on migrations.
Seems related to the schema validation, but the schema seems to be right.

    /builds/issue/drupal-3384789/core/lib/Drupal/Core/Config/Development/ConfigSchemaChecker.php:98
    /builds/issue/drupal-3384789/vendor/symfony/event-dispatcher/EventDispatcher.php:206
    /builds/issue/drupal-3384789/vendor/symfony/event-dispatcher/EventDispatcher.php:56
    /builds/issue/drupal-3384789/core/lib/Drupal/Core/Config/Config.php:230
    /builds/issue/drupal-3384789/core/lib/Drupal/Core/Config/ConfigInstaller.php:396
    /builds/issue/drupal-3384789/core/lib/Drupal/Core/Config/ConfigInstaller.php:149
    /builds/issue/drupal-3384789/core/lib/Drupal/Core/ProxyClass/Config/ConfigInstaller.php:75
    /builds/issue/drupal-3384789/core/lib/Drupal/Core/Extension/ModuleInstaller.php:333
    /builds/issue/drupal-3384789/core/lib/Drupal/Core/ProxyClass/Extension/ModuleInstaller.php:83
    /builds/issue/drupal-3384789/core/includes/install.core.inc:1916
    /builds/issue/drupal-3384789/core/includes/batch.inc:296
    /builds/issue/drupal-3384789/core/includes/form.inc:977
    /builds/issue/drupal-3384789/core/includes/install.core.inc:654
    /builds/issue/drupal-3384789/core/includes/install.core.inc:572
    /builds/issue/drupal-3384789/core/includes/install.core.inc:121
    /builds/issue/drupal-3384789/core/lib/Drupal/Core/Test/FunctionalTestSetupTrait.php:315
    /builds/issue/drupal-3384789/core/tests/Drupal/Tests/BrowserTestBase.php:546
    /builds/issue/drupal-3384789/core/tests/Drupal/Tests/BrowserTestBase.php:363
    /builds/issue/drupal-3384789/core/tests/Drupal/FunctionalJavascriptTests/PerformanceTestBase.php:28
smustgrave’s picture

smustgrave commented

Ah are there user.settings in any profile or test modules?

eduardo morales alberti’s picture

eduardo morales alberti
Primary language Spanish
Location Spain, 🇪🇺
commented

Thank you! We saw that some profiles have their user.settings like demo umami, we review it!

eduardo morales alberti’s picture

eduardo morales alberti
Primary language Spanish
Location Spain, 🇪🇺
commented

The remaining failed tests do not seem to be related to this change.

Any clue?

Failed PHPUnit: Solved after rerun the test.

There was 1 failure:
1)
Drupal\Tests\editor\Functional\EditorSecurityTest::testEditorXssFilterOverride
Behat\Mink\Exception\ExpectationException: The field "edit-body-0-value"
value is "Hello, Dumbo Octopus!alert(0)", but "Hello, Dumbo
Octopus!alert(0)" expected.
/builds/issue/drupal-3384789/vendor/behat/mink/src/WebAssert.php:888
/builds/issue/drupal-3384789/vendor/behat/mink/src/WebAssert.php:781
/builds/issue/drupal-3384789/core/tests/Drupal/Tests/WebAssert.php:991
/builds/issue/drupal-3384789/core/modules/editor/tests/src/Functional/EditorSecurityTest.php:454
FAILURES!
Tests: 3, Assertions: 131, Failures: 1.
---- Drupal\Tests\language\Functional\ConfigurableLanguageManagerTest ----

    Time: 00:39.668, Memory: 8.00 MB
    
    There was 1 failure:
    
    1)
    Drupal\Tests\responsive_image\FunctionalJavascript\ResponsiveImageFieldUiTest::testResponsiveImageFormatterUi
    Behat\Mink\Exception\ExpectationException: The string "Select a responsive
    image style." was not found anywhere in the HTML response of the current
    page.
    
    /builds/issue/drupal-3384789/vendor/behat/mink/src/WebAssert.php:888
    /builds/issue/drupal-3384789/vendor/behat/mink/src/WebAssert.php:363
    /builds/issue/drupal-3384789/core/tests/Drupal/Tests/WebAssert.php:558
    /builds/issue/drupal-3384789/core/modules/responsive_image/tests/src/FunctionalJavascript/ResponsiveImageFieldUiTest.php:94
    
    FAILURES!
    Tests: 1, Assertions: 11, Failures: 1.
---- Drupal\FunctionalJavascriptTests\Ajax\AjaxFormCacheTest ----

Failed Nightwatch: Solved after rerun the test.

  Error
    ✖ NightwatchAssertError
   Timed out while waiting for element <form.system-modules [name="modules[navigation][enable]"]:disabled> to be present for 10000 milliseconds. - expected "found" but got: "not found" (10495ms)
    Error location:
    /builds/issue/drupal-3384789/core/tests/Drupal/Nightwatch/Commands/drupalInstallModule.js:39
    –––––––––––––––––––––––––––––––––––––––––––––––––––––––––––––––––––––––––––––––––––––––––––
     37 |     // Wait for the checkbox for the module to be disabled as a sign that the
     38 |     // module has been enabled.
     39 |     this.waitForElementPresent( 
     40 |       `form.system-modules [name="modules[${module}][enable]"]:disabled`,
     41 |       10000,
    –––––––––––––––––––––––––––––––––––––––––––––––––––––––––––––––––––––––––––––––––––––––––––
 Wrote HTML report file to: /builds/issue/drupal-3384789/nightwatch_output/nightwatch-html-report/index.html
eduardo morales alberti’s picture

eduardo morales alberti
Primary language Spanish
Location Spain, 🇪🇺
commented
Status: Needs work » Needs review

All tests fixed.

eduardo morales alberti’s picture

eduardo morales alberti
Primary language Spanish
Location Spain, 🇪🇺
commented

Fixed MR conflicts.

eduardo morales alberti’s picture

eduardo morales alberti
Primary language Spanish
Location Spain, 🇪🇺
commented

Ready to review

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Status: Needs review » Needs work
Issue tags: -Needs framework manager review

Great work here, left a review

eduardo morales alberti’s picture

eduardo morales alberti
Primary language Spanish
Location Spain, 🇪🇺
commented

Pending review PHPUnit test after applying suggestions.
Failing Drupal\Tests\user\Functional\UserSubAdminTest::testCancelAccount

├ Behat\Mink\Exception\ElementNotFoundException: Form field with id|name|label|value "user_cancel_method[user_cancel_block]" not found.

eduardo morales alberti’s picture

eduardo morales alberti
Primary language Spanish
Location Spain, 🇪🇺
commented

All threads solved but one, pending to review configuration

eduardo morales alberti’s picture

eduardo morales alberti
Primary language Spanish
Location Spain, 🇪🇺
commented
Status: Needs work » Needs review

All threads solved!! Ready to review

eduardo morales alberti’s picture

eduardo morales alberti
Primary language Spanish
Location Spain, 🇪🇺
commented

Reverting the latest commit and applying the schema again, as it had too many changes not related to the issue

eduardo morales alberti’s picture

eduardo morales alberti
Primary language Spanish
Location Spain, 🇪🇺
commented

Ready to review!!

eduardo morales alberti’s picture

eduardo morales alberti
Primary language Spanish
Location Spain, 🇪🇺
commented
Issue summary: View changes
eduardo morales alberti’s picture

eduardo morales alberti
Primary language Spanish
Location Spain, 🇪🇺
commented
Issue summary: View changes
needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Needs review » Needs work
StatusFileSize
new 3384789-nr-bot_1c4m1ocq.txt6.67 KB

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

eduardo morales alberti’s picture

eduardo morales alberti
Primary language Spanish
Location Spain, 🇪🇺
commented
Status: Needs work » Needs review

Updated branch and solve bot messages

dcam’s picture

dcam commented
Status: Needs review » Needs work

I took a quick look at the MR and noticed that the new update path test doesn't have the newest attributes. I suggest rebasing it and updating the test.

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.