Add validation constraints to user.settings

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.

cancel_method has to be nullable according to testConfigForm

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.

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

I agree, this looks like the best way

omkar.podey → changed the visibility of the branch 3436164-add-validation-constraints to hidden.

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 ?

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.

Getting close!

99% ready!

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

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

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 😅

pradhumanjain2311 → made their first commit to this issue’s fork.

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 [] 'verify_mail' is a required key., 1 [] 'notify' is a required key., 2 [] 'register' is a required key., 3 [] 'cancel_method' is a required key., 4 [] 'password_reset_timeout' is a required key., 5 [] 'password_strength' 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.

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

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 👍

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

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.

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.

Several nits and one commit-blocking remark.

kunal.sachdev → made their first commit to this issue’s fork.

Appears additional feedback has been addressed

Retested using configuration inspector (see #44)

Committed f562df4 and pushed to 11.x. Thanks!

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