Add validation constraints to user.settings

Created on 26 March 2024, 4 months ago
Updated 12 July 2024, about 23 hours ago

Problem/Motivation

vendor/bin/drush config:inspect --filter-keys=user.settings --detail --list-constraints

โžœ  ๐Ÿค– Analyzingโ€ฆ

 Legend for Data:
  โœ…โ“  โ†’ Correct primitive type, detailed validation impossible.
  โœ…โœ…  โ†’ Correct primitive type, passed all validation constraints.
 ----------------------------------------------------- --------- ------------- ------ ---------------------------------------------------------------------------------------------
  Key                                                   Status    Validatable   Data   Validation constraints
 ----------------------------------------------------- --------- ------------- ------ ---------------------------------------------------------------------------------------------
  user.settings                                         Correct   84%           โœ…โ“   ValidKeys: '<infer>'
   user.settings:                                       Correct   Validatable   โœ…โœ…   ValidKeys: '<infer>'
   user.settings:_core                                  Correct   Validatable   โœ…โœ…   ValidKeys:
                                                                                         - default_config_hash
   user.settings:_core.default_config_hash              Correct   Validatable   โœ…โœ…   NotNull: {  }
                                                                                       Regex: '/^[a-zA-Z0-9\-_]+$/'
                                                                                       Length: 43
                                                                                       โ†ฃ PrimitiveType: {  }
   user.settings:anonymous                              Correct   Validatable   โœ…โœ…   Regex:
                                                                                         pattern: '/([^\PC])/u'
                                                                                         match: false
                                                                                         message: 'Labels are not allowed to span multiple lines or contain control characters.'
                                                                                       NotBlank: {  }
                                                                                       โ†ฃ PrimitiveType: {  }
   user.settings:cancel_method                          Correct   NOT           โœ…โ“   โš ๏ธ  @todo Add validation constraints here
   user.settings:langcode                               Correct   Validatable   โœ…โœ…   NotNull: {  }
                                                                                       Choice:
                                                                                         callback: 'Drupal\Core\TypedData\Plugin\DataType\LanguageReference::getAllValidLangcodes'
                                                                                       โ†ฃ PrimitiveType: {  }
   user.settings:notify                                 Correct   Validatable   โœ…โœ…   ValidKeys: '<infer>'
   user.settings:notify.cancel_confirm                  Correct   Validatable   โœ…โœ…   โ†ฃ PrimitiveType: {  }
   user.settings:notify.password_reset                  Correct   Validatable   โœ…โœ…   โ†ฃ PrimitiveType: {  }
   user.settings:notify.register_admin_created          Correct   Validatable   โœ…โœ…   โ†ฃ PrimitiveType: {  }
   user.settings:notify.register_no_approval_required   Correct   Validatable   โœ…โœ…   โ†ฃ PrimitiveType: {  }
   user.settings:notify.register_pending_approval       Correct   Validatable   โœ…โœ…   โ†ฃ PrimitiveType: {  }
   user.settings:notify.status_activated                Correct   Validatable   โœ…โœ…   โ†ฃ PrimitiveType: {  }
   user.settings:notify.status_blocked                  Correct   Validatable   โœ…โœ…   โ†ฃ PrimitiveType: {  }
   user.settings:notify.status_canceled                 Correct   Validatable   โœ…โœ…   โ†ฃ PrimitiveType: {  }
   user.settings:password_reset_timeout                 Correct   NOT           โœ…โ“   โš ๏ธ  @todo Add validation constraints here
   user.settings:password_strength                      Correct   Validatable   โœ…โœ…   โ†ฃ PrimitiveType: {  }
   user.settings:register                               Correct   NOT           โœ…โ“   โš ๏ธ  @todo Add validation constraints here
   user.settings:verify_mail                            Correct   Validatable   โœ…โœ…   โ†ฃ PrimitiveType: {  }
 ----------------------------------------------------- --------- ------------- ------ ---------------------------------------------------------------------------------------------

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

๐Ÿ“Œ Task
Status

Fixed

Version

11.0 ๐Ÿ”ฅ

Component
User moduleย  โ†’

Last updated about 22 hours ago

Created by

