| Project: | Drupal core |
| Version: | 8.x-dev |
| Component: | user.module |
| Category: | task |
| Priority: | major |
| Assigned: | Unassigned |
| Status: | needs work |
| Issue tags: | #d7ux, #d8ux, needs backport to D7, Novice, string freeze, tcdrupal2012 |
Issue Summary
Problem/Motivation
A number of core user settings are missing support for hook_field_extra_fields() to make fields easily (re)order on the Manage Fields tab. This creates unnecessary work for site developer who will have to manually patch or create custom code to fix this.
Proposed resolution
Adjust "hook_field_extra_fields" for each core module that may add something to user form (eg: Overlay, Locale, Contact, and the User module)
Remaining tasks
Patch written, tested, and committed to 8.x, however, chunk of code missing between earlier working patch (#63) and committed patch (#87) so committed patch may be incomplete.
Additional issues:
For sites that don't use it, the "Personalize blocks" fieldset will never appear on any user's account pages, but now it will appear on the "Manage Fields" screen for all Drupal sites that use the block module. As a result, we will be asking users to rearrange fields in relation to it even though it doesn't exist.
Is it possible/practical to change the description ['description' => t('Block module form element.'),] to explain that this element will not be present on all user account pages, and do the same for other elements from the patch in #63 that are not always expected to appear?
Backport to Drupal 7.
Determine what code is missing (as per #71).
User interface changes
This patch makes it possible to reorder the settings form for Picture, Signature, Overlay, Contact and Blocks. With fieldgroup module installed it could be possible to customize user account edit page.
API changes
Original report by DocuAnt
A number of core user settings are missing support for hook_field_extra_fields() to make them easily organized on the Manage Fields tab. This makes them appear randomly as soon as any reorganizing is made. It also prevents them from being put in for example tabs using the Field Group contrib module.
In short, it severely blocks the intended use of the Manage Fields tab to be able to organize as theme user profile editing and view without the need of custom coding.
The form fields the patch in #33 enables this for is:
- block
- contact
- locale
- overlay_control
- picture (user picture upload)
- signature_settings
Practically any site built with Drupal will use at least one of the above form fields. Thus, creating a lot of extra, unnecessary work for site developer having to manually patch or create custom code to fix this.
| Attachment | Size | Status | Test result | Operations |
|---|---|---|---|---|
| Test_Drupal7b2_UserFields.png | 56.95 KB | Ignored: Check issue status. | None | None |
Comments
#1
Following patch in #988974: Picture and signature forms cannot be reordered on user account settings / manage fields, the solution would be to adjust "hook_field_extra_fields" for each core module that may add something to user form, right? AFAIK: Overlay, Locale, Contact, and the User module itself.
#2
Version change from 7.0-beta2 to 7.x-dev because the problem is still there in 7.0-beta3 and 7.x-rc1.
#3
reattaching patch from #988974: Picture and signature forms cannot be reordered on user account settings / manage fields
#4
#5
The last submitted patch, 967566_3.patch, failed testing.
#6
Re-rolled against HEAD. This patch allows UserName/Password and UserPicture forms to be reordered. I tried similar approach with contact form settings, but I failed. The question could be, if we need to reorder this form, or just to put it on the bottom. Maybe we can fix the position of system forms, like Administrative overlay, Contact form, Personalize Blocks, Locale Settings? I would also prefer to have them collapsed by default, since adding more and more custom fields will end up with long long user account edit page... It is too late to put such things into i.e. vertical tabs and collapsing them is just a small change without impact to API, but from the UX point of view might be really helpful.
#7
#8
The last submitted patch, 967566_5.patch, failed testing.
#9
This patch makes it possible to reorder settings form for Picture, Signature, Overlay, Contact and Blocks. I rerolled against current HEAD. With fieldgroup module installed it could be possible to totally customize user account edit page.
#10
The last submitted patch, 967566_9.patch, failed testing.
#11
What about now, Dear Bot?
#12
tagging it, since the patch adds some description text
#13
Just polished the patch. This time, the weights are set to 0 by default. I'm attaching also the screenshots. This needs review. On all my D7dev installations it works nice. To summarize:
- Adds the missing dot to the end of Username/Password description
and allows to reorder the following forms on user account manage fields tab:
- User module: Picture/Signature forms
- Block module: Personalize Blocks form
- Overlay module: Administrative Overlay form
- Contact module: Personal contact form
#14
And screenshots
#15
While this is nice to have, it's a new feature rather than a bug.
#16
Well, I still think, this is not a feature request. I agree that this not so major, but, from the UX point of view, while we have so many things polished and configurable, the user account edit page with fields added will look totally messy, just because some forms have hardcoded weights and some not. My question is: why we allow to reorder username/password and timezone forms and we cannot reorder picture/signature forms on the user account manage fields tab? And it seems to be inconsistent, since those forms comes from the same module. I know the workaround for that problem, as described in op in #988974: Picture and signature forms cannot be reordered on user account settings / manage fields, but I can imagine number of sites, where possibility of reordering system forms on user account edit page can be crucial. People will ask, why the picture form is destroying the look of account edit page (screenshot in op). Having fields on user account is such great possibility, not only for providing information, but also, combining with Fieldgroup module, to redesign the look and feel on the account edit page. This is important to have a better first impression, especially for new users coming to your site.
The old Profile module allowed to reorder all custom stuff, and this works with fields, too. This time, however, we have to deal with system forms (user, contact, overlay, block and system modules) on the same page, since we don't have a separate tab for profile fields. That's why the support for reordering forms on user account edit page is missing.
#17
#11: 967566_10.patch queued for re-testing.
#18
#13: 967566_13.patch queued for re-testing.
#19
The last submitted patch, 967566_13.patch, failed testing.
#20
re-rolled #13. I also created a small module which does such thing, however, I still think, it should be in Core, see https://github.com/mslonina/d8ux/tree/master/d8ux_account
#21
#22
The problem is quite simple.
Your patch adds new translatable strings. And that is not allowed in a stable version. So I agree that this would be nice to have (I actually found that we're missing this for Privatemsg but it's exactly the same problem there: Privatemsg 7.x-1.0 is stable)
Note that it must not be originating module that declares extra fields. You could do this in a simple contrib module. I fully agree that it is crap to have a module enabled just for this but that's probably how it will have to be in 7.x.
+++ modules/user/user.module@@ -215,6 +215,26 @@ function user_field_extra_fields() {
+ if (variable_get('user_pictures', 1) == 1) { ¶
+ $return['user']['user']['form'] += array(
+ 'picture' => array(
+ 'label' => 'User picture',
+ 'description' => t('User module picture form element.'),
+ 'weight' => 0,
+ ), ¶
+ ); ¶
+ }
+ ¶
+ if (variable_get('user_signatures', 1) == 1) { ¶
+ $return['user']['user']['form'] += array(
+ 'signature_settings' => array(
+ 'label' => 'User signature',
+ 'description' => t('User module signature form element.'),
+ 'weight' => 0,
+ ), ¶
+ ); ¶
+ }
+
1000 trailing spaces :)
Powered by Dreditor.
#23
Fixed trailing spaces.
And I did this in a small contrib module, too... currently I posted it on github, probably I need to apply for cvs account on d.o
#24
The last submitted patch, 967566_23.patch, failed testing.
#25
Forgot about --no-prefix when generating patch.
#26
#27
This issue has been really bugging me. It's great to be able to add user fields with core, but the account editing form becomes such a mess! I just tried using @mariusz.slonina's little module mentioned in #20 (thanks!), but I got this error on a WSOD:
Fatal error: Unsupported operand types in mysite/modules/field_ui/field_ui.admin.inc on line 456
#28
Once I disabled the d8ux module and enabled the d8ux_account module only, it seems to be working fine. Thank you!
#29
Thanks, the module is not finished, and to be honest, I didn't have much time for it. But since my new project needs this feature, and D.o is moving to git, I'll probably post it here
#30
Please, test the recent revision posted on Github, I did some cleanups, and moved d8ux_account to d8ux. Thanks :)
#31
I have installed your new version, @mariusz.slonina, and it still seems to be working fine. FWIW, I'm also using Field Groups, Profile2, Comment Notify, and User Locations, and there are no apparent conflicts. The Location section and the Comment Notify section aren't available for weighting by d8ux, but they are in .dev or .alpha versions.
#32
It's because Location/Comment notify should define own hook_field_extra_fields() -- my patch/module is only for core modules. Thanks for testing :)
Patch #25 is still valid for current 7-dev.
#33
This patch adds missing form for Language settings (Locale module).
#34
Please, see http://drupal.org/sandbox/mariusz.slonina/1075174 for a sandbox D8UX module
#35
Moving to the 8.x queue it certainly needs to be fixed there first, I still fear that this can't be backported but I'd be happy to be proven otherwise.
#36
#37
#33: 967566_33.patch queued for re-testing.
#38
Subscribe.
#39
tagging for D8
#40
Guys, patch #33 contains such lines of codes $arr += arrray(...). Do you think it will be working good in php 5.3? I'ts better to use array_merge instead.
#41
@dealancer: += is not the same as array_merge, and yes, both are supported by PHP 5.3.
#42
How to change the #weight of contact, locale, language fieldsets on user edit page from form_user_profile_form_alter? I cant find these fields in $form array, they are empty.
#43
@drynok: that's because hook_form_FORM_ID_alter() is called before hook_form_alter(). locale.module adds the language widget in hook_form_alter(), so you have to use that hook, too. And you need to make sure that your module comes after locale.module (increase the module weight of your module).
#44
I'm sorry, that comment was not correct: of course the more general hook_form_alter() is called before hook_form_FORM_ID_alter(), and only in the scope of one module. So you just have to increase the weight of your module.
#45
Marking #1176556: Make user profile fields available on the Manage Fields page and #1176562: Make all Core fields available in Manage Fields/Display as needed to this one. Both contains patches as well.
Also bumping it to major as it severely breaks the core features to organize users settings and the display of user profiles.
Haven't tested the patch in #33, but noticed that weight are mostly set to 0. IMO, they should have sensible values so the fields are presented in a logical order by default. Overlay for example has a weight of 4 for display on the edit page. Same weight should be used as default value for the Manage Fields tab as well.
#46
#47
For one site where profiles/editing is a core feature, I ended up simply putting together a 400-line long hook_form_alter to seriously clean up the form. Hope this can get fixed for D8.
#48
This is more task than bug, core should be consistent with this though I agree so not dropping from major.
#49
Rerolled mariusz.slonina's patch from #33, but added some default weights based on each module's current form_alter. The correct order's obviously up for debate, but I don't think it should hold us back from getting this in. Also, I removed changes to system.admin.inc that I don't think was intended in #33.
#50
The last submitted patch, 967566_48.patch, failed testing.
#51
Trying again.
#52
#53
Hi, thanks, the system part was intended to add "Show/Hide descriptions" link on more adminstration pages, IMHO for better UX -- think you click once to hide descriptions, then forget about it, and then try to find out where descriptions are... However, that's separate issue right now, we should move extra_fields forward, and if possible backport it do D7, since it is criticial if you have a lot of fields in user profiles (i.e. my current project).
#54
Tagging.
#55
Patch looks as good as it can. Also tested and found working fine. Applies to Drupal 7 with some offsets too, so can do a direct backport commit too. Here is a screenshot showing the effect of the patch applied. Note that the patch merely lets people reorder these elements on the user form:
#56
Really looking forward to when this lands in D7. Going to make things so much easier to maintain.
#57
Patch #55 works nicely on 7.8. This is a nice change when using a lot of fields.
#58
I have applied 967566_50.patch to D7.8 with success as well. Thank you!
#59
We're missing
t()functions around thelabelfields, no?#60
User module omits the t() http://api.drupal.org/api/drupal/modules--user--user.module/function/use...
But the generic documentation includes it http://api.drupal.org/api/drupal/modules--field--field.api.php/function/...
Need another issue to sort that out.
#61
@bfroehle: looks like user.module got fixed in the meantime? (I'm seeing it uses t() now at http://api.drupal.org/api/drupal/modules--user--user.module/function/use...). Agreed, we need t() on the labels.
#62
Yes, fixed very recently in #1253204: user_field_extra_fields() does not translate labels.
#63
Updated patch with t()
Cheers.
#64
Patch in #63 tested against 7.9-dev and works as it should.
People, lets hurry up with this one so we can get it into 7.9 please.
#65
This definitely won't get into 7.9 - that's in code freeze until the release except for any critical regressions found since yesterday.
I'll make sure I give this a proper look by the end of the week though, first glance looks fine to me.
#66
+++ b/modules/contact/contact.moduleundefined@@ -255,3 +255,16 @@ function contact_form_user_admin_settings_alter(&$form, &$form_state) {
\ No newline at end of file
Probably need a newline at the end of the file here.
Not going to change the status over that though. :P
#67
@catch: Oki, just found out how the code freeze works. Thanks for having a look at it so we can get it in the next release. It will for sure take away a major annoyance for me when it does...
#68
Thanks for working on this patch. It could use an automated test before it is committed. If you need help you can ask the friendly people in the #drupal-contribute IRC channel.
#69
Darn. Looks like we missed getting this one in again. I haven't had time to learn how to write the needed tests, but if everything goes well I will hopefully be able to ask the friendly people in the #drupal-contribute channel later this week for some pointers on how to do it. Just need to get my local dev server up and running first...
#70
Taggin cos needs test
#71
It seems like most of the items in the #63 patch has been added to the modules. Only the block.module is left unpatched. I have attached a new patch that only includes that part.
Oddly though, they don't show up as expected in the Manage Fields tab...
Also, none of them have so far been backported to Drupal 7...
#72
I applied @tsvenson's patch, which works and displayed "Personalize blocks" field element. I added following test implementation which passed all the way down except finding the "Personalize blocks" string. Oddly though the patch in #63 didn't work in D8.
#73
The last submitted patch, block_extra_field-simpletest-967566-72.patch, failed testing.
#74
Setting back to Needs review for #71
#75
test for "Personalize blocks" field element now works
#76
+++ b/core/modules/block/block.testundefined@@ -844,3 +844,27 @@ class BlockHiddenRegionTestCase extends DrupalWebTestCase {
+ $this->assertText('Personalize blocks', 'Personalized block not found');
This message is what the expected behavior should be, not the fail. So "Personalized block is present." or something similar.
Otherwise this looks great, the test fails as expected, and the hook is correct.
#77
Indeed the text was wrong. Didn't notice this as I was running the test from the command line and don't see these messages with drush test-run?
#78
Thanks folks! That test looks sufficient to me too. I reviewed the latest patch and found a few more minor things to fix:
+++ b/core/modules/block/block.testundefined@@ -844,3 +844,27 @@ class BlockHiddenRegionTestCase extends DrupalWebTestCase {
+ * Test the block settings in people's accounts.
This should begin with a third-person verb (so "tests" rather than "test").
Also, I'd say something a little more specific (and refer to user accounts rather than to "people"). Maybe:
Tests personalized block settings for user accounts.+++ b/core/modules/block/block.testundefined@@ -844,3 +844,27 @@ class BlockHiddenRegionTestCase extends DrupalWebTestCase {
+ 'name' => 'People Settings',
"People settings" doesn't really make sense to me. Can we come up with a slightly better name for this test case? :)
+++ b/core/modules/block/block.testundefined@@ -844,3 +844,27 @@ class BlockHiddenRegionTestCase extends DrupalWebTestCase {
+ 'description' => t("Test the block settings in people's accounts."),
This description should not be translated. Reference: http://drupal.org/simpletest-tutorial-drupal7#t
Also, the same note from the docblock also applies to the description here.
+++ b/core/modules/block/block.testundefined@@ -844,3 +844,27 @@ class BlockHiddenRegionTestCase extends DrupalWebTestCase {
+ $this->assertText('Personalize blocks', 'Personalized block is present.');
I think the first argument needs to be translated with
t(), because it is matching an actual UI string provided by the Block module. (The second string, the assertion message, can remain untranslated.) Reference: http://drupal.org/simpletest-tutorial-drupal7#tAdding the novice tag because these are easy cleanups. Thanks!
#79
#80
Alright, this looks ready to me, and it's been around the block a few times. Thanks everyone!
#81
+++ b/core/modules/block/block.test@@ -844,3 +844,27 @@ class BlockHiddenRegionTestCase extends DrupalWebTestCase {
+class BlockUserAccountSettingsTestCase extends DrupalWebTestCase {
+ public static function getInfo() {
+ return array(
...
+ );
+ }
Wrong indentation (only one space on the return level).
+++ b/core/modules/block/block.test@@ -844,3 +844,27 @@ class BlockHiddenRegionTestCase extends DrupalWebTestCase {
+ 'group' => 'Block'
Missing trailing comma.
+++ b/core/modules/block/block.test@@ -844,3 +844,27 @@ class BlockHiddenRegionTestCase extends DrupalWebTestCase {
+ $admin_user = $this->drupalCreateUser(array('administer blocks', 'access administration pages', 'administer users'));
I don't think that administer blocks is required for the extra field to appear?
access administration pages is probably not needed, too.
+++ b/core/modules/block/block.test@@ -844,3 +844,27 @@ class BlockHiddenRegionTestCase extends DrupalWebTestCase {
+ function testAccountSettingsPage() {
Missing phpDoc.
#82
Wow, not sure how I missed the missing docblock. Twice, no less. Patch attached as penance.
Attached grants only
administer userspermission. Didn't run the test locally. Go, bot, go!Edit: the return indentation is still wrong.
#84
Sorry, testbot.
#85
Needed reroll after #135464: Add language options to block visibility.
#86
The last submitted patch, drupal-967566-86-combined.patch, failed testing.
#87
I clearly missed the memo on s/DrupalWebTestCase/WebTestBase...
#88
Thanks. Good to go if bot comes back green.
#89
Committed to 8.x. Thanks!
#90
It looks like #63 was never committed, so the patch committed in #87 was incomplete, or am I missing something?
#91
I think @catch is right - a lot of code disappeared between the patches in #63 and #71 and wasn't ever added anywhere else.
Also:
+ 'label' => t('Personalize blocks'),+ 'description' => t('Block module form element.'),
So, this is really unfortunate, because only about 0.00001% of Drupal sites actually use the "Personalize blocks" feature (and that's not much of an exaggeration). For sites that don't use it, the "Personalize blocks" fieldset will never appear on any user's account pages. But now, we're putting this on the "Manage Fields" screen for 100% of Drupal sites that use the block module, at which point we're asking everyone to rearrange fields in relation to it even though it doesn't actually exist. This is confusing.
Maybe we can at least change the description above to explain that this element will not be present on all user account pages? (And the same thing for other elements from the patch in #63 that are not always expected to appear.)
#92
working on issue summary
#93
added new issue summary