Updated: Comment #0

See Issue summary for #2010930: [META] Apply formatters and widgets to rendered entity base fields (all besides node.title).

This issue is about the User entity type's name & e-mail fields.

Remaining tasks

Fix test failures.

User interface changes

None.

API changes

None.

CommentFileSizeAuthor
#101 2227381-101.patch40.62 KBkarishmaamin
#100 2227381-100.patch40.48 KBshivam-kumar
#99 2227381-99.patch40.48 KBAkram Khan
#98 2227381-98.patch40.63 KBNivethaSubramaniyan
#97 2566827-97.patch40.84 KBNitin shrivastava
#96 2227381-96.patch41.2 KBNitin shrivastava
#91 interdiff_90-91.txt1.56 KBadityasingh
#91 2227381-91.patch48.97 KBadityasingh
#90 2227381-90.patch48.61 KBkarishmaamin
#81 issue-on-user-profile.jpg42.6 KBshobhit_juyal
#81 patch-apply-issue.jpg30.43 KBshobhit_juyal
#80 interdiff_76-80.txt49.51 KBphilltran
#80 manageable_fields-2227381-80.patch49.82 KBphilltran
#76 2227381-reroll-70.patch90.56 KBgnuget
#73 2227381-manageable_fields.interdiff.patch6.58 KBnetw3rker
#73 2227381-manageable_fields-D880.patch93.83 KBnetw3rker
#70 2227381-70.patch90.34 KBnetw3rker
#67 2227381-67.patch89.66 KBnetw3rker
#64 2227381-64.patch90.38 KBsavkaviktor16@gmail.com
#40 2227381-40.patch54.35 KBdawehner
#40 interdiff.txt2.35 KBdawehner
#38 2227381-38.patch52 KBdawehner
#36 interdiff.txt623 bytesdawehner
#36 2227381-35.patch52 KBdawehner
#32 interdiff.txt3.49 KBdawehner
#32 2227381-32.patch53 KBdawehner
#31 2227381-31.patch53.46 KBdawehner
#31 interdiff.txt9.88 KBdawehner
#28 user_fields-2227381-28-interdiff.txt4.27 KBBerdir
#28 user_fields-2227381-28.patch48.27 KBBerdir
#24 interdiff.txt8.31 KBdawehner
#24 user_fields-2227381-24.patch46.08 KBdawehner
#22 interdiff.txt13.65 KBdawehner
#22 user_widgets-2227381-22.patch41.42 KBdawehner
#15 user_base_fields_formatters_widgets-2227381-15-interdiff.txt6.33 KBBerdir
#15 user_base_fields_formatters_widgets-2227381-15.patch45.07 KBBerdir
#13 user_base_fields_formatters_widgets-2227381-13-interdiff.txt11.23 KBBerdir
#13 user_base_fields_formatters_widgets-2227381-13.patch40.62 KBBerdir
#11 user_base_fields_formatters_widgets-2227381-11-interdiff.txt4.91 KBBerdir
#11 user_base_fields_formatters_widgets-2227381-11.patch16.29 KBBerdir
#9 Workspace 1_069.png116.47 KBmarcvangend
#5 user_base_fields_formatters_widgets-2227381-5.patch13.19 KBrvilar
#4 intediff.txt796 bytesrvilar
#4 user_base_fields_formatters_widgets-2227381-3.patch10.65 KBrvilar
#1 user_base_fields_formatters_widgets-2227381-1.patch10.99 KBWim Leers
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Wim Leers’s picture

rvilar’s picture

Assigned: Unassigned » rvilar

Status: Needs review » Needs work

The last submitted patch, 1: user_base_fields_formatters_widgets-2227381-1.patch, failed testing.

rvilar’s picture

This patch solves a problem that don't show the username in form element. The problem was the condition to show the value.

rvilar’s picture

Attached a patch with a first step on password widget

jsbalsera’s picture

Status: Needs work » Needs review

The last submitted patch, 4: user_base_fields_formatters_widgets-2227381-3.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 5: user_base_fields_formatters_widgets-2227381-5.patch, failed testing.

marcvangend’s picture

FileSize
116.47 KB

The patch as it currently is, introduces a couple of differences in field order and descriptions. I currently have no time to list them all, but the attached screenshot (current HEAD on the left, patched version on the right) tells the story.

Berdir’s picture

  1. +++ b/core/modules/user/lib/Drupal/user/AccountFormController.php
    @@ -83,39 +86,18 @@ public function form(array $form, array &$form_state) {
    +    $form['account']['name'] = $form['name'];
    +    unset($form['name']);
    

    I think we can do this using #group now, see NodeformController, same for those below.

  2. +++ b/core/modules/user/lib/Drupal/user/Entity/User.php
    @@ -433,14 +433,17 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
     
    +    // @todo Use LanguageWidget once https://drupal.org/node/2226493 lands.
         $fields['langcode'] = FieldDefinition::create('language')
           ->setLabel(t('Language code'))
           ->setDescription(t('The user language code.'));
    

    Interesting, not sure if the widget is going to work here, as this is quite different from the usual content entity langcode behavior, especially the two below.

  3. +++ b/core/modules/user/lib/Drupal/user/Entity/User.php
    @@ -448,25 +451,45 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
    +        'max_length' => USERNAME_MAX_LENGTH,
    +      ))
           ->setPropertyConstraints('value', array(
             // No Length constraint here because the UserName constraint also covers
             // that.
    

    That's now kind of conflicting, I understand we need max_length for the size, maybe just remove the comment?

Berdir’s picture

Fixing some stuff in the form.

Status: Needs review » Needs work

The last submitted patch, 11: user_base_fields_formatters_widgets-2227381-11.patch, failed testing.

Berdir’s picture

This will still need more work, but fixing some tests and the password widget seems to be basically working again.

Status: Needs review » Needs work

The last submitted patch, 13: user_base_fields_formatters_widgets-2227381-13.patch, failed testing.

Berdir’s picture

So much cruft in the user forms...

Status: Needs review » Needs work

The last submitted patch, 15: user_base_fields_formatters_widgets-2227381-15.patch, failed testing.

yched’s picture

+++ b/core/modules/user/lib/Drupal/user/RegisterFormController.php
@@ -50,6 +50,10 @@ public function form(array $form, array &$form_state) {
+    $form['pass']['#access'] = !\Drupal::config('user.settings')->get('verify_mail') || $admin;

Could that be done using the regular field access API ?

Berdir’s picture

Yes, I think so, but we need #2029855: Missing access control for user base fields for that, then we can clean up the access stuff.

yched’s picture

@Berdir: awesome, thanks :-)

Wim Leers’s picture

#17–#19: OMG AWESOME! That's the part I feared we couldn't get right, and I had no idea about that issue. Wonderful :)

yched’s picture

Issue summary: View changes
dawehner’s picture

Status: Needs work » Needs review
FileSize
41.42 KB
13.65 KB

Started with a reroll together with some fixes all over the place. Let's see how many are left. The interdiff is kind of the change between
the first (bad) reroll and the patch itself.

Status: Needs review » Needs work

The last submitted patch, 22: user_widgets-2227381-22.patch, failed testing.

dawehner’s picture

Assigned: rvilar » Unassigned
Status: Needs work » Needs review
FileSize
46.08 KB
8.31 KB

A couple of more test fixes.

mparker17’s picture

+++ b/core/modules/user/src/Entity/User.php
@@ -469,25 +472,46 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
+//      ->setPropertyConstraints('value', array('UserMailUnique' => array()))

I did not do a formal code review, but while looking briefly over the code, I noticed that this line made it into the patch (a UserMailUnique constraint is set a few lines below in a different way).

mparker17’s picture

Sorry, noticed one more thing...

+++ b/core/modules/user/src/Entity/User.php
@@ -453,14 +453,17 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
+    // @todo Use LanguageWidget once https://drupal.org/node/2226493 lands.
...
+    // @todo Use LanguageWidget once https://drupal.org/node/2226493 lands.
...
+    // @todo Use LanguageWidget once https://drupal.org/node/2226493 lands.

I think you meant to reference #2230637: Create a Language field widget and the related formatter?

Status: Needs review » Needs work

The last submitted patch, 24: user_fields-2227381-24.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
Issue tags: -Novice
FileSize
48.27 KB
4.27 KB

This is so not Novice ;)

Rerolled and fixed some tests.

Status: Needs review » Needs work

The last submitted patch, 28: user_fields-2227381-28.patch, failed testing.

dawehner’s picture

I'll try to work on it for a while.

dawehner’s picture

Status: Needs work » Needs review
FileSize
9.88 KB
53.46 KB

It should not be much left.

dawehner’s picture

FileSize
53 KB
3.49 KB

own-review.

The last submitted patch, 31: 2227381-31.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 32: 2227381-32.patch, failed testing.

andypost’s picture

Password is affected so summary needs update, the password field type introduced in #2338559: Never serialize password fields by default

  1. +++ b/core/modules/user/src/AccountForm.php
    @@ -131,7 +104,7 @@ public function form(array $form, FormStateInterface $form_state) {
    -        $protected_values['mail'] = $form['account']['mail']['#title'];
    +        $protected_values['mail'] = $this->t('E-mail address');
    
    +++ b/core/modules/user/src/Entity/User.php
    @@ -468,25 +471,46 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
    +      ->setLabel(t('E-mail address'))
    

    why not use field name?

  2. +++ b/core/modules/user/src/AccountForm.php
    @@ -162,20 +135,16 @@ public function form(array $form, FormStateInterface $form_state) {
    -        '#description' => $this->t('Provide a password for the new account in both fields.'),
    ...
    +      $form['pass']['widget'][0]['value']['#description'] = $this->t('Provide a password for the new account in both fields.');
    
    +++ b/core/modules/user/src/Entity/User.php
    @@ -468,25 +471,46 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
    +      ->setDescription(t('The password of this user (hashed).'))
    

    password can have a description

  3. +++ b/core/modules/user/src/Plugin/Field/FieldWidget/UserNameWidget.php
    @@ -0,0 +1,57 @@
    +      // Only show name field on registration form or user can change own
    +      // username.
    +      '#access' => ($is_anon || ($user->id() == $account->id() && $user->hasPermission('change own username')) || $is_admin),
    

    suppose this should be a field access care

  4. +++ b/core/modules/user/src/RegisterForm.php
    @@ -66,6 +56,8 @@ public function form(array $form, FormStateInterface $form_state) {
    +    $form['name']['#access'] = TRUE;
    

    maybe configure that in form display?

dawehner’s picture

Status: Needs work » Needs review
FileSize
52 KB
623 bytes

Just rerolled / fix one faillure for now.

Status: Needs review » Needs work

The last submitted patch, 36: 2227381-35.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
52 KB

Just a reroll.

Status: Needs review » Needs work

The last submitted patch, 38: 2227381-38.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
2.35 KB
54.35 KB

Alright.

yched’s picture

Will post a review in the next comment.

But I have to say - and I'd be happy to be convinced otherwise - that after reading the patch I'm not fully convinced that using widgets for those specific fields buys us much in the end :-/

Those three fields have a *lot* of associated custom business logic, so :
- the widgets that are added here are actually very specific to those fields and couldn't be used for any other field
- and external code (the Form classes, some form helpers) still need to interact with them and make assumptions on the structure of the widgets.

So, as opposed to current HEAD where "the Form puts some harcoded $form['name'] element, and has corresponding logic at validate and submit time", we end up with code that is scattered but very coupled. Not a great win for code simplicity in the end.

The typical wins for "use widgets for XXX base field" are :
- reusability of "feature-rich" widgets, swappable by the user
- abstraction of logic and decoupling
- free integration with Quickedit / edit-in-place (which is why @Wim created filed many of those issues to begin with)

The first two don't apply here, and I'm not sure Quickedit works or even applies for user name/email/password ?

yched’s picture

Erm, after those kind word :-/, the review :

  1. +++ b/core/modules/user/src/Entity/User.php
    @@ -451,14 +451,17 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
    +    // @todo Use LanguageWidget once https://drupal.org/node/2226493 lands.
    ...
    +    // @todo Use LanguageWidget once https://drupal.org/node/2226493 lands.
    ...
    +    // @todo Use LanguageWidget once https://drupal.org/node/2226493 lands.
    

    That issue link looks stale, the correct issue is #2230637: Create a Language field widget and the related formatter - also, not really related to this issue here, do we really want to add those in this patch ?

  2. +++ b/core/modules/user/src/Entity/User.php
    @@ -467,25 +470,46 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
         $fields['mail'] = BaseFieldDefinition::create('email')
    ...
    +      ->setDisplayConfigurable('form', TRUE);
    

    The widget for 'mail' is configurable, and thus the E-mail field will appear in the "Manage forms" drag-n-drop table.
    But AccountForm moves the 'mail' entry into the $form['account'] group, which is exposed as an extra-field and thus also has its own row in "Manage forms".
    So that "Manage forms" will offer you both 'Email' and 'Account' for re-order, while one will in fact be a child of the other, so the weights assigned are going to be weird.

    Do we really have a need to make the mail widget configurable ?

  3. +++ b/core/modules/user/src/Entity/User.php
    @@ -467,25 +470,46 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
    -      ->setConstraints(array('UserMailUnique' => array()));
    +//      ->setPropertyConstraints('value', array('UserMailUnique' => array()))
    +      ->setDisplayOptions('form', array(
    +        'type' => 'email_user',
    +        'weight' => -10,
    +      ))
    +      ->setConstraints(array(
    +        'UserMailUnique' => array(),
    +      ))
    

    Commented line is commented :-)

    Also : applying UserMailUnique as a constraint to the field instead of to a property "just works", without any modification in how UserMailUnique is implemented ?

  4. +++ b/core/modules/user/config/schema/user.schema.yml
    @@ -174,3 +174,13 @@ condition.plugin.user_role:
    +entity_form_display.field.email_user:
    +  type: entity_field_form_display_base
    +  label: 'User mail widget.'
    +  mapping:
    +    settings:
    +      type: sequence
    +      label: 'Settings'
    +      sequence:
    +        - type: string
    

    Outdated - schema entries for widget settings are now field.widget.settings.[plugin_id] - but since that widget doesn't have settings, no dedicated entry is needed, the default one for 'field.widget.settings.*' is good enough.

  5. +++ b/core/modules/user/src/Plugin/Field/FieldWidget/EmailUserWidget.php
    @@ -0,0 +1,47 @@
    +    $account = $items->getParent()->getValue();
    

    $items->getEntity();

  6. +++ b/core/modules/user/src/Plugin/Field/FieldWidget/EmailUserWidget.php
    @@ -0,0 +1,47 @@
    +    $element['value'] = $element + array(
    +      '#type' => 'email',
    +      '#description' => $this->t('A valid e-mail address. All e-mails from the system will be sent to this address. The e-mail address is not made public and will only be used if you wish to receive a new password or wish to receive certain news or notifications by e-mail.'),
    

    The incoming $element has a #description already, so this one here will never be applied ?
    (PasswordWidget and UserNameWidget both have a correct workaround for that)

  7. +++ b/core/modules/user/src/Plugin/Field/FieldWidget/EmailUserWidget.php
    @@ -0,0 +1,47 @@
    +      '#default_value' => (!$account->isAnonymous() ? $account->getEmail() : ''),
    

    Nitpick, but would be a bit clearer with a positive condition on $account->isAuthenticated() ?

  8. +++ b/core/modules/user/src/Plugin/Field/FieldWidget/UserNameWidget.php
    @@ -0,0 +1,57 @@
    +    $account = $items->getParent()->getValue();
    

    $items->getEntity()

  9. +++ b/core/modules/user/src/Plugin/Field/FieldWidget/UserNameWidget.php
    @@ -0,0 +1,57 @@
    +      '#size' => $this->getSetting('size'),
    

    Yet the widget defines no 'size' setting.

  10. +++ b/core/modules/user/src/Plugin/Field/FieldWidget/UserNameWidget.php
    @@ -0,0 +1,57 @@
    +      // Only show name field on registration form or user can change own
    +      // username.
    +      '#access' => ($is_anon || ($user->id() == $account->id() && $user->hasPermission('change own username')) || $is_admin),
    

    As @andypost pointed, doesn't look like something that belongs to the widget, but is more tied to access('edit') for the user name field ?
    We'd want to enforce that on REST saves too, right ?

  11. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldWidget/PasswordWidget.php
    @@ -0,0 +1,42 @@
    + *   settings = {
    + *     "size" = ""
    + *   }
    

    Outdated, widget settings are now defined by the static defaultSettings() method now.
    + really, empty string as a default value ?

    Honestly, I'm not sure this widget really needs a 'size' setting ?

    If the settings stays, though, there should be a config schema entry for field.widget.settings.password

  12. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldWidget/PasswordWidget.php
    @@ -0,0 +1,42 @@
    +    $element['value']['#description'] = $this->t('To change the current user password, enter the new password in both fields.');
    

    This makes the widget fairly specific to the case of "the 'password' on the User entity type".
    If so, why is it provided by Core rather than user.module like the other two widgets added by the patch ?

  13. More generally, those three new widgets, in the way they work, are really specific to the case of "the name/mail/password field of the User entity type", and will break or produce unexpected behavior if applied to any other field.
    So they should use WidgetInterface::isApplicable() so that they don't show up in the UI for other fields of the same type ('string'/'email')

    We could also harmonize their plugin_id / class names / descriptions to make that clearer :
    user_name / UserNameWidget / "User name"
    user_email / UserEmailWidget / "User e-mail"
    user_password / UserPasswordWidget / "User password" ?

  14. +++ b/core/modules/user/src/AccountForm.php
    @@ -163,20 +136,16 @@ public function form(array $form, FormStateInterface $form_state) {
    +      $form['pass']['widget'][0]['value']['#description'] = $this->t('Provide a password for the new account in both fields.');
    +      $form['pass']['widget'][0]['value']['#required'] = TRUE;
    

    Hardcodes logic about the structure of the widget. Not great, but sort of acceptable since the widget for the password field is hardcoded and not configurable.

  15. +++ b/core/modules/user/src/AccountForm.php
    @@ -163,20 +136,16 @@ public function form(array $form, FormStateInterface $form_state) {
         // When not building the user registration form, prevent web browsers from
         // autofilling/prefilling the email, username, and password fields.
         if ($this->getOperation() != 'register') {
           foreach (array('mail', 'name', 'pass') as $key) {
    -        if (isset($form['account'][$key])) {
    -          $form['account'][$key]['#attributes']['autocomplete'] = 'off';
    +        if (isset($form[$key]['widget'][0]['value'])) {
    +          $form[$key]['widget'][0]['value']['#attributes']['autocomplete'] = 'off';
             }
           }
         }
    

    That, OTOH, is more problematic, since it makes assumptions on the structure of the widget used for 'mail', which is configurable and thus could change under our feet.
    As mentioned above, do we really need to allow configurability of the widget used for 'email' ?

  16. +++ b/core/modules/user/src/RegisterForm.php
    @@ -59,6 +49,8 @@ public function form(array $form, FormStateInterface $form_state) {
    +    $form['name']['#access'] = TRUE;
    

    As pointed by @andypost - weird, why do we need to add that ?

  17. +++ b/core/modules/user/src/RegisterForm.php
    @@ -78,16 +70,13 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
    -      $pass = $form_state->getValue('pass');
    +      $pass = $form_state->getValue(array('pass', 0, 'value'));
    ...
    -    $form_state->setValue('pass', $pass);
    +    $form_state->setValue(array('pass', 0, 'value'), $pass);
    

    Same, this is assumptions on the structure of the widget.
    Can't we run parent::submitForm() first and then work direclty on $this->entity ?

  18. +++ b/core/modules/user/user.module
    @@ -449,7 +452,8 @@ function user_validate_current_pass(&$form, FormStateInterface $form_state) {
    -    if ((strlen(trim($form_state->getValue($key))) > 0) && ($form_state->getValue($key) != $current_value)) {
    +    $form_value = isset($form_state->getValue($key)[0]['value']) ? $form_state->getValue($key)[0]['value'] : $form_state->getValue($key);
    +    if ((strlen(trim($form_value)) > 0) && ($form_value != $current_value)) {
    

    Same, this is assumptions on the structure of the widget. Could that logic be handled in AccountForm::validate() directly ?

Status: Needs review » Needs work

The last submitted patch, 40: 2227381-40.patch, failed testing.

effulgentsia’s picture

But I have to say - and I'd be happy to be convinced otherwise - that after reading the patch I'm not fully convinced that using widgets for those specific fields buys us much in the end...
So, as opposed to current HEAD where "the Form puts some harcoded $form['name'] element, and has corresponding logic at validate and submit time", we end up with code that is scattered but very coupled. Not a great win for code simplicity in the end.

So, I guess a question is whether we consider it acceptable for ContentEntityForm::validate() to not automatically validate every field that has been edited and thus require the specific form's validate() to do the rest. If we have any case of that being done in core, then contrib will likely also follow suit and have such cases. OTOH, if we make core strictly adhere to "use widgets everywhere so that validation is covered by the base class / form display", it'll make it easier to strongly recommend for contrib to do the same. An advantage of centralizing all validation in the field constraints + form display + form base class is that it ensures that everything you need validated is actually implemented with a field constraint. Otherwise, if people get in the habit of custom form validate() implementations, they might forget to also add the constraints to the field, leading to problems once a REST API is used on that site.

Adding a "blocker" tag to this issue, since we need to either complete it for #2002180: Entity forms skip validation of fields that are edited without widgets or decide not to and update that issue's plan. Because of the possibility of the latter, I'm not yet promoting this issue to Critical.

yched’s picture

@effulgentsia : right, forgot about #2002180: Entity forms skip validation of fields that are edited without widgets, which is another reason for moving to widgets : establish a practice that validation rules are enforced as entity/field-level constraints rather than in form logic (validate() / #validate / #element_validate in custom form code - or in widgets, for that matter)

That's a valid point. I'm not sure the patch here adresses that in its current state though - still has a fair bit of specifc logic in AccountFrom / RegisterForm and the added specialized widgets.

fago’s picture

The typical wins for "use widgets for XXX base field" are :
- reusability of "feature-rich" widgets, swappable by the user
- abstraction of logic and decoupling
- free integration with Quickedit / edit-in-place (which is why @Wim created filed many of those issues to begin with)

The first two don't apply here, and I'm not sure Quickedit works or even applies for user name/email/password ?

I think "decoupling" still applies here: Even though the code is moved into classes, it's separated from the main entity's form and could be easier swapped out or maybe even re-used. For example, a module providing e-mail verification could just swap out the email widget.
Finally, as this allows editing e.g. the user name only in a form I think this makes sense to do.

From looking at the patch, I think we can achieve a reasonable separation of code by moving description changes also in the widget. We won't be able to cover the non-existing grouping, but that's fine as it does not touch widget internals.

mikey_p’s picture

I just saw that #2403485: Complete conversion of comment form validation to entity validation was committed which just includes some logic in the entity form to check the entity validation for specific fields. Should we just leave these fields as is for now and do the same here?

fago’s picture

Bumped into the following problem while working on #2405943: User entity validation misses form validation logic:

    $user_preferred_langcode = $register ? $language_interface->getId() : $account->getPreferredLangcode();
    $user_preferred_admin_langcode = $register ? $language_interface->getId() : $account->getPreferredAdminLangcode(FALSE);

The form has hard-coded defaults for the language fields here, which in case of the preferred admin langcode does not match the field definition default value. The form should respect the default values of the fields though. I'm not sure which one is right, but when cleaning it up to use widgets I assume the problem would be solved as well.

fago’s picture

ok, opened a separate issue for it, so it can be addressed even if this goes nowhere: #2418103: User account form ignores preferred language field default values

Wim Leers’s picture

@fago: so is this issue blocked on #2418103, or…?

dawehner’s picture

Well, I think there can be work done on this issue, regardless of the issue opened up by @fago in #49

daffie’s picture

Issue tags: -beta target +Needs upgrade path, +Needs upgrade path tests

If we are going to work on #2227503: Apply formatters and widgets to Comment base fields. Should we also start working on this issue. As both are the last open child issues of #2010930: [META] Apply formatters and widgets to rendered entity base fields (all besides node.title).

dawehner’s picture

Version: 8.0.x-dev » 8.1.x-dev

Maybe a topic for 8.1?

xjm’s picture

Issue tags: +VDC

This is a great minor version target. Fixing this would also be valuable for making entity row styles in Views more complete and robust.

xjm’s picture

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

andypost’s picture

Roles also makes sense to convert - maybe separate issue

andypost’s picture

Meanwhile "roles" is missing from editable fields because has no component in display
Also it could help REST to validate roles

savkaviktor16@gmail.com’s picture

Status: Needs work » Needs review
FileSize
90.38 KB

Re-rolled, but caught fatal error whilst creating user:

Fatal error: Declaration of Drupal\Component\Utility\EmailValidator::isValid($email, Egulias\EmailValidator\Validation\EmailValidation $email_validation = NULL) must be compatible with Egulias\EmailValidator\EmailValidatorInterface::isValid($email, $checkDNS = false, $strict = false) in /var/www/drupal/core/lib/Drupal/Component/Utility/EmailValidator.php on line 12

Status: Needs review » Needs work

The last submitted patch, 64: 2227381-64.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

netw3rker’s picture

Not sure why, but this patch fails to apply to the latest 8.7.x, so here's a re-roll of this patch against the latest.

I didn't run into the same issues as reported in #65 though.

FWIW, I came across this due to issues with the Inline Entity Form module's handling of referenced users. There's a patch under relatively active development there that is totally not needed if this patch is used. (see https://www.drupal.org/project/inline_entity_form/issues/2702401)

It would be great to see this core issue help alleviate that contrib issue!

Wim Leers’s picture

Status: Needs work » Needs review

It would indeed be great to fix this in core :)

andypost’s picture

Version: 8.7.x-dev » 8.8.x-dev
Status: Needs review » Needs work
Issue tags: +Needs reroll

needs reroll for 8.8 core (too late for 8.7)

netw3rker’s picture

Rerolled for 8.8.x

MegaChriz’s picture

Issue tags: +Needs change record
+++ b/core/modules/user/tests/src/Functional/UserRolesAssignmentTest.php
@@ -46,16 +46,16 @@ public function testCreateUserWithRole() {
-      'name' => $this->randomMachineName(),
-      'mail' => $this->randomMachineName() . '@example.com',
-      'pass[pass1]' => $pass = $this->randomString(),
-      'pass[pass2]' => $pass,
+      'name[0][value]' => $this->randomMachineName(),
+      'mail[0][value]' => $this->randomMachineName() . '@example.com',
+      'pass[0][value][pass1]' => $pass = $this->randomString(),
+      'pass[0][value][pass2]' => $pass,

I see that the form field names change. So I expect that this change is going to break some tests in contrib modules, for example in modules that alter registration forms (like Email registration, for example, see https://git.drupalcode.org/project/email_registration/blob/8.x-1.x/tests...). Therefore, I think this needs a change record.

I haven't checked yet, but besides tests, it may also break more in these modules.

vacho’s picture

Issue tags: -Needs reroll
netw3rker’s picture

I did some testing of this over the past couple of weeks and noticed several interesting things / issues:

1) The most pressing of which is, the patch in #67 removes the password field on the account create and edit forms. It also pretty much breaks the confirm existing password field too.

The next big issue I noticed is that while the patch defines base fields for the entity, (which is nice, and helpful for things like my issue of inline entity forms), it does not allow the fields to be UI configurable in admin/config/people/accounts/form-display . If I want to change or rearrange the order of the account fields it's not possible. This was mentioned in #42 as problematic because of grouping. I'd suggest that grouping these fields isn't necessary & there are plenty of reasons why a user would want to completely rearrange these fields. It's a bit unfair to force our idea/notion of what fields should appear in what order, and only allow users to generate their own fields above or below this collection. Also, should grouping and internal weighting within a group be needed, there is always the Field Group module (https://www.drupal.org/project/field_group) for that.

Additionally, several of the fields that are converted from form elements to basefields have title/label text that is different than the original values. This may seem mundane, but a lot of discussion goes into things like the status field being "Status" instead of "User Status". This will also break the translations for these labels that have existed, so this would be a relatively impactful change. I would recommend that this conversion work to maintain a 1:1 mapping from old to new as much as possible.

Lastly, since this patch parts out several pieces of the "Username and Password" option in the field UI, that option is essentially redundant now. Interestingly, that field option is a bit of a misnomer anyways, since it also includes (when applicable) the Roles and Status fields. None of which are UI configurable. I'd like to discuss what to do with the now vestigial "Username and Password" base field. I'd suggest it get removed, and replaced with the actual username and password fields that are now UI configurable, but I think that's going to require an update hook.

Attached is an updated patch that makes the fields configurable via UI for the account page, removes the bug that causes password fields to disappear, and corrects some of the text issues. I've also included (my first!) interdiff between the reroll at #70, and this patch.

What still needs to be done:
1) The Changed field seems to be defined internally in a different way than the Created field. In order for field widgets to appear for this field I establish the Changed field using the definitions from the Created field.
2) The Roles field should not provide the Anonymous User role as an option, and should provide an always selected Authenticated User option. It should also default to using a checkboxes widget.
3) The "Confirm Current Password" option should be a password field, and I would suggest that the widget for it provide an option to select which fields the confirmation is being used for.
4) The description for this issue needs to be broadened to include the other user fields since this patch codes for them anyways.

Final notes on this:
I really like the idea of creating display modes that can configure these fields independently. Now that we have layout builder in place it's easier to embed things like the Create Account form into new pages, and then specify a form display mode that is tailored to that particular thing. One specific use-case I can foresee is creating a display mode for quickly creating a user account and just supplying a username, email, first and last name. and another display mode for more advanced management by other users which includes normally un editable fields such as department etc. Right now this requires a lot of code, *but* this issue/patch would allow for that to happen with just some core configuration logic.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

gnuget’s picture

Status: Needs work » Needs review

I think this needs a reroll, I'm going to switch back to "Needs review" so the bot can confirm this to us.

gnuget’s picture

I tried to reroll #73 but I wasn't able to do it, the patch didn't applied before it was published, not sure what went wrong :-/ So I rerolled #70.

@netw3rker How difficult would be to re-add your changes that you added at #73 in this new patch?

(I was also working with EFI and the referenced users as you, so I'm also interested on this :-p)

Thanks!

gnuget’s picture

Status: Needs review » Needs work

Weird. Several tests failed (as expected) but the bot didn't return the expected message... I'm going to switch this manually to "Needs work".

netw3rker’s picture

It looks like I rolled my patch against /core, rather than the root folder. if you apply it via that path does it fix your problem? if so, I'll apply it and reroll it against the parent directory.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

philltran’s picture

Here is a re-roll of the patch from #76 to include changes in #73.

I did not review the issues outlined in comment #73 yet. I will dig into that next.

shobhit_juyal’s picture

Hi @philltran,

I've tried to tested your patch at #80 on 9.1.x branch. It was not applying, might be due to some updates in the core. However, I manually fixed the issue and then created the fresh patch but then it was not showing some fields on the user profile form.

Sharing some screenshot, which might be helpful for you. Also if required, then I can share that patch which I recreated to apply the changes.

philltran’s picture

Hi @shobhit_juyal

Thanks for testing. I only rolled the patch for 8.8.x. I will see what is different in 9.1.x

Also comment #76 mentioned a few issues that still need to be resolved.

shobhit_juyal’s picture

Issue tags: +Drupal 9 compatibility Drupal 9 porting weekend DIACWMay2020
Gábor Hojtsy’s picture

Issue tags: -Drupal 9 compatibility Drupal 9 porting weekend DIACWMay2020 +Drupal 9 compatibility, +Drupal 9 porting weekend, +DIACWMay2020

Fixing tags.

dww’s picture

Or better yet... none of the above. ;) This issue has nothing to do with "D9 compatibility" nor "D9 porting". This is a regular old D9 issue...

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

catch’s picture

Issue tags: +Needs reroll
karishmaamin’s picture

Tried re-roll of patch against 9.4.x. Please review

adityasingh’s picture

Fixed some warnings and removing the needs reroll tag. Please review.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

tkiehne’s picture

Using the patch in #91 I get a couple of exceptions due to a bare / undefined constant USERNAME_MAX_LENGTH that should be UserInterface::USERNAME_MAX_LENGTH

After bypassing these I get an exception when trying to manage form settings for user accounts: "Drupal\Component\Plugin\Exception\PluginNotFoundException: The "email_user" plugin does not exist." - likely due to a schema change? Is there an update procedure for applying this to an already existing site?

andypost’s picture

Nitin shrivastava’s picture

#91 patch does not applied on 10.1.x , re-roll for 10.1.x

Nitin shrivastava’s picture

try to fix #96

NivethaSubramaniyan’s picture

Fixing CCF #97

Akram Khan’s picture

fixing #98

shivam-kumar’s picture

Fixed errors of #98.

karishmaamin’s picture

Fixed custom code error in #100

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.