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.

Files: 
CommentFileSizeAuthor
#194 html_array_error.jpg164.81 KBfstrevisan_eZly
#186 drupal7-user-extra-fields-967566-186.patch3.72 KBDavid_Rothstein
PASSED: [[SimpleTest]]: [MySQL] 40,082 pass(es).
[ View ]
#186 interdiff-176-186.txt1.12 KBDavid_Rothstein
#176 drupal7-user-extra-fields.176.patch3.72 KBanon
PASSED: [[SimpleTest]]: [MySQL] 40,503 pass(es).
[ View ]
#170 drupal7-user-extra-fields.170.patch4.32 KBtsvenson
PASSED: [[SimpleTest]]: [MySQL] 39,694 pass(es).
[ View ]
#164 user_fields_display_artifacts.png22.47 KBtsvenson
#162 user_fields_stdinst_without_patch.png8.06 KBtsvenson
#162 user_fields_stdinst_with_patch.png13.68 KBtsvenson
#162 user_fields_stdinst_overlay_with_patch.png13.1 KBtsvenson
#160 drupal8.user-extra-fields.160.patch3.16 KBsun
PASSED: [[SimpleTest]]: [MySQL] 50,570 pass(es).
[ View ]
#159 drupal8.user-extra-fields.159.patch1.67 KBsun
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#151 drupal8.user-extra-fields.151.patch3.17 KBsun
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal8.user-extra-fields.151.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#141 967566-132-Implement-hook_field_extra_fields-to-provide-user-se_tests.patch5.01 KBFabianx
FAILED: [[SimpleTest]]: [MySQL] 48,760 pass(es), 4 fail(s), and 0 exception(s).
[ View ]
#141 967566-132-Implement-hook_field_extra_fields-to-provide-user-se.patch9.13 KBFabianx
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 967566-132-Implement-hook_field_extra_fields-to-provide-user-se_0.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#132 967566-132-Implement-hook_field_extra_fields-to-provide-user-se.patch9.13 KBLetharion
PASSED: [[SimpleTest]]: [MySQL] 40,432 pass(es).
[ View ]
#132 967566-132-Implement-hook_field_extra_fields-to-provide-user-se_tests.patch5.01 KBLetharion
FAILED: [[SimpleTest]]: [MySQL] 40,425 pass(es), 4 fail(s), and 0 exception(s).
[ View ]
#131 967566-131-Implement-hook_field_extra_fields-to-provide-user-se_tests.patch5.01 KBLetharion
FAILED: [[SimpleTest]]: [MySQL] 40,421 pass(es), 4 fail(s), and 0 exception(s).
[ View ]
#131 967566-131-Implement-hook_field_extra_fields-to-provide-user-se.patch9.12 KBLetharion
FAILED: [[SimpleTest]]: [MySQL] 40,416 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#129 967566-129-Implement-hook_field_extra_fields-to-provide-user-se_tests.patch1.86 KBLetharion
FAILED: [[SimpleTest]]: [MySQL] 40,415 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#124 967566-124-Implement-hook_field_extra_fields-to-provide-user-se.patch6 KBLetharion
PASSED: [[SimpleTest]]: [MySQL] 40,355 pass(es).
[ View ]
#123 967566-123-Implement-hook_field_extra_fields-to-provide-user-se_tests.patch1.86 KBLetharion
PASSED: [[SimpleTest]]: [MySQL] 40,360 pass(es).
[ View ]
#122 967566-122-Implement-hook_field_extra_fields-to-provide-user-se.patch4.38 KBLetharion
PASSED: [[SimpleTest]]: [MySQL] 40,354 pass(es).
[ View ]
#120 967566-120-Implement-hook_field_extra_fields-to-provide-user-se.patch4.44 KBLetharion
PASSED: [[SimpleTest]]: [MySQL] 40,364 pass(es).
[ View ]
#112 967566-112-Implement-hook_field_extra_fields-to-provide-user-se.patch4.49 KBLetharion
PASSED: [[SimpleTest]]: [MySQL] 40,332 pass(es).
[ View ]
#107 967566-107-Implement-hook_field_extra_fields-to-provide-user-se.patch3.49 KBLetharion
PASSED: [[SimpleTest]]: [MySQL] 40,335 pass(es).
[ View ]
#105 967566-105-Implement-hook_field_extra_fields-to-provide-user-se.patch3.42 KBLetharion
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 967566-105-Implement-hook_field_extra_fields-to-provide-user-se.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#101 user_account_extra_fields_before.png27.04 KBtsvenson
#100 user_account_manage_fields.png39 KBtsvenson
#100 967566-extra_fields-100.patch3.42 KBtsvenson
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 967566-extra_fields-100.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#99 967566-100.patch2.19 KBvalthebald
PASSED: [[SimpleTest]]: [MySQL] 40,570 pass(es).
[ View ]
#97 967566-98.patch2.8 KBvalthebald
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/block/block.module.
[ View ]
#87 drupal-967566-87-test.patch1.11 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 36,600 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#87 drupal-967566-87-combined.patch1.75 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-967566-87-combined.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#85 drupal-967566-86-tests.patch1.12 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed during invocation of run-tests.sh.
[ View ]
#85 drupal-967566-86-combined.patch1.76 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed during invocation of run-tests.sh.
[ View ]
#84 967566-84.patch1.77 KBxjm
PASSED: [[SimpleTest]]: [MySQL] 35,095 pass(es).
[ View ]
#84 interdiff-79-84.txt1.25 KBxjm
#82 967566-82-test.patch1.12 KBxjm
FAILED: [[SimpleTest]]: [MySQL] 35,079 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#82 967566-82-complete.patch1.77 KBxjm
PASSED: [[SimpleTest]]: [MySQL] 35,087 pass(es).
[ View ]
#82 interdiff.txt1.01 KBxjm
#79 block-extra-967566-79.patch1.75 KBoriol_e9g
PASSED: [[SimpleTest]]: [MySQL] 35,927 pass(es).
[ View ]
#79 interdiff.txt877 bytesoriol_e9g
#77 block_extra_field-simpletest-967566-77-test.patch1.09 KBjoestewart
FAILED: [[SimpleTest]]: [MySQL] 35,721 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#77 block_extra_field-simpletest-967566-77.patch1.76 KBjoestewart
PASSED: [[SimpleTest]]: [MySQL] 35,720 pass(es).
[ View ]
#75 block_extra_field-simpletest-967566-75-test.patch1.09 KBjoestewart
FAILED: [[SimpleTest]]: [MySQL] 35,699 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#75 block_extra_field-simpletest-967566-75.patch1.76 KBjoestewart
PASSED: [[SimpleTest]]: [MySQL] 35,702 pass(es).
[ View ]
#72 block_extra_field-simpletest-967566-72.patch1.71 KBadnasa
FAILED: [[SimpleTest]]: [MySQL] 33,963 pass(es), 1 fail(s), and 0 exception(es).
[ View ]
#71 block_extra_field-967566-71.patch689 bytestsvenson
PASSED: [[SimpleTest]]: [MySQL] 33,360 pass(es).
[ View ]
#63 967566-extra_fields-63.patch3.43 KBindytechcook
PASSED: [[SimpleTest]]: [MySQL] 33,634 pass(es).
[ View ]
#55 UserFormElements.png138.76 KBGábor Hojtsy
#51 967566_50.patch3.73 KBrbayliss
PASSED: [[SimpleTest]]: [MySQL] 33,644 pass(es).
[ View ]
#49 967566_48.patch3.97 KBrbayliss
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 967566_48.patch. See the log in the details link for more information.
[ View ]
#33 967566_33.patch4.66 KBmariusz.slonina
PASSED: [[SimpleTest]]: [MySQL] 29,411 pass(es).
[ View ]
#25 967566_24.patch4 KBmariusz.slonina
PASSED: [[SimpleTest]]: [MySQL] 31,464 pass(es).
[ View ]
#23 967566_23.patch4.04 KBmariusz.slonina
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 967566_23.patch.
[ View ]
#20 967566_20.patch3.33 KBmariusz.slonina
PASSED: [[SimpleTest]]: [MySQL] 31,452 pass(es).
[ View ]
#14 967566_before.png82.12 KBmariusz.slonina
#14 967566_13.png93.14 KBmariusz.slonina
#13 967566_13.patch3.33 KBmariusz.slonina
FAILED: [[SimpleTest]]: [MySQL] 30,415 pass(es), 6 fail(s), and 1 exception(es).
[ View ]
#11 967566_10.patch2.94 KBmariusz.slonina
PASSED: [[SimpleTest]]: [MySQL] 30,414 pass(es).
[ View ]
#9 967566_9.patch2.97 KBmariusz.slonina
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 967566_9.patch.
[ View ]
#6 967566_5.patch2.32 KBmariusz.slonina
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 967566_5.patch.
[ View ]
#3 967566_3.patch1.62 KBmariusz.slonina
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 967566_3_1.patch.
[ View ]
Test_Drupal7b2_UserFields.png56.95 KBDocuAnt