๐Ÿ‡ฎ๐Ÿ‡ณIndia omkar.podey

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @omkar.podey
  • Pipeline finished with Failed
    4 months ago
    Total: 861s
    #130240
  • ๐Ÿ‡ฎ๐Ÿ‡ณIndia omkar.podey
    1. register key could have 3 values according to \Drupal\user\UserInterface
    2. cancel_method can have any function assigned to it but i think it can't be empty.
    3. password_reset_timeout It is never being set just being read so I think it should just not be null.
  • Pipeline finished with Failed
    4 months ago
    Total: 847s
    #130423
  • Pipeline finished with Success
    4 months ago
    Total: 494s
    #130523
  • ๐Ÿ‡ฎ๐Ÿ‡ณIndia omkar.podey

    cancel_method has to be nullable according to testConfigForm

  • Status changed to Needs review 4 months ago
  • ๐Ÿ‡ฎ๐Ÿ‡ณIndia narendraR Jaipur, India
  • Status changed to Needs work 4 months ago
  • ๐Ÿ‡ง๐Ÿ‡ชBelgium Wim Leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ
  • Pipeline finished with Failed
    3 months ago
    Total: 505s
    #134378
  • Pipeline finished with Failed
    3 months ago
    Total: 490s
    #136241
  • Pipeline finished with Failed
    3 months ago
    Total: 493s
    #136364
  • ๐Ÿ‡ฎ๐Ÿ‡ณIndia omkar.podey

    The migration tests and UserMailNotifyTest isn't happy with making all the keys required because of marking user.settings as fullyValidatable .I could add keys to UserMailNotifyTest and remove the requiredkey: false from most of them, doing that next.

  • ๐Ÿ‡ง๐Ÿ‡ชBelgium borisson_ Mechelen, ๐Ÿ‡ง๐Ÿ‡ช

    I could add keys to UserMailNotifyTest and remove the requiredkey: false from most of them

    I agree, this looks like the best way

  • ๐Ÿ‡ฎ๐Ÿ‡ณIndia omkar.podey

    omkar.podey โ†’ changed the visibility of the branch 3436164-add-validation-constraints to hidden.

  • Pipeline finished with Failed
    3 months ago
    Total: 588s
    #142773
  • ๐Ÿ‡ฎ๐Ÿ‡ณIndia omkar.podey

    Okay so I switched to 10.3.x. and right now the only test that's failing is Drupal\Tests\user\Kernel\UserAdminSettingsFormTest::testConfigForm

    To fix this there were two options, either add config target data to core/modules/user/src/AccountSettingsForm.php or mark the keys that are failing the test as requiredKey: false in user.schema.yml.

    So I thought that the requiredKey false would be the solution but then I ran into the langcode issue again where the langcode isn't defined in user.schema.yml so I can't mark it requiredKey: false.

    thoughts ?

  • Pipeline finished with Failed
    3 months ago
    Total: 499s
    #144875
  • Pipeline finished with Success
    3 months ago
    Total: 2273s
    #146928
  • Pipeline finished with Success
    3 months ago
    Total: 4886s
    #147006
  • Status changed to Needs review 3 months ago
  • Status changed to Needs work 3 months ago
  • 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.

  • ๐Ÿ‡ฎ๐Ÿ‡ณIndia narendraR Jaipur, India
  • Status changed to Needs review 3 months ago
  • Status changed to Needs work 3 months ago
  • ๐Ÿ‡ฎ๐Ÿ‡ณIndia narendraR Jaipur, India
  • Pipeline finished with Success
    3 months ago
    Total: 996s
    #147741
  • Status changed to Needs review 3 months ago
  • Status changed to Needs work 3 months ago
  • ๐Ÿ‡ฎ๐Ÿ‡ณIndia narendraR Jaipur, India
  • Pipeline finished with Failed
    3 months ago
    Total: 201s
    #149010
  • Pipeline finished with Success
    3 months ago
    Total: 1017s
    #149030
  • Status changed to Needs review 3 months ago
  • Pipeline finished with Success
    3 months ago
    Total: 1077s
    #149824
  • Status changed to Needs work 3 months ago
  • ๐Ÿ‡ง๐Ÿ‡ชBelgium Wim Leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

    Getting close!

  • Pipeline finished with Canceled
    3 months ago
    Total: 374s
    #149968
  • Pipeline finished with Success
    3 months ago
    Total: 958s
    #149975
  • Pipeline finished with Success
    3 months ago
    Total: 1110s
    #149988
  • Status changed to Needs review 3 months ago
  • Status changed to Needs work 3 months ago
  • ๐Ÿ‡ง๐Ÿ‡ชBelgium Wim Leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

    99% ready!

  • Status changed to Needs review 3 months ago
  • Pipeline finished with Success
    3 months ago
    Total: 1035s
    #153062
  • Status changed to Needs work 3 months ago
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States smustgrave

    This is looking really good! Can the MR be updated for 11.x though please.

  • Status changed to Needs review 2 months ago
  • ๐Ÿ‡ฎ๐Ÿ‡ณIndia omkar.podey

    I would like to get it reviewed first and then we can decide if we need the 11.x branch explicitly.

  • Status changed to Needs work 2 months ago
  • ๐Ÿ‡ง๐Ÿ‡ชBelgium Wim Leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

    I would like to get it reviewed first and then we can decide if we need the 11.x branch explicitly.

    All changes land in 11.x first. So we need an MR that targets 11.x anyway.

    Still, reviewed it for you โ€ฆ and what I wrote in #24 is still true.

    If this were already targeting 11.x, I would and could've RTBC'd ๐Ÿ˜…

  • Pipeline finished with Canceled
    2 months ago
    Total: 3096s
    #159645
  • ๐Ÿ‡ฎ๐Ÿ‡ณIndia pradhumanjainOSL

    pradhumanjain2311 โ†’ made their first commit to this issueโ€™s fork.

  • Pipeline finished with Failed
    2 months ago
    Total: 6206s
    #159665
  • Pipeline finished with Failed
    2 months ago
    Total: 760s
    #159741
  • Pipeline finished with Success
    2 months ago
    Total: 1197s
    #159740
  • ๐Ÿ‡ง๐Ÿ‡ชBelgium Wim Leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

    The addition of the UserCancelMethod constraint seems fine, but the API addition in user_cancel_methods() is somewhat questionable. Then again, I don't see any better choice available to us โ€ฆ this is just super old code that has never been converted to more modern Drupal standards.

    #3135592: Cannot implement a custom user cancellation method โ†’ and โœจ Provide a Entity Handler for user cancelation Needs work are the issues to modernize that part of Drupal core. So until either of those land, this is doing the best we can. We don't need to make this the most elegant thing possible, because it isn't already anyway, and user_cancel_methods() will disappear when those are finished.

    Ah, no, the 11.x branch has two remaining failures:

    Drupal\Tests\user\Kernel\UserEntityLabelTest::testLabelCallback
    Drupal\Core\Config\Schema\SchemaIncompleteException: Schema errors for user.settings with the following errors: 0 [] &#039;verify_mail&#039; is a required key., 1 [] &#039;notify&#039; is a required key., 2 [] &#039;register&#039; is a required key., 3 [] &#039;cancel_method&#039; is a required key., 4 [] &#039;password_reset_timeout&#039; is a required key., 5 [] &#039;password_strength&#039; is a required key.
    

    and

    Drupal\Tests\user\Kernel\UserAdminSettingsFormTest::testConfigForm
    Input values: <em class="placeholder">Array
    (
        [anonymous] =&gt; 3rj{f&gt;&amp;3y@
        [user_mail_cancel_confirm_body] =&gt; {;D5&gt;&amp;vM
        [user_mail_cancel_confirm_subject] =&gt; @R)&quot;o=`EfB&gt;&amp;T)#!\)V&lt;
        [register_pending_approval_admin_body] =&gt; 6e*n&gt;&amp;Ob
        [register_pending_approval_admin_subject] =&gt; %`6JwPffFF&gt;&amp;9WqUZ&#039;_4
        [user_mail_password_reset_subject] =&gt; C&gt;)w&gt;&amp;q:
        [user_mail_register_admin_created_subject] =&gt; \o&#039;A&gt;&amp;nH
        [user_mail_register_no_approval_required_subject] =&gt; APXk&gt;&amp;O&#039;
        [user_mail_register_pending_approval_subject] =&gt; My1A&gt;&amp;8{
        [user_mail_status_activated_subject] =&gt; ixV4&gt;&amp;^{
        [user_mail_status_blocked_subject] =&gt; DmL!&gt;&amp;,z
        [user_mail_status_canceled_subject] =&gt; 9K1&#039;&gt;&amp;=V
    )
    </em><br/>Validation handler errors: <em class="placeholder">&#039;register_pending_approval&#039; is a required key. This value should not be null. This value should not be null.</em>
    Failed asserting that false is true.
    
  • ๐Ÿ‡ง๐Ÿ‡ชBelgium Wim Leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ
  • ๐Ÿ‡ง๐Ÿ‡ชBelgium Wim Leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

    Wim Leers โ†’ changed the visibility of the branch 3436164-add-validation-constraints-user-settings to hidden.

  • Pipeline finished with Success
    2 months ago
    Total: 619s
    #160423
  • Pipeline finished with Success
    2 months ago
    Total: 503s
    #160441
  • Status changed to Needs review 2 months ago
  • Status changed to RTBC 2 months ago
  • ๐Ÿ‡ง๐Ÿ‡ชBelgium Wim Leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ
  • Status changed to Needs work 2 months ago
  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom alexpott ๐Ÿ‡ช๐Ÿ‡บ๐ŸŒ
  • Pipeline finished with Success
    2 months ago
    Total: 592s
    #163176
  • Status changed to Needs review 2 months ago
  • Status changed to RTBC 2 months ago
  • ๐Ÿ‡ง๐Ÿ‡ชBelgium Wim Leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

    I think this is what @alexpott was asking for โ€” until #3135592: Cannot implement a custom user cancellation method โ†’ and/or โœจ Provide a Entity Handler for user cancelation Needs work happen, a truly elegant approach won't be possible anyway ๐Ÿ‘

  • Status changed to Needs work 2 months ago
  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom alexpott ๐Ÿ‡ช๐Ÿ‡บ๐ŸŒ

    Added some comments to the MR. This needs to go in 11.x first.

  • Pipeline finished with Success
    2 months ago
    Total: 571s
    #165249
  • Status changed to Needs review 2 months ago
  • Status changed to RTBC 2 months ago
  • ๐Ÿ‡ง๐Ÿ‡ชBelgium Wim Leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ
  • Status changed to Needs work about 2 months ago
  • ๐Ÿ‡ฆ๐Ÿ‡บAustralia larowlan ๐Ÿ‡ฆ๐Ÿ‡บ๐Ÿ.au GMT+10

    Looking good folks, I think we need some new tests for the constraints here.
    It would also be good to have a couple of follow ups to make this not feel so dirty in places.

  • Pipeline finished with Failed
    about 1 month ago
    Total: 1061s
    #185961
  • Pipeline finished with Canceled
    about 1 month ago
    Total: 137s
    #186773
  • Pipeline finished with Success
    about 1 month ago
    Total: 604s
    #186775
  • Status changed to Needs review about 1 month ago
  • Status changed to RTBC about 1 month ago
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States smustgrave

    Removing tests tag as they seem to be added here https://git.drupalcode.org/issue/drupal-3436164/-/jobs/1734989

    From what I can tell the feedback in threads/followups have been resolved.

    On a standard install of 11.x with config inspector installed I get all checks for user.settings.

  • Status changed to Needs work about 1 month ago
  • ๐Ÿ‡ง๐Ÿ‡ชBelgium Wim Leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

    Several nits and one commit-blocking remark.

  • ๐Ÿ‡ฎ๐Ÿ‡ณIndia kunal.sachdev

    kunal.sachdev โ†’ made their first commit to this issueโ€™s fork.

  • Pipeline finished with Success
    19 days ago
    Total: 506s
    #206631
  • Pipeline finished with Failed
    19 days ago
    Total: 199s
    #206735
  • Pipeline finished with Success
    19 days ago
    Total: 597s
    #206742
  • Pipeline finished with Failed
    19 days ago
    Total: 541s
    #206757
  • Pipeline finished with Success
    19 days ago
    Total: 594s
    #206780
  • Pipeline finished with Success
    19 days ago
    Total: 577s
    #206789
  • Status changed to Needs review 19 days ago
  • Status changed to RTBC 19 days ago
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States smustgrave

    Appears additional feedback has been addressed

    Retested using configuration inspector (see #44)

  • Status changed to Fixed 15 days ago
  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom alexpott ๐Ÿ‡ช๐Ÿ‡บ๐ŸŒ

    Committed f562df4 and pushed to 11.x. Thanks!

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

Production build 0.69.0 2024