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.

CommentFileSizeAuthor
#216 do-not-test-settings-php-967566-216.patch775 bytesaerozeppelin
#216 interdiff-967566-186-216.txt5.97 KBaerozeppelin
#216 967566-216.patch5.08 KBaerozeppelin
#194 html_array_error.jpg164.81 KBfstrevisan_eZly
#186 drupal7-user-extra-fields-967566-186.patch3.72 KBDavid_Rothstein
#186 interdiff-176-186.txt1.12 KBDavid_Rothstein
#176 drupal7-user-extra-fields.176.patch3.72 KBanon
#170 drupal7-user-extra-fields.170.patch4.32 KBtsvenson
#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
#159 drupal8.user-extra-fields.159.patch1.67 KBsun
#151 drupal8.user-extra-fields.151.patch3.17 KBsun
#141 967566-132-Implement-hook_field_extra_fields-to-provide-user-se_tests.patch5.01 KBFabianx
#141 967566-132-Implement-hook_field_extra_fields-to-provide-user-se.patch9.13 KBFabianx
#132 967566-132-Implement-hook_field_extra_fields-to-provide-user-se.patch9.13 KBLetharion
#132 967566-132-Implement-hook_field_extra_fields-to-provide-user-se_tests.patch5.01 KBLetharion
#131 967566-131-Implement-hook_field_extra_fields-to-provide-user-se_tests.patch5.01 KBLetharion
#131 967566-131-Implement-hook_field_extra_fields-to-provide-user-se.patch9.12 KBLetharion
#129 967566-129-Implement-hook_field_extra_fields-to-provide-user-se_tests.patch1.86 KBLetharion
#124 967566-124-Implement-hook_field_extra_fields-to-provide-user-se.patch6 KBLetharion
#123 967566-123-Implement-hook_field_extra_fields-to-provide-user-se_tests.patch1.86 KBLetharion
#122 967566-122-Implement-hook_field_extra_fields-to-provide-user-se.patch4.38 KBLetharion
#120 967566-120-Implement-hook_field_extra_fields-to-provide-user-se.patch4.44 KBLetharion
#112 967566-112-Implement-hook_field_extra_fields-to-provide-user-se.patch4.49 KBLetharion
#107 967566-107-Implement-hook_field_extra_fields-to-provide-user-se.patch3.49 KBLetharion
#105 967566-105-Implement-hook_field_extra_fields-to-provide-user-se.patch3.42 KBLetharion
#101 user_account_extra_fields_before.png27.04 KBtsvenson
#100 user_account_manage_fields.png39 KBtsvenson
#100 967566-extra_fields-100.patch3.42 KBtsvenson
#99 967566-100.patch2.19 KBvalthebald
#97 967566-98.patch2.8 KBvalthebald
#87 drupal-967566-87-test.patch1.11 KBtim.plunkett
#87 drupal-967566-87-combined.patch1.75 KBtim.plunkett
#85 drupal-967566-86-tests.patch1.12 KBtim.plunkett
#85 drupal-967566-86-combined.patch1.76 KBtim.plunkett
#84 967566-84.patch1.77 KBxjm
#84 interdiff-79-84.txt1.25 KBxjm
#82 967566-82-test.patch1.12 KBxjm
#82 967566-82-complete.patch1.77 KBxjm
#82 interdiff.txt1.01 KBxjm
#79 block-extra-967566-79.patch1.75 KBoriol_e9g
#79 interdiff.txt877 bytesoriol_e9g
#77 block_extra_field-simpletest-967566-77-test.patch1.09 KBjoestewart
#77 block_extra_field-simpletest-967566-77.patch1.76 KBjoestewart
#75 block_extra_field-simpletest-967566-75-test.patch1.09 KBjoestewart
#75 block_extra_field-simpletest-967566-75.patch1.76 KBjoestewart
#72 block_extra_field-simpletest-967566-72.patch1.71 KBadnasa
#71 block_extra_field-967566-71.patch689 bytestsvenson
#63 967566-extra_fields-63.patch3.43 KBindytechcook
#55 UserFormElements.png138.76 KBGábor Hojtsy
#51 967566_50.patch3.73 KBrbayliss
#49 967566_48.patch3.97 KBrbayliss
#33 967566_33.patch4.66 KBmariusz.slonina
#25 967566_24.patch4 KBmariusz.slonina
#23 967566_23.patch4.04 KBmariusz.slonina
#20 967566_20.patch3.33 KBmariusz.slonina
#14 967566_before.png82.12 KBmariusz.slonina
#14 967566_13.png93.14 KBmariusz.slonina
#13 967566_13.patch3.33 KBmariusz.slonina
#11 967566_10.patch2.94 KBmariusz.slonina
#9 967566_9.patch2.97 KBmariusz.slonina
#6 967566_5.patch2.32 KBmariusz.slonina
#3 967566_3.patch1.62 KBmariusz.slonina
#3 967566_3.patch1.62 KBmariusz.slonina
Test_Drupal7b2_UserFields.png56.95 KBDocuAnt
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mariusz.slonina’s picture

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.

DocuAnt’s picture

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.

mariusz.slonina’s picture

mariusz.slonina’s picture

Status: Active » Needs review

Status: Needs review » Needs work

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

mariusz.slonina’s picture

FileSize
2.32 KB

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.

mariusz.slonina’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

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

mariusz.slonina’s picture

Status: Needs work » Needs review
FileSize
2.97 KB

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.

mariusz.slonina’s picture

Status: Needs work » Needs review
FileSize
2.94 KB

What about now, Dear Bot?

mariusz.slonina’s picture

Issue tags: +String freeze

tagging it, since the patch adds some description text

mariusz.slonina’s picture

FileSize
3.33 KB

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

mariusz.slonina’s picture

FileSize
93.14 KB
82.12 KB

And screenshots

catch’s picture

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.

mariusz.slonina’s picture

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.

mariusz.slonina’s picture

Issue tags: -String freeze, -#d7ux

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

mariusz.slonina’s picture

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

mariusz.slonina’s picture

FileSize
3.33 KB

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

mariusz.slonina’s picture

Status: Needs work » Needs review
Berdir’s picture

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.

mariusz.slonina’s picture

Status: Needs work » Needs review
FileSize
4.04 KB

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.

mariusz.slonina’s picture

FileSize
4 KB

Forgot about --no-prefix when generating patch.

mariusz.slonina’s picture

Status: Needs work » Needs review
David D’s picture

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

David D’s picture

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

mariusz.slonina’s picture

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

mariusz.slonina’s picture

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

David D’s picture

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.

mariusz.slonina’s picture

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.

mariusz.slonina’s picture

FileSize
4.66 KB

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

mariusz.slonina’s picture

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

Berdir’s picture

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.

Berdir’s picture

Version: 7.x-dev » 8.x-dev
Berdir’s picture

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

geerlingguy’s picture

Subscribe.

mariusz.slonina’s picture

Issue tags: +#d8ux

tagging for D8

dealancer’s picture

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.

Damien Tournoud’s picture

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

drynok’s picture

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.

klausi’s picture

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

klausi’s picture

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.

tsvenson’s picture

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.

tsvenson’s picture

Title: Can't control the position of Contact, Locale or Language settings » Several important Core User settings lack support for hook_field_extra_fields()
geerlingguy’s picture

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.

catch’s picture

Category: bug » task

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

rbayliss’s picture

Status: Needs work » Needs review
FileSize
3.97 KB

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.

rbayliss’s picture

FileSize
3.73 KB

Trying again.

geerlingguy’s picture

Status: Needs work » Needs review
mariusz.slonina’s picture

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

xjm’s picture

Tagging.

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs backport to D7
FileSize
138.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:

tsvenson’s picture

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

interX’s picture

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

David D’s picture

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

Dries’s picture

Status: Reviewed & tested by the community » Needs review

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

bfroehle’s picture

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.

Gábor Hojtsy’s picture

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

bfroehle’s picture

indytechcook’s picture

Updated patch with t()

Cheers.

tsvenson’s picture

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.

catch’s picture

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.

xjm’s picture

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

tsvenson’s picture

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

chx’s picture

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.

tsvenson’s picture

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

andypost’s picture

Issue tags: +Needs tests

Taggin cos needs test

tsvenson’s picture

Status: Needs work » Needs review
FileSize
689 bytes

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

adnasa’s picture

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.

tsvenson’s picture

Status: Needs work » Needs review

Setting back to Needs review for #71

joestewart’s picture

test for "Personalize blocks" field element now works

tim.plunkett’s picture

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.

joestewart’s picture

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?

xjm’s picture

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!

oriol_e9g’s picture

Status: Needs work » Needs review
FileSize
877 bytes
1.75 KB
xjm’s picture

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!

sun’s picture

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.

xjm’s picture

Status: Needs work » Needs review
FileSize
1.01 KB
1.77 KB
1.12 KB

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.

xjm’s picture

FileSize
1.25 KB
1.77 KB

Sorry, testbot.

tim.plunkett’s picture

Status: Needs review » Needs work

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

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
1.75 KB
1.11 KB

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

sun’s picture

Status: Needs review » Reviewed & tested by the community

Thanks. Good to go if bot comes back green.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 8.x. Thanks!

catch’s picture

Status: Fixed » Needs work

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

David_Rothstein’s picture

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

amyhribar’s picture

Assigned: Unassigned » amyhribar
Issue tags: +tcdrupal2012

working on issue summary

amyhribar’s picture

Issue summary: View changes

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

amyhribar’s picture

Issue summary: View changes

updated issue summary

amyhribar’s picture

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

added new issue summary

Uzo’s picture

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.

tsvenson’s picture

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.

valthebald’s picture

Status: Needs work » Needs review
FileSize
2.8 KB

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.

valthebald’s picture

Status: Needs work » Needs review
FileSize
2.19 KB

Obvious type in block.module, rerun

tsvenson’s picture

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.

tsvenson’s picture

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.

hass’s picture

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

oriol_e9g’s picture

Status: Needs work » Needs review

Patch for D8 in #99

Letharion’s picture

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
Letharion’s picture

Status: Needs work » Needs review
FileSize
3.49 KB

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

Letharion’s picture

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.

David_Rothstein’s picture

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?

hass’s picture

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!

Letharion’s picture

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.

Letharion’s picture

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.

hass’s picture

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.

Letharion’s picture

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.

tsvenson’s picture

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

Gábor Hojtsy’s picture

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

Letharion’s picture

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?

hass’s picture

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.

Gábor Hojtsy’s picture

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.

Letharion’s picture

Status: Needs work » Needs review
FileSize
4.44 KB

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

hass’s picture

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"?

Letharion’s picture

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

Letharion’s picture

Status: Needs work » Needs review
FileSize
1.86 KB

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

Letharion’s picture

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
Letharion’s picture

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.

Letharion’s picture

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

sun’s picture

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

Letharion’s picture

Status: Needs work » Needs review
FileSize
1.86 KB

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
Letharion’s picture

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.

Letharion’s picture

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
tsvenson’s picture

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.

Letharion’s picture

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.

tsvenson’s picture

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

tsvenson’s picture

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

hass’s picture

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.

tsvenson’s picture

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

Letharion’s picture

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

Fabianx’s picture

sun’s picture

+++ 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
Letharion’s picture

Status: Needs work » Needs review

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

Letharion’s picture

Letharion’s picture

Status: Needs review » Needs work
Issue tags: +Usability, +Novice, +String freeze, +#d7ux, +#d8ux, +Needs backport to D7, +tcdrupal2012
tsvenson’s picture

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.

hass’s picture

yoroy’s picture

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!

sun’s picture

Status: Needs work » Needs review
FileSize
3.17 KB

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: Remove user signature and move it to contrib

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.

David_Rothstein’s picture

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.

tsvenson’s picture

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?

yoroy’s picture

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.

sun’s picture

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

YesCT’s picture

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

YesCT’s picture

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.

sun’s picture

Status: Needs work » Needs review
FileSize
1.67 KB

Wow! This is still not in?

Re-rolled against HEAD.

sun’s picture

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

tsvenson’s picture

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

tsvenson’s picture

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.

sun’s picture

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

tsvenson’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
22.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.

andypost’s picture

+1 to rtbc!

Gábor Hojtsy’s picture

Looks good to me too.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 8.x. Thanks.

tsvenson’s picture

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

Tor Arne Thune’s picture

Status: Fixed » Patch (to be ported)
tsvenson’s picture

Status: Patch (to be ported) » Needs review
FileSize
4.32 KB

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

andypost’s picture

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

cosmicdreams’s picture

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

tsvenson’s picture

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.

tsvenson’s picture

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.

David_Rothstein’s picture

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

anon’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
3.72 KB

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: Reviewed & tested by the community » 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.

tsvenson’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

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

anon’s picture

Status: Needs work » Needs review

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.

andypost’s picture

tsvenson’s picture

Status: Needs work » Needs review
tsvenson’s picture

Status: Needs review » Reviewed & tested by the community

Great, patch in #176 is green and RTBC!

johnv’s picture

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.

David_Rothstein’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
1.12 KB
3.72 KB

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?

David_Rothstein’s picture

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

Fabianx’s picture

Also, still no one has thoughts on #175.3?

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

Thanks!

tsvenson’s picture

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.

David_Rothstein’s picture

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.

johnv’s picture

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

Paul B’s picture

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

tsvenson’s picture

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

fstrevisan_eZly’s picture

FileSize
164.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!

David_Rothstein’s picture

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

fstrevisan_eZly’s picture

Sure, sorry that I forgot to mention.

fstrevisan_eZly’s picture

Issue summary: View changes

updated issue summary

David_Rothstein’s picture

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.

andypost’s picture

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

David_Rothstein’s picture

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:

/**
 * 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)?

tsvenson’s picture

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.

Grimreaper’s picture

Hello,

Thanks to the patch in #186, I made quickly a patch for the signature field missing in field_group_field_extra_fields() https://www.drupal.org/node/2333803

But those extra fields should be in core. I have started a discussion in field_group issue queue : https://www.drupal.org/node/2465075

matslats’s picture

I would like to see the $account->status and $account->roles widgets in their own ExtraField.
The use case is that I'm breaking up the user profile form into several pages, one page of which is for administering the user.
So I would like role and status on a separate form-display to the username and password and email.

lpalgarvio’s picture

reviving

hass’s picture

Status: Needs review » Reviewed & tested by the community

I need to sort these 3 core fields. Can we commit this now?

Fabianx’s picture

Status: Reviewed & tested by the community » Needs work

I like this patch a lot.

However to not break BC, lets make this an optional setting. There could be 100000's of sites that have implemented the same code and or rely in their theme on this.

I suggest:

if (variable_get('drupal_enable_core_extra_fields')) {
}

It _is_ a disruptive change - unfortunately.

hass’s picture

Status: Needs work » Reviewed & tested by the community

There is no need for making this optional. No idea what this could break. Today it is broken.

cweagans’s picture

Status: Reviewed & tested by the community » Needs work

hass, the problem is explained in #199. Other modules and many thousands of sites have already worked around this issue in their own custom code, so extra fields suddenly appearing when core is updated would be a problem.

hass’s picture

As said above - the field group module does something that it should not do! Their fault. Workarounds can always break. That is the nature of a wonky workaround.

Solution is - fix the buggy field group module and add this patch here to core. I'm tired of patching core after every release just because someone else made mistakes.

Add a note to release notes, release a new field group version at the same day (or earlier) of core and we are finally done. And we implemented it simply - right.

I'm personally not using field group module as this broken sh** module has killed a site ~2 years ago.

cweagans’s picture

You can argue that field_group should not have implemented that on behalf of core (and for the record, I agree with you. That's kind of messed up). However, the fact is, they did, and now with the however many thousands of sites running field_group with that code included (and whatever custom code that developers have added to sites - I know I've done that more than a few times to avoid patching core), we have to deal with it appropriately. Unfortunately, that means not shipping an update to core that will make some extra fields show up twice in the field display list.

We should not throw site owners/builders under the bus because a fairly popular contrib module did something questionable.

andypost’s picture

#199 said

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

I'm sure this is a bug and that should be fixed, core can't merge arrays?

Fabianx’s picture

#210: Yes, that is a separate bug, but even fixing it, we still should make this optional as the order can't be guaranteed either.

  • Dries committed f9e44fc on 8.3.x
    - Patch #967566 by mariusz.slonina, rbayliss, joestewart, xjm, tim....
  • Dries committed dc3466b on 8.3.x
    Issue #967566 by Letharion, mariusz.slonina, indytechcook, adnasa,...

  • Dries committed f9e44fc on 8.3.x
    - Patch #967566 by mariusz.slonina, rbayliss, joestewart, xjm, tim....
  • Dries committed dc3466b on 8.3.x
    Issue #967566 by Letharion, mariusz.slonina, indytechcook, adnasa,...
Sutharsan’s picture

Issue tags: -Novice

Removing 3 year old Novice tag.

Fabianx’s picture

Issue tags: +Novice

Actionable item:

Add:

if (variable_get('drupal_enable_core_extra_fields')) {
}

around the code that adds the new extra fields.

Document setting in settings.php.

aerozeppelin’s picture

Status: Needs work » Needs review
FileSize
5.08 KB
5.97 KB
775 bytes

Updated patch according to comments in #215.

Fabianx’s picture

+++ b/settings.php
@@ -372,11 +386,16 @@ ini_set('session.cookie_lifetime', 2000000);
+ *   fields easily (re)order on the manage fields tab.

(re)order => orderable?

--

Not sure this should say this should not be enabled when field_group < version X is installed?

And maybe we should patch field_group to take the variable into account, too?

Can someone open an issue to start the discussion?

Overall looks good to me already.

  • Dries committed f9e44fc on 8.4.x
    - Patch #967566 by mariusz.slonina, rbayliss, joestewart, xjm, tim....
  • Dries committed dc3466b on 8.4.x
    Issue #967566 by Letharion, mariusz.slonina, indytechcook, adnasa,...

  • Dries committed f9e44fc on 8.4.x
    - Patch #967566 by mariusz.slonina, rbayliss, joestewart, xjm, tim....
  • Dries committed dc3466b on 8.4.x
    Issue #967566 by Letharion, mariusz.slonina, indytechcook, adnasa,...
hass’s picture

Can we commit this now, please?

ugintl’s picture

Testing

ugintl’s picture

I have applied the patch 967566-216.patch successfully, but the user picture field is not appearing in the manage display.

jollysolutions’s picture

Status: Needs review » Reviewed & tested by the community
hass’s picture

I'm against adding crap to settings.php. field_group should have never done this and need to be fixed. We can add a release note and we are done and field_group creates a new version with this bug fixed.

David_Rothstein’s picture

Status: Reviewed & tested by the community » Needs work

This patch puts existing fields in user_field_extra_fields() underneath the settings.php variable ("User name and password", "Timezone", and "History") so it causes those fields to disappear from the admin UI when the patch is applied.

David_Rothstein’s picture

I also don't really understand the reason for using a settings.php variable here. If the goal is to make this feature opt-in, why couldn't there just be a module that adds it? (Is the idea that since it could break some sites, people shouldn't be allowed to enable this via the admin UI? If so, we certainly need to add a description of the variable to settings.php which says that.)

I think we should just fix the actual bug instead, like I suggested in #199:

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

In particular the first option. Can't we just do that and then not have to worry about it anymore?

In #211 Fabianx said that might not be sufficient (I think he was talking about the fact that we can't guarantee the order that the hook implementations will run in, so we don't know which of the conflicting implementations will win out?) but I'm not sure that's a major concern in practice. I think we mostly just want to make sure there are no fatal errors :) Also, if we're worried about that, I feel like we could do something like use hook_module_implements_alter() to put the new core implementations as early as possible (so that contrib modules can override them) or in the worst case go with the second (hackier) solution of putting the core implementations in hook_field_extra_fields_alter() and then making it so they only add each setting if another module didn't add it already.

ugintl’s picture

This http://foggyperspective.com/article/positioning-users-profile-picture-ma... worked for me. However, when I asked the writer that how i can showw a formatter in field ui for user picture, he said you can use user picture field module. I am not sure how to use it. I want to make user picture editable. I am using panels. Also editable module which provides a field formatter but it does not work on user pic field.

  • Dries committed f9e44fc on 9.1.x
    - Patch #967566 by mariusz.slonina, rbayliss, joestewart, xjm, tim....
  • Dries committed dc3466b on 9.1.x
    Issue #967566 by Letharion, mariusz.slonina, indytechcook, adnasa,...