Comments

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.

Version:7.0-beta2» 7.x-dev

Version change from 7.0-beta2 to 7.x-dev because the problem is still there in 7.0-beta3 and 7.x-rc1.

StatusFileSize
new1.62 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 967566_3_1.patch.
[ View ]
new1.62 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 967566_3_0.patch.
[ View ]

Status:Active» Needs review

Status:Needs review» Needs work

The last submitted patch, 967566_3.patch, failed testing.

StatusFileSize
new2.32 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 967566_5.patch.
[ View ]

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.

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, 967566_5.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new2.97 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 967566_9.patch.
[ View ]

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.

Status:Needs review» Needs work

The last submitted patch, 967566_9.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new2.94 KB
PASSED: [[SimpleTest]]: [MySQL] 30,414 pass(es).
[ View ]

What about now, Dear Bot?

Issue tags:+string freeze

tagging it, since the patch adds some description text

StatusFileSize
new3.33 KB
FAILED: [[SimpleTest]]: [MySQL] 30,415 pass(es), 6 fail(s), and 1 exception(es).
[ View ]

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

StatusFileSize
new93.14 KB
new82.12 KB

And screenshots

Component:field_ui.module» user.module
Category:bug» task
Priority:Major» Normal

While this is nice to have, it's a new feature rather than a bug.

Category:task» bug
Issue tags:+#d7ux

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.

Issue tags:-string freeze, -#d7ux

#11: 967566_10.patch queued for re-testing.

#13: 967566_13.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+string freeze, +#d7ux

The last submitted patch, 967566_13.patch, failed testing.

StatusFileSize
new3.33 KB
PASSED: [[SimpleTest]]: [MySQL] 31,452 pass(es).
[ View ]

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

Status:Needs work» Needs review

Status:Needs review» Needs work

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.

Status:Needs work» Needs review
StatusFileSize
new4.04 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 967566_23.patch.
[ View ]

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

Status:Needs review» Needs work

The last submitted patch, 967566_23.patch, failed testing.

StatusFileSize
new4 KB
PASSED: [[SimpleTest]]: [MySQL] 31,464 pass(es).
[ View ]

Forgot about --no-prefix when generating patch.

Status:Needs work» Needs review

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

Once I disabled the d8ux module and enabled the d8ux_account module only, it seems to be working fine. Thank you!

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

Please, test the recent revision posted on Github, I did some cleanups, and moved d8ux_account to d8ux. Thanks :)

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.

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.

StatusFileSize
new4.66 KB
PASSED: [[SimpleTest]]: [MySQL] 29,411 pass(es).
[ View ]

This patch adds missing form for Language settings (Locale module).

Please, see http://drupal.org/sandbox/mariusz.slonina/1075174 for a sandbox D8UX module

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.

Version:7.x-dev» 8.x-dev

#33: 967566_33.patch queued for re-testing.

Subscribe.

Issue tags:+#d8ux

tagging for D8

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.

@dealancer: += is not the same as array_merge, and yes, both are supported by PHP 5.3.

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.

@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).

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.

Priority:Normal» Major
Status:Needs review» Needs work

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.

Title:Can't control the position of Contact, Locale or Language settingsSeveral important Core User settings lack support for hook_field_extra_fields()

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.

Category:bug» task

This is more task than bug, core should be consistent with this though I agree so not dropping from major.

Status:Needs work» Needs review
StatusFileSize
new3.97 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 967566_48.patch. See the log in the details link for more information.
[ View ]

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.

Status:Needs review» Needs work

The last submitted patch, 967566_48.patch, failed testing.

StatusFileSize
new3.73 KB
PASSED: [[SimpleTest]]: [MySQL] 33,644 pass(es).
[ View ]

Trying again.

Status:Needs work» Needs review

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).

Tagging.

