Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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.
Comment | File | Size | Author |
---|---|---|---|
#101 | 2227381-101.patch | 40.62 KB | karishmaamin |
#100 | 2227381-100.patch | 40.48 KB | shivam-kumar |
#99 | 2227381-99.patch | 40.48 KB | Akram Khan |
#98 | 2227381-98.patch | 40.63 KB | NivethaSubramaniyan |
#97 | 2566827-97.patch | 40.84 KB | Nitin shrivastava |
Comments
Comment #1
Wim LeersComment #2
rvilarComment #4
rvilarThis patch solves a problem that don't show the username in form element. The problem was the condition to show the value.
Comment #5
rvilarAttached a patch with a first step on password widget
Comment #6
jsbalseraComment #9
marcvangendThe 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.
Comment #10
BerdirI think we can do this using #group now, see NodeformController, same for those below.
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.
That's now kind of conflicting, I understand we need max_length for the size, maybe just remove the comment?
Comment #11
BerdirFixing some stuff in the form.
Comment #13
BerdirThis will still need more work, but fixing some tests and the password widget seems to be basically working again.
Comment #15
BerdirSo much cruft in the user forms...
Comment #17
yched CreditAttribution: yched commentedCould that be done using the regular field access API ?
Comment #18
BerdirYes, I think so, but we need #2029855: Missing access control for user base fields for that, then we can clean up the access stuff.
Comment #19
yched CreditAttribution: yched commented@Berdir: awesome, thanks :-)
Comment #20
Wim Leers#17–#19: OMG AWESOME! That's the part I feared we couldn't get right, and I had no idea about that issue. Wonderful :)
Comment #21
yched CreditAttribution: yched commentedComment #22
dawehnerStarted 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.
Comment #24
dawehnerA couple of more test fixes.
Comment #25
mparker17I 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).
Comment #26
mparker17Sorry, noticed one more thing...
I think you meant to reference #2230637: Create a Language field widget and the related formatter?
Comment #28
BerdirThis is so not Novice ;)
Rerolled and fixed some tests.
Comment #30
dawehnerI'll try to work on it for a while.
Comment #31
dawehnerIt should not be much left.
Comment #32
dawehnerown-review.
Comment #35
andypostPassword is affected so summary needs update, the password field type introduced in #2338559: Never serialize password fields by default
why not use field name?
password can have a description
suppose this should be a field access care
maybe configure that in form display?
Comment #36
dawehnerJust rerolled / fix one faillure for now.
Comment #38
dawehnerJust a reroll.
Comment #40
dawehnerAlright.
Comment #41
yched CreditAttribution: yched commentedWill 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 ?
Comment #42
yched CreditAttribution: yched commentedErm, after those kind word :-/, the review :
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 ?
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 ?
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 ?
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.
$items->getEntity();
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)
Nitpick, but would be a bit clearer with a positive condition on $account->isAuthenticated() ?
$items->getEntity()
Yet the widget defines no 'size' setting.
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 ?
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
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 ?
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" ?
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.
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' ?
As pointed by @andypost - weird, why do we need to add that ?
Same, this is assumptions on the structure of the widget.
Can't we run parent::submitForm() first and then work direclty on $this->entity ?
Same, this is assumptions on the structure of the widget. Could that logic be handled in AccountForm::validate() directly ?
Comment #44
effulgentsia CreditAttribution: effulgentsia commentedSo, 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.
Comment #45
yched CreditAttribution: yched commented@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.
Comment #46
fagoI 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.
Comment #47
mikey_p CreditAttribution: mikey_p commentedI 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?
Comment #48
fagoBumped into the following problem while working on #2405943: User entity validation misses form validation logic:
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.
Comment #49
fagook, 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
Comment #50
Wim Leers@fago: so is this issue blocked on #2418103, or…?
Comment #51
dawehnerWell, I think there can be work done on this issue, regardless of the issue opened up by @fago in #49
Comment #52
daffie CreditAttribution: daffie commentedIf 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).
Comment #53
dawehnerMaybe a topic for 8.1?
Comment #54
xjmThis is a great minor version target. Fixing this would also be valuable for making entity row styles in Views more complete and robust.
Comment #55
xjmComment #62
andypostRoles also makes sense to convert - maybe separate issue
Comment #63
andypostMeanwhile "roles" is missing from editable fields because has no component in display
Also it could help REST to validate roles
Comment #64
savkaviktor16@gmail.com CreditAttribution: savkaviktor16@gmail.com at Skilld commentedRe-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
Comment #67
netw3rker CreditAttribution: netw3rker commentedNot 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!
Comment #68
Wim LeersIt would indeed be great to fix this in core :)
Comment #69
andypostneeds reroll for 8.8 core (too late for 8.7)
Comment #70
netw3rker CreditAttribution: netw3rker commentedRerolled for 8.8.x
Comment #71
MegaChriz CreditAttribution: MegaChriz as a volunteer commentedI 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.
Comment #72
vacho CreditAttribution: vacho at Skilld commentedComment #73
netw3rker CreditAttribution: netw3rker commentedI 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.
Comment #75
gnugetI think this needs a reroll, I'm going to switch back to "Needs review" so the bot can confirm this to us.
Comment #76
gnugetI 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!
Comment #77
gnugetWeird. Several tests failed (as expected) but the bot didn't return the expected message... I'm going to switch this manually to "Needs work".
Comment #78
netw3rker CreditAttribution: netw3rker commentedIt 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.
Comment #80
philltran CreditAttribution: philltran at Symmetri Technology commentedHere 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.
Comment #81
shobhit_juyal CreditAttribution: shobhit_juyal as a volunteer and at Srijan | A Material+ Company commentedHi @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.
Comment #82
philltran CreditAttribution: philltran at Symmetri Technology commentedHi @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.
Comment #83
shobhit_juyal CreditAttribution: shobhit_juyal as a volunteer and at Srijan | A Material+ Company commentedComment #84
Gábor HojtsyFixing tags.
Comment #85
dwwOr 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...
Comment #89
catchComment #90
karishmaamin CreditAttribution: karishmaamin at Specbee commentedTried re-roll of patch against 9.4.x. Please review
Comment #91
adityasingh CreditAttribution: adityasingh as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedFixed some warnings and removing the needs reroll tag. Please review.
Comment #94
tkiehne CreditAttribution: tkiehne at University of Washington commentedUsing the patch in #91 I get a couple of exceptions due to a bare / undefined constant
USERNAME_MAX_LENGTH
that should beUserInterface::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?
Comment #95
andypostSurely patch is incomplete yet
Comment #96
Nitin shrivastava CreditAttribution: Nitin shrivastava at OpenSense Labs commented#91 patch does not applied on 10.1.x , re-roll for 10.1.x
Comment #97
Nitin shrivastava CreditAttribution: Nitin shrivastava at OpenSense Labs commentedtry to fix #96
Comment #98
NivethaSubramaniyan CreditAttribution: NivethaSubramaniyan as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedFixing CCF #97
Comment #99
Akram Khanfixing #98
Comment #100
shivam-kumar CreditAttribution: shivam-kumar as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedFixed errors of #98.
Comment #101
karishmaamin CreditAttribution: karishmaamin at Specbee commentedFixed custom code error in #100