Status:Needs review» Reviewed & tested by the community
Issue tags:+needs backport to D7
StatusFileSize
new138.76 KB

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:

Really looking forward to when this lands in D7. Going to make things so much easier to maintain.

Patch #55 works nicely on 7.8. This is a nice change when using a lot of fields.

I have applied 967566_50.patch to D7.8 with success as well. Thank you!

Status:Reviewed & tested by the community» Needs review

We're missing t() functions around the label fields, no?

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.

@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.

StatusFileSize
new3.43 KB
PASSED: [[SimpleTest]]: [MySQL] 33,634 pass(es).
[ View ]

Updated patch with t()

Cheers.

Status:Needs review» Reviewed & tested by the community

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.

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.

+++ 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

@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...

Status:Reviewed & tested by the community» Needs work

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.

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...

Issue tags:+Needs tests

Taggin cos needs test

Status:Needs work» Needs review
StatusFileSize
new689 bytes
PASSED: [[SimpleTest]]: [MySQL] 33,360 pass(es).
[ View ]

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...

StatusFileSize
new1.71 KB
FAILED: [[SimpleTest]]: [MySQL] 33,963 pass(es), 1 fail(s), and 0 exception(es).
[ View ]

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.

Status:Needs review» Needs work

The last submitted patch, block_extra_field-simpletest-967566-72.patch, failed testing.

Status:Needs work» Needs review

Setting back to Needs review for #71

StatusFileSize
new1.76 KB
PASSED: [[SimpleTest]]: [MySQL] 35,702 pass(es).
[ View ]
new1.09 KB
FAILED: [[SimpleTest]]: [MySQL] 35,699 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

test for "Personalize blocks" field element now works

Status:Needs review» Needs work
Issue tags:-Needs tests

+++ 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.

Status:Needs work» Needs review
StatusFileSize
new1.76 KB
PASSED: [[SimpleTest]]: [MySQL] 35,720 pass(es).
[ View ]
new1.09 KB
FAILED: [[SimpleTest]]: [MySQL] 35,721 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

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?

Status:Needs review» Needs work
Issue tags:+Novice

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#t

Adding the novice tag because these are easy cleanups. Thanks!

Status:Needs work» Needs review
StatusFileSize
new877 bytes
new1.75 KB
PASSED: [[SimpleTest]]: [MySQL] 35,927 pass(es).
[ View ]

Status:Needs review» Reviewed & tested by the community

Alright, this looks ready to me, and it's been around the block a few times. Thanks everyone!

Status:Reviewed & tested by the community» Needs work

+++ 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.

Status:Needs work» Needs review
StatusFileSize
new1.01 KB
new1.77 KB
PASSED: [[SimpleTest]]: [MySQL] 35,087 pass(es).
[ View ]
new1.12 KB
FAILED: [[SimpleTest]]: [MySQL] 35,079 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Wow, not sure how I missed the missing docblock. Twice, no less. Patch attached as penance.

Attached grants only administer users permission. Didn't run the test locally. Go, bot, go!

Edit: the return indentation is still wrong.

StatusFileSize
new1.25 KB
new1.77 KB
PASSED: [[SimpleTest]]: [MySQL] 35,095 pass(es).
[ View ]

Sorry, testbot.

StatusFileSize
new1.76 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed during invocation of run-tests.sh.
[ View ]
new1.12 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed during invocation of run-tests.sh.
[ View ]

Status:Needs review» Needs work

The last submitted patch, drupal-967566-86-combined.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new1.75 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-967566-87-combined.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new1.11 KB
FAILED: [[SimpleTest]]: [MySQL] 36,600 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

I clearly missed the memo on s/DrupalWebTestCase/WebTestBase...

Status:Needs review» Reviewed & tested by the community

Thanks. Good to go if bot comes back green.

Status:Reviewed & tested by the community» Fixed

Committed to 8.x. Thanks!

Status:Fixed» Needs work

It looks like #63 was never committed, so the patch committed in #87 was incomplete, or am I missing something?

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.)

Assigned:Unassigned» amyhribar
Issue tags:+tcdrupal2012

working on issue summary

Issue summary:View changes

Rewrite the issue summary to better explain what this patch is doing.

Issue summary:View changes

updated issue summary

Assigned:amyhribar» Unassigned
Issue tags:-Needs issue summary update

added new issue summary

Status:Needs work» Needs review
Issue tags:-Novice, -string freeze, -#d7ux, -#d8ux, -needs backport to D7, -tcdrupal2012

#87: drupal-967566-87-combined.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+Novice, +string freeze, +#d7ux, +#d8ux, +needs backport to D7, +tcdrupal2012

The last submitted patch, drupal-967566-87-combined.patch, failed testing.

Title:Several important Core User settings lack support for hook_field_extra_fields()Several important Core User settings need to implement hook_field_extra_fields()

Slightly improving the title.

Status:Needs work» Needs review
StatusFileSize
new2.8 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/block/block.module.
[ View ]

Attached patch contains differences between #63 (never committed) and #87 (committed)

Status:Needs review» Needs work

The last submitted patch, 967566-98.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new2.19 KB
PASSED: [[SimpleTest]]: [MySQL] 40,570 pass(es).
[ View ]

Obvious type in block.module, rerun

StatusFileSize
new3.42 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 967566-extra_fields-100.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new39 KB

Here is a re-roll of the patch in #63 against current Drupal 7.x-dev. Tests will fail as this issue is set for 8.x as version.

After applying this patch the user account manage fields tab:
user_account_manage_fields.png

What is needed for this to make it into 7.16? Everything has worked fine with this patch for well over a year now.

StatusFileSize
new27.04 KB

To compare, here is a screenshot on how the manage fields tab looks without the patch in #100:

user_account_extra_fields_before.png

Status:Needs review» Needs work

The last submitted patch, 967566-extra_fields-100.patch, failed testing.

Patch from #100 is working fine for me under D7. THX and I wonder why this issue has not yet been fixed :-(((.

Status:Needs work» Needs review

Patch for D8 in #99

StatusFileSize
new3.42 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 967566-105-Implement-hook_field_extra_fields-to-provide-user-se.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

According to the current summary, the idea is to implement extra fields for:
- block
- contact
- locale
- overlay_control
- picture (user picture upload)
- signature_settings

Block is already in. As far as I can tell, the last D8 patch in #100 lost the locale implementation. Attaching a D8 patch with the remaining pieces: contact, locale, overlay_control, user picture upload, signature_settings.

@#91, we can certainly change the text to note that not all of them will show up, but we could also try to ensure the settings don't show when they are not relevant.
Is that to complicated?

I tried going over all the existing patches, but could find no SimpleTests for the remaining functionality. If I've missed it, please point it out and I'll merge it in. Otherwise someone needs to write those tests as well. :)

Status:Needs review» Needs work

Status:Needs work» Needs review
StatusFileSize
new3.49 KB
PASSED: [[SimpleTest]]: [MySQL] 40,335 pass(es).
[ View ]

That's what I get for running an out of date core. :(

An example of solving #91 would be to (mostly) re-use the logic that decides if the form should be available on the user profile page, and use it to determine whether or not to display the extra setting.

Hm, that might get complicated, especially for elements that appear on the user profile form under dynamic conditions... Also there could be issues with someone saving the Manage Fields page with the element not there and then the weights not being correctly preserved later (if the element ever decides to reappear on the form).

So although hiding them would be nice, I think labeling them as not-always-present is probably the simpler way to go?

Status:Needs review» Reviewed & tested by the community

Committed adapted patch to D7 Google Analytics and Piwik module.

#100, D7 version is RTBC! Commit it, please. No need to wait another 2 years!

Status:Reviewed & tested by the community» Needs review

@#110, we're not done with D8 yet, there's a usability problem remaining, as well as lack of tests. Please help with those if you want to move this forward.

StatusFileSize
new4.49 KB
PASSED: [[SimpleTest]]: [MySQL] 40,332 pass(es).
[ View ]

Added "This element will not be present on all user account pages." to all five hook implementations, including the already commited block hook.

I opted for a very generic message on all of them, as the option, figuring out the specific details around when each of the 5 blocks shows up, seemed to me like it carried a large risk of getting the message wrong under some conditions.

Please suggest otherwise if you disagree and think the messages should be more specific.

Still no more tests in the patch.

Issue tags:+Usability

D7 suxxx a lot. I never ever ran any previous core version with 6-10 major patches just to make my drupal 7 working / usable. Commit the patches and work on the remaining stuff in a followup, please. Not beeing able to order user fields ends with *extreme* usability issues to the users and a cluttered user form. I have many user fields... It's much more important than any minor detail you are speculating about and a missing test.

I think D7 is really, really awesome, even on the projects where I have core patches. :)

I leave it to the more experienced core maintainers to decide whether tests and UX are a separate issue, but personally I disagree. Yes, it will take longer to finish, but we will also be certain we get it right.

If you use patches, you should be using a makefile anyway, and the cost to you for running your project with another patch while we figure out the details, should be absolutely minimal.

Of course, if you were to provide the missing tests, things might progress faster.

@hass:

While I share your frustration about that this issue isn't resolved yet, I think your tone in #113 was a bit too harsh. @letharion has kindly offering to help out with this and only so after I approached him about it as i am not a skilled developer myself. I'm very grateful for his help here.

Regarding the issue about things not get backported and/or committed to D7, @webchick took the initiative to make that easier by creating the wiki page Proposal: Loosen rules regarding patches that may be backported during a stable release (7.x+). Its well worth a read and I'm sure it will help make that issue easier.

I can also recommend @Dries Drupal 8 session from Munich. I specificly asked in the Q/A section about how backporting and completing incomplete core features in point releases can be improved. While I didn't get the perfect answer, I got enough to understand this is an important issue they want to solve.

Status:Needs review» Needs work

The Language field is provided by language module, not Locale module in Drupal 8. Also, it recently got included in the manage fields screen via #1074672: Allow language select to be rearranged inside node form (which is set to be backported to D7). So I don't think that needs anything to do anymore, unless it is not working for you (in which case that issue should be reopened).

Status:Needs work» Needs review

@#116, thanks for pointing that out. Sorry if I'm being slow here, but aren't those two different things? I haven't dug into this issue greatly to be honest, but isn't the setting here about implementing the extra fields for user specific settings, and the issue you link to is about node specific settings?

Status:Needs review» Needs work

@Letharion this was not against your work. Don't get this wrong. I'm only frustrated about the D7 banana software quality for the reason that I wasted in past 6 months 400h (lost time) only to find out that this is a bug in suxxx unready D7. In general it could be awesome, but for sure it's not. I cannot theme forms #1114398: Form element & Form element label theming is broken, I cannot add js files for IE (see #865536: drupal_add_js() is missing the 'browsers' option for a useless discussion just to complicate things) only and so on and so on. Every day a new issue that takes 10h for nothing. And the bad, there are patches waiting for commit only for ages and tons of users confirmed they are working well. This is annoying only. I wish I could use Drupal 7 from time to time without debugging every module for bugs. I never experienced this with 4.7, 5 and 6.

Right, that was about content settings, and this is about user settings. However, it still belongs to the language module now in Drupal 8, not to locale. In fact user module itself adds the field and language module just populates it differently from core.

Status:Needs work» Needs review
StatusFileSize
new4.44 KB
PASSED: [[SimpleTest]]: [MySQL] 40,364 pass(es).
[ View ]

Moving code from locale to language module, thanks Gábor.

Status:Needs review» Needs work

This element will not be present on all user account pages.This element will not be present o

One space is missing on the first sentences end. I'm not a native English speaker, but shouldn't we better say "may not present"?

StatusFileSize
new4.38 KB
PASSED: [[SimpleTest]]: [MySQL] 40,354 pass(es).
[ View ]

"may" sounds good to me, as it's less definite than "will", and that seems like a good thing here.

Removed the entire duplicate sentence.

Status:Needs work» Needs review
StatusFileSize
new1.86 KB
PASSED: [[SimpleTest]]: [MySQL] 40,360 pass(es).
[ View ]

Mostly blind copy/paste/replace of the blocks test for a new language test.

StatusFileSize
new6 KB
PASSED: [[SimpleTest]]: [MySQL] 40,355 pass(es).
[ View ]

Both code and a new test together.

Status:Needs review» Needs work
Issue tags:-Usability, -Novice, -string freeze, -#d7ux, -#d8ux, -needs backport to D7, -tcdrupal2012

Status:Needs work» Needs review
Issue tags:+Usability, +Novice, +string freeze, +#d7ux, +#d8ux, +needs backport to D7, +tcdrupal2012

#124: 967566-124-Implement-hook_field_extra_fields-to-provide-user-se.patch queued for re-testing.

#123 clearly doesn't work as it should have one fail. I don't understand why #124 failed, so I'm trying that one again.

There we go, atleast #124 is green. I tried asking in #d-c, but no one had time to help. As far as I can tell locally, the new test isn't being picked up, and I don't understand why. :(

Status:Needs review» Needs work

b/core/modules/language/lib/Drupal/language/Tests/LanguageUserAccountSettingsTest.php
* Definition of Drupal\language\Tests\LangaugeUserAccountSettingsTest.
class LangaugeUserAccountSettingsTest extends WebTestBase {

Because you have a typo in "Langauge" in both the phpDoc and the class name, whereas the file name is correct ;)

Status:Needs work» Needs review
StatusFileSize
new1.86 KB
FAILED: [[SimpleTest]]: [MySQL] 40,415 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

I could have sworn I copy pasted that name, but apparently not so.
Thanks a lot sun!

Unfortunately correcting the naming doesn't help locally, let's see what the bot says.

Status:Needs review» Needs work

Status:Needs work» Needs review
StatusFileSize
new9.12 KB
FAILED: [[SimpleTest]]: [MySQL] 40,416 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
new5.01 KB
FAILED: [[SimpleTest]]: [MySQL] 40,421 pass(es), 4 fail(s), and 0 exception(s).
[ View ]

Tests for language, user signature/picture and contact.

Is overlay really "test-less"? I tried finding an issue on the topic, but only found a UX-test one.

I believe this sums up the feedback so far on the issue. If the test bot shows 3 fails for the test only patch, and 0 fails for the complete patch, the patch is ready for a serious review.

StatusFileSize
new5.01 KB
FAILED: [[SimpleTest]]: [MySQL] 40,425 pass(es), 4 fail(s), and 0 exception(s).
[ View ]
new9.13 KB
PASSED: [[SimpleTest]]: [MySQL] 40,432 pass(es).
[ View ]

Upon a closer review, I noticed not all strings were correct, so some tests will fail even with the functionality.

Status:Needs review» Needs work

Status:Needs work» Needs review

First, thanks for great work last days on completing the D8 patch @letharion. Much appreciated.

I've had a discussion with @catch in IRC and he also had a quick look at it and said " I haven't properly reviewed yet but yeah general principle seems fine to me."

However, we still have the issue about Personalized Blocks. Thanks to @catch I finally understand what it does after he said:

It lets people disable or enable specific blocks on an individual basis from their profile.

I've also searched d.o for more info about it and it is very sparse. Found a couple of posts where users where looking to how to disable it. So @David_Rothstein guess that maybe "0.00001% of Drupal sites actually use the "Personalize blocks" feature" seems to be about right :)

Catch was even suggesting that it should be dropped from D8. Which seems reasonable to me as no one is using it and also all the new stuff coming in that will allow much better ways of creating the same UX for users.

Even so, as long as the code remains in D8 I don't see any harm of it being included in this patch as it will make it more visible. Then when/if a decision is made to remove it the new patch will do that.

For D7 it needs to be included to make the field properly visible in the UI, especially in regards to make it possible to control field weight and set permissions or put it in groups when using modules such as field permissions and/or field groups.

@Bojhan's opinion (from the IRC discussion) is to ditch it for D8, but make it visible for D7.

Great if we can get a quick decision about this last show stopper and finally get this committed to D8 and backported to D7.

For a D7 back-port, I guess the question mainly comes down to the string freeze, correct?

I agree that we could commit this without a decision on whether or not "Personlized blocks" stays. The current patch does very little with that functionality, and the rest of the patch should go in anyway.

Is there any string changes involved in the patch? I thought it was only new ones introduced due to that the fields will be available in the Manage tabs?

And since none are user-facing they aren't actually affected by the string freeze. At least not in Proposal: Loosen rules regarding patches that may be backported during a stable release (7.x+) where @webchick defines it as:

String freeze
Don't change any user-facing text (e.g. anything in a t() function), for the benefit of translation teams.

For admin facing strings the proposal says:

String change (admin-facing)
Any changes or additions to any admin-facing string (string wrapped in t() or st()). Forces translators to translate new/changed strings, and multilingual sites see English strings until this is done.
Release notes mention

If so, that shouldn't be a hinder to get this patch committed.

Regarding "Personlized blocks" I'm fine with leaving it out for now if that is blocking this patch from landing. It can always be taken care of in a follow up patch if needed.

I just want this getting in sooner rather than later...

[Should have read the comments more thoroughly to the proposal first...]

Its not a proposal anymore, it is a finished document called What patches may be backported to a stable release of Drupal core?

For the admin strings the note now says:

Release notes mention, but string-affecting patches will be committed as early in the monthly release cycle as possible to give translators time to prepare.

Thus, still wont be an issue for this patch in my book.

I think this String freeze makes no longer sense at all. We have localize.drupal.org and we can translate everything. We no longer have static PO files packages within the projects. Everyone can update the translation online with l10n_update and we made heavy changes to Core strings in German in past 6 months as very many of the translated stings have been and are still very buggy. We do nearly daily changes if we find the bugs and if you would have downloaded a translation 6 months ago, you will see tons of changed strings everywhere. If we would have such a strong rule the UI strings would never get fixed. I thinks this string rule need to change. We should only try to do only a very few changes, but if something is plain wrong or new, there is totally no reason to stop. The strings will go into D8, too and in this case D8 will have a more ready translation on day 1 than ever before. I don't think we have any core translation that is 100% complete and has perfect quality in all strings.

@hass: This is is not the right place to debate the rules around string freeze.

@webchick did an amazing job in taking initiative to improving how that and other backporting and point release improvements are handled in what now is the document I linked to. Now we have a great document covering and outlining practically all of the important things involved.

So, please lets try and stay focused on the scope of this issue, which is to get this patch committed to D8 and then backported and committed to D7 as well.

I hadn't seen the backports policy. Now that I've read it, I agree that it's probably fine to backport.

StatusFileSize
new9.13 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 967566-132-Implement-hook_field_extra_fields-to-provide-user-se_0.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new5.01 KB
FAILED: [[SimpleTest]]: [MySQL] 48,760 pass(es), 4 fail(s), and 0 exception(s).
[ View ]

Re-upload to check if this still applies to HEAD.

+++ b/core/modules/block/block.module
@@ -631,7 +631,7 @@ function block_form_user_profile_form_alter(&$form, &$form_state) {
-    'description' => t('Block module form element.'),
+    'description' => t('Block module form element. This element may not be present on all user account pages.'),

User account categories are gone, so I think we can drop these additions in all of the extra field descriptions?

+++ b/core/modules/contact/lib/Drupal/contact/Tests/ContactUserAccountSettingsTest.php
@@ -0,0 +1,45 @@
+/**
+ * Tests personalized contact settings for user accounts.
+ */
+class ContactUserAccountSettingsTest extends WebTestBase {

I'm not sure whether we need this test — there is test coverage for the personal contact form already, which probably tests this implicitly already, no?

Same for the other tests — let's make sure to not introduce duplicate test coverage here.

Status:Needs review» Needs work

Status:Needs work» Needs review

What? Patches still apply locally. I'm gonna try again, before I address Sun's concerns in in #142.

Status:Needs review» Needs work
Issue tags:+Usability, +Novice, +string freeze, +#d7ux, +#d8ux, +needs backport to D7, +tcdrupal2012

Feature freeze is now passed and afaik this patch did not make it. Hopefully the freeze extension will still allow it to get into D8, but that's for others to decide.

Can we please make the last needed effort to get this much needed usability improvement in and then also backported to D7.

tsvenson pointed me to this issue. After #132 some off-target (at least at this time) discussion around backport/string freeze whatnot, it seems that the patch itself was already in quite good shape.

FWIW, this would really be a boon to site builders to be able to exercise control over in the UI. From that perspective seems like a bug, not a task even. But nevermind that.

#141 and #142 have the latest patch and discussion points. Would be great to see this moving forward again!

Status:Needs work» Needs review
StatusFileSize
new3.17 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal8.user-extra-fields.151.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

As mentioned before, the string changes to existing hook_field_extra_fields() implementations are obsolete, since there are no user account categories anymore.

The hook implementation for Language module is obsolete; it doesn't implement a hook_form_alter() anymore.

Likewise, the user picture is a real field now, so that's obsolete, too.

User signatures still exist as extra field, though there's still hope we convert them into a real field for D8 via #1548204: Convert user signature into a normal field

Lastly, I also dislike all of those one-off test classes for the extra field exposure in Field UI, which each consist of two (2) actual lines of test code only. That's simply not worth it.

For the same reason, I'm removing the test coverage from the originally committed patch in #87.

As mentioned before, the string changes to existing hook_field_extra_fields() implementations are obsolete, since there are no user account categories anymore.

I don't understand that. There aren't user categories, however things like the Overlay settings or the "Personalize Blocks" settings are still added to the user form conditionally, depending either on the user's permissions or how the site is configured or both. So it's still the case that some sites/users will have them appear on the form, and others won't.

Status:Needs review» Needs work

@sun: Thank you so much for getting the D8 patch up to current level of all the changes.

With all the things you list that now are very different from how things are in D7, how do you suggest we proceed with the backport so we can get that in maybe even to the next D7 point release?

Can we discuss backport after the commit to 8? It's a sure way to derail this discussion (again).
Lets focus on getting this in for D8 first.

Status:Needs work» Needs review

re: #152:
Yes, some of the extra fields only appear conditionally. However:

1) The entire definition and appearance of extra fields is pretty poor right now in the first place: A seemingly random description à la "Contact module form element." appears in a table column that has the label/heading "Field type". One should ask why the actual field type isn't "Fieldset" or "Details" or "Fieldgroup". The textual description that appears for extra fields is a user interface bug in the first place, if you'd ask me.

2) All other (real) fields are bound to field access constraints, too. They do not denote that in the UI either. Thus, I don't see why extra fields should try to explain that either. Doing so would only increase the problem of those descriptions, since i) real fields do not have a description at all, ii) some (but not all!) of the extra field descriptions would be a huge wall of text, and iii) there's very limited space available to begin with.

Ergo, that's a preexisting issue, which is neither introduced nor changed here.

I'd recommend to file a separate issue for "Clarify that some fields may not necessarily appear in Field UI overview", category task, priority normal, tagged usability.

(FWIW, I will definitely create another separate issue to have extra fields specify their actual field type either way.)

#1883858: Clarify that some fields may not necessarily appear in Field UI overview created (needs correcting/clarifying)

I think there might be one more issue needed (see the end of #155)

The patch seemed to be in good shape, but will likely need a reroll now. (reroll doc in case someone new wants to try: http://drupal.org/patch/reroll)

Anything else?

Status:Needs review» Needs work
Issue tags:+Usability, +Novice, +string freeze, +#d7ux, +#d8ux, +needs backport to D7, +tcdrupal2012

The last submitted patch, drupal8.user-extra-fields.151.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new1.67 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

Wow! This is still not in?

Re-rolled against HEAD.

StatusFileSize
new3.16 KB
PASSED: [[SimpleTest]]: [MySQL] 50,570 pass(es).
[ View ]

Apparently, outdated, too. Added new Language settings extra field to User module (and also corrected existing definitions there).

@sun: Thanks for the re-roll and added feature. I will do an UX review of the new patch over the weekend.

Status:Needs review» Needs work
StatusFileSize
new13.1 KB
new13.68 KB
new8.06 KB

Alright, sorry for being a little late. Did promise weekend, but been swamped. Here comes my UX review of the patch.

Patch tested on fully git updated D8 current as of now.

The only settings change I've don was turning on the Signature options for users.

Before patch was applied:
user_fields_stdinst_without_patch.png

After patch was applied :
user_fields_stdinst_with_patch.png

Note that the "Administrative overlay" field isn't showing above.

It did however show up a little later after I came back to the settings page:
user_fields_stdinst_overlay_with_patch.png

I'm not sure why, might have been a cache issue or something similar.

+++ b/core/modules/user/user.module
@@ -235,29 +235,38 @@ function user_field_info_alter(&$info) {
+  $fields['user']['user']['display']['member_for'] = array(
+    'label' => t('Member for'),
+    'description' => t('User module \'member for\' view element.'),

This field did not show up though and I couldn't find any setting for turning that on either.

It seems to me it is part of the User module. Is that correct or do I need to enable some other core module for it to show?

Besides the issue with the "Administrative overlay" and "Member for" field, this patch seems to work its magic. But due to these issues I put it back to Needs work for now.

Great work @sun and others, lets get these remaining nasties fixed so we can get this committed asap thanks.

Status:Needs work» Needs review

1) Overlay must have been a temporary cache fluke.

2) "Member for" is not a field, but an extra display thingie, so you won't find it on "Manage Fields", but instead on "Manage Display".

Status:Needs review» Reviewed & tested by the community
StatusFileSize
new22.47 KB

1, Lets hope so. FYI: I did apply/reset the patch live so I could check and test that everything got there. Also, my local Drush (5.8) complained when I tried to clean the cache with it.

2, Aha, that's what ['display'] means them. Found it and confirm it does what it is supposed to.

So, with that said those things makes it RTBC.

However I did notice two artifacts on the Display tab:
user_fields_display_artifacts.png

  • In Overlay they are both visible.
  • Without the overlay only the table border (left of the Display label) is visible. The drop-down looks OK then.

I don't think that is because of this patch though. Great if you can point me to where to file that issue, unless it is already known.

+1 to rtbc!

Looks good to me too.

Status:Reviewed & tested by the community» Fixed

Committed to 8.x. Thanks.

Version:8.x-dev» 7.x-dev
Assigned:Unassigned» tsvenson
Issue tags:-tcdrupal2012

Thank you everyone for making this happen, its been a roller coaster. I'm turning this back D7 and assigning to myself. This is a patch I believe I have enough coding skills for to make while the hot shots keep their focus on D8 ;)

Status:Fixed» Patch (to be ported)

Status:Patch (to be ported)» Needs review
StatusFileSize
new4.32 KB
PASSED: [[SimpleTest]]: [MySQL] 39,694 pass(es).
[ View ]

Here is a backport patch for Drupal 7. I have:

  • Fixed strings and weights so they match the D8 patch
  • Changed the return variable name to $extra to be used in all places
  • Cleaned up whitespaces

I tried to add the "Member for" to the Manage Display. I got it displaying, but it didn't affect the output. So either I didn't do it correct or it isn't supported in D7 (I'm not skilled enough to be able to find out that).

It needs to care that having this defined does not break ds/panels implementation.
Overall patch great!

+++ b/modules/user/user.moduleundefined
@@ -234,7 +234,27 @@ function user_field_extra_fields() {
/**
--
1.7.10.4

What is this at the end of the patch? does dreditor add this now?

You mean:

--
1.7.10.4

That is the git version I used. The patch is created using "git format-patch" and then it looks slightly different.

Status:Needs review» Reviewed & tested by the community

Spoke with @cosmicdreams re #172 and that was his only objection. Since that is part of "git format-patch" it wont affect the commit and thus this is now RTBC.

Status:Reviewed & tested by the community» Needs review
  1. +  if (variable_get('user_pictures', 1) == 1) {
    ....
    +  if (variable_get('user_signatures', 1) == 1) {

    Why is the default value of these variables "1" when it's "0" everywhere else they are used in the codebase?

    Also (minor), the "==1" isn't necessary since the if() statement will evaluate it as a boolean already.

  2. Otherwise, it looks good to me, but has anyone actually tested the Drupal 7 patch? (To make sure it doesn't affect the default ordering of the form elements, and that it actually works in general...) I don't see any comments to that effect above, and especially since there are no automated tests this could use some extra clicking around.
  3. It needs to care that having this defined does not break ds/panels implementation.

    Offhand I'm not sure why it would, but has anyone looked into this?

  4. The only other issue is that because this patch adds lots of new strings, we probably can't get it into the upcoming (i.e. next week) release. Per http://drupal.org/node/1527558 we should commit patches like this early in the cycle. But it's no problem after that, since these are all admin-facing strings.

Also, if people really want to throw "Personalize blocks" in there as it is, I guess we can :) I'm still pretty skeptical about it though. It is quite different from the others. For the others, the vast majority of administrators on the vast majority of Drupal sites will at least see those form elements on their own accounts... but not so for "Personalize blocks". I think a lot of people are going to see this on the Manage Fields screen and be quite confused why it's available to drag and drop even though it never appears on the actual form. Nonetheless, we can try it and see what happens (and hopefully improve it in #1883858: Clarify that some fields may not necessarily appear in Field UI overview).

Status:Needs review» Reviewed & tested by the community
StatusFileSize
new3.72 KB
PASSED: [[SimpleTest]]: [MySQL] 40,503 pass(es).
[ View ]

I have changed the user_pictures and user_signatures to have 0 as default values like David_Rothstein said in #175.

Patch is also tested against latest d7.

Status:Needs work» Needs review

#176: drupal7-user-extra-fields.176.patch queued for re-testing.

Status:Needs review» Needs work

The last submitted patch, drupal7-user-extra-fields.176.patch, failed testing.

Status:Needs work» Needs review

#176: drupal7-user-extra-fields.176.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+Usability, +Novice, +string freeze, +#d7ux, +#d8ux, +needs backport to D7

The last submitted patch, drupal7-user-extra-fields.176.patch, failed testing.

Status:Needs work» Needs review

#176: drupal7-user-extra-fields.176.patch queued for re-testing.

Status:Needs review» Reviewed & tested by the community

Great, patch in #176 is green and RTBC!

Just a question (using D7):
Why is this patch only changing the form $extra['user']['user']['form'][<extra_field>], and not the display mode $extra['user']['user']['display'][<extra_field>], too?

It seems to me we're doing half the job!
Having several view modes for the 'user' entity, I'd like to show/reorder some of these fields on the view mode, too.

(However, It doesn't seem to work. I've added some extrafields to the 'display' array part. The fields do show up on the 'manage display' tab, but there is no effect when viewing the user, using the 'default' view mode or a custom view mode.)

[EDIT] The system says I've updated the summary, too. I didn't. Not deliberately, anyway.

Status:Reviewed & tested by the community» Needs review
StatusFileSize
new1.12 KB
new3.72 KB
PASSED: [[SimpleTest]]: [MySQL] 40,082 pass(es).
[ View ]

I still don't see anyone really saying they did serious manual testing of this patch... So I tried it myself now.

The patch doesn't work correctly; if you apply the patch and clear caches it messes up the order of fields on the user edit form. Not good for a live website.

It looks like this is due to the Locale module and User module weights being set in the patch in a way that didn't match their weights on the form. (I have not checked if Drupal 8 has the same issue, but someone should.) Attached patch fixes that and also fixes the Contact label to match what's in use on the actual form in Drupal 7 (and what it says in Drupal 8).

There's still an issue where if you add some real fields, apply this patch, then go to admin/config/people/accounts/fields the order of form fields on that admin page doesn't match what's on the form. That's due to some fields having the same weight as each other, and might be an unavoidable consequence of adding new fields. At least once you make a change on the manage fields screen and save it you're confirming that's what you want the new order to be (and everything is OK after that), though, so we can probably commit it with that issue.

Also, still no one has thoughts on #175.3?

@johnv, almost all the extra fields being added by this patch are form elements that don't have an equivalent when the user account is being viewed, so it wouldn't make sense for them to appear on the Manage Display screen.

(The one exception is the user picture, I suppose, but I think making the placement of that configurable should be a separate issue.)

Also, still no one has thoughts on #175.3?

I only see: #175.1, #175.2 and #175.4 there. Where is .3?

Thanks!

Status:Needs review» Needs work

@David_Rothstein: Did a quick test of patch in #186 and it looks good for me. Did reorder and saved several times and everything works just like we want.

I also agree with @johnv point in #185 that we need to also think of the Manage Display. As you point out in #187 it is really only the user picture that needs to be made available there.

So for that reason I'm setting it to needs work so we can add that before its RTBC.

As per #175 point 3 I haven't done any testing there and not skilled enough to really judge that. But, unless I have totally misunderstood things, both Display Suite and Panels either use the rendered output from core, Manage tabs controls the output, or, in Panels case, simply grabs the objects on field level to be placed in the layout. Since several of those fields already have the stuff we are adding here I can't really imagine it would cause any problems.

I only see: #175.1, #175.2 and #175.4 there. Where is .3?

It shows up for me in Firefox, although I've heard rumors that other browsers sometimes have trouble with a blockquote immediately at the beginning of a numbered list item. Strange.

Anyway, for clarity, it was this:

It needs to care that having this defined does not break ds/panels implementation.

Offhand I'm not sure why it would, but has anyone looked into this?

(which was in reference to @andypost's comment in #171.

Thanks @David_Rothstein, my Google Chrome on Windows didn't show the #175.3 either.

The patch in #186 works for reordering the fields, but I would like to move the user picture to a separate category with the profile_lite module (profile_lite doesn't support pseudo-fields, but that should be easy to fix).

The patch doesn't help for that, since the 'account' category is hardcoded in user_profile_form().

Status:Needs work» Reviewed & tested by the community

@Paul B

Splitting up the field combos as you suggest in #192 is out of the scope for this patch. I agree it is kinda frustrating how some of those field combos are in D7. The goal here is to at least make the fields available to rearrange in the UI, which they today are not.

@David_Rothstein

I've done some more testing and for me the changes this patch adds is working without issues. My suggestion is that we RTBC it as it is and then leave adding the user picture to the display tab to a follow up. Its soon coming up on 3 years for this issue now and that's time enough ;)

StatusFileSize
new164.81 KB

I have a clean install of D7.23 minimal profile; installed and configured a lot of modules and then went to setup user profile fields order, which of course didn't work as I expected. Then I went to apply this patch, and now I'm receiving a lot of "htmlspecialchars() expects parameter 1 to be string, array given in check_plain() (line 1565..."

Also, the admin page now shows fields as arrays, not as their field type - I'm attaching a print screen of the problem.

Reversed the patch and the errors where gone; but of course the field order is all wrong again. Someone?

Thanks a lot!

Did you clear caches after applying the patch? I think this patch would require that.

Sure, sorry that I forgot to mention.

Issue summary:View changes

updated issue summary

It would be great to commit this patch, but I don't know how to interpret the last three [edit: four!] months of silence.

Can anyone reproduce or refute @fstrevisan_eZly's results in #194? I can't reproduce it myself, and it's not obvious to me how this patch would cause that, but it would be very bad if it did.

@fstrevisan_eZly, what modules are you using that might be doing things on the Manage Fields page? (Field Group, maybe? Any others?)

Also, no one took me up on checking whether the issue fixed in #186 (with the weights) applies to Drupal 8 also. I took a quick look now, though, and seems like it doesn't.

I got no issues applying patch #186, probably #104 caused some contrib module and combination.

David, extra fields approach is the same in D8 and probably will stay, anyway it makes sense to describe all extra fields that modules provide. I don't think that it could cause regressions for existing sites that have their user fields ordered already

Issue summary:View changes
Status:Reviewed & tested by the community» Needs review

If there was a mismatch in the weights it would cause problems with new Drupal 8 sites also. But anyway, it looks like there is no mismatch in Drupal 8.

However, I just tested the Drupal 7 patch with the Field Group module and unfortunately was able to reproduce @fstrevisan_eZly's results in #194. Actually, for me it was worse - I got a fatal error ("Unsupported operand types in ....web/drupal/modules/field_ui/field_ui.admin.inc on line 477") when visiting the Manage Fields page.

The problem is that the Field Group module has its own code in hook_field_extra_fields() to work around this bug. For example:

<?php
/**
* Implements hook_field_extra_fields().
*/
function field_group_field_extra_fields() {
....
 
// Field to itegrate with contact module.
 
if (module_exists('contact')) {
   
$extra['user']['user']['form']['contact'] = array(
     
'label' => t('Contact'),
     
'description' => t('Contact user element'),
    
'weight' => 5,
    );
  }
?>

(and similar for the other fields)

When we add the core implementation that does the same thing (and tries to set $extra['user']['user']['form']['contact'] also) the resulting array structure when the Field API calls module_invoke_all('field_extra_fields') gets broken.

I don't really know what to do about this. The Field Group module really shouldn't be trying to set these values for modules it doesn't control. On the other hand, it's used on 100,000 Drupal 7 sites, and we obviously don't want to break all those sites by committing this patch to core.

Perhaps the best solution is to change the code in core that invokes module_invoke_all('field_extra_fields') to deal with the possibility that more than one module might be trying to set the same extra field (and properly merge the arrays in that case)? Or move the core implementations to hook_field_extra_fields_alter() instead (kind of a hack)?

Bummer, I am starting to believe this patch is jinxed :(

Re weight, I double checked those against D8 for the patches I contributed in #170 to make sure they matched.

Would it be possible to work with the Field Group maintainers to come up with a solution on this. For example that they remove that code and roll a new version before this patch get committed?

Would that work to allow core to just take over control of those fields without Field Group having to clean up anything?

Thus, making this a two step update:

1 - Update to new Field Group with the fix
2 - Then update Core with this fix

I do realize that far from all sites using Field Group will know about this problem. But if I understand things right, it is actually just the Manage Fields config pages for the user that will break. It won't break the live site or anything else.

If that is correct, then by working with the Field Group maintainers the fix is just to update to the new version that remove the conflicting code.