![Announcement icon Announcement icon](https://www.drupal.org/files/styles/grid-2-2x-square/public/announcements/drupal-evergreen-logo-280X280px%20%281%29_0.jpg?itok=PHpn6rCb)
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.
Comment | File | Size | Author |
---|---|---|---|
#216 | do-not-test-settings-php-967566-216.patch | 775 bytes | aerozeppelin |
#216 | interdiff-967566-186-216.txt | 5.97 KB | aerozeppelin |
#216 | 967566-216.patch | 5.08 KB | aerozeppelin |
#186 | drupal7-user-extra-fields-967566-186.patch | 3.72 KB | David_Rothstein |
![Acquia logo](/files/ads/acquia-issue-sponsorship.png)
Comments
Comment #1
mariusz.slonina CreditAttribution: mariusz.slonina commentedFollowing 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.
Comment #2
DocuAnt CreditAttribution: DocuAnt commentedVersion change from 7.0-beta2 to 7.x-dev because the problem is still there in 7.0-beta3 and 7.x-rc1.
Comment #3
mariusz.slonina CreditAttribution: mariusz.slonina commentedreattaching patch from #988974: Picture and signature forms cannot be reordered on user account settings / manage fields
Comment #4
mariusz.slonina CreditAttribution: mariusz.slonina commentedComment #6
mariusz.slonina CreditAttribution: mariusz.slonina commentedRe-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.
Comment #7
mariusz.slonina CreditAttribution: mariusz.slonina commentedComment #9
mariusz.slonina CreditAttribution: mariusz.slonina commentedThis 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.
Comment #11
mariusz.slonina CreditAttribution: mariusz.slonina commentedWhat about now, Dear Bot?
Comment #12
mariusz.slonina CreditAttribution: mariusz.slonina commentedtagging it, since the patch adds some description text
Comment #13
mariusz.slonina CreditAttribution: mariusz.slonina commentedJust 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
Comment #14
mariusz.slonina CreditAttribution: mariusz.slonina commentedAnd screenshots
Comment #15
catchWhile this is nice to have, it's a new feature rather than a bug.
Comment #16
mariusz.slonina CreditAttribution: mariusz.slonina commentedWell, 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.
Comment #17
mariusz.slonina CreditAttribution: mariusz.slonina commented#11: 967566_10.patch queued for re-testing.
Comment #18
mariusz.slonina CreditAttribution: mariusz.slonina commented#13: 967566_13.patch queued for re-testing.
Comment #20
mariusz.slonina CreditAttribution: mariusz.slonina commentedre-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
Comment #21
mariusz.slonina CreditAttribution: mariusz.slonina commentedComment #22
BerdirThe 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.
1000 trailing spaces :)
Powered by Dreditor.
Comment #23
mariusz.slonina CreditAttribution: mariusz.slonina commentedFixed 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
Comment #25
mariusz.slonina CreditAttribution: mariusz.slonina commentedForgot about --no-prefix when generating patch.
Comment #26
mariusz.slonina CreditAttribution: mariusz.slonina commentedComment #27
David D CreditAttribution: David D commentedThis 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
Comment #28
David D CreditAttribution: David D commentedOnce I disabled the d8ux module and enabled the d8ux_account module only, it seems to be working fine. Thank you!
Comment #29
mariusz.slonina CreditAttribution: mariusz.slonina commentedThanks, 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
Comment #30
mariusz.slonina CreditAttribution: mariusz.slonina commentedPlease, test the recent revision posted on Github, I did some cleanups, and moved d8ux_account to d8ux. Thanks :)
Comment #31
David D CreditAttribution: David D commentedI 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.
Comment #32
mariusz.slonina CreditAttribution: mariusz.slonina commentedIt'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.
Comment #33
mariusz.slonina CreditAttribution: mariusz.slonina commentedThis patch adds missing form for Language settings (Locale module).
Comment #34
mariusz.slonina CreditAttribution: mariusz.slonina commentedPlease, see http://drupal.org/sandbox/mariusz.slonina/1075174 for a sandbox D8UX module
Comment #35
BerdirMoving 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.
Comment #36
BerdirComment #37
Berdir#33: 967566_33.patch queued for re-testing.
Comment #38
geerlingguy CreditAttribution: geerlingguy commentedSubscribe.
Comment #39
mariusz.slonina CreditAttribution: mariusz.slonina commentedtagging for D8
Comment #40
dealancer CreditAttribution: dealancer commentedGuys, 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.
Comment #41
Damien Tournoud CreditAttribution: Damien Tournoud commented@dealancer: += is not the same as array_merge, and yes, both are supported by PHP 5.3.
Comment #42
drynok CreditAttribution: drynok commentedHow 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.
Comment #43
klausi@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).
Comment #44
klausiI'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.
Comment #45
tsvenson CreditAttribution: tsvenson commentedMarking #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.
Comment #46
tsvenson CreditAttribution: tsvenson commentedComment #47
geerlingguy CreditAttribution: geerlingguy commentedFor 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.
Comment #48
catchThis is more task than bug, core should be consistent with this though I agree so not dropping from major.
Comment #49
rbayliss CreditAttribution: rbayliss commentedRerolled 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.
Comment #51
rbayliss CreditAttribution: rbayliss commentedTrying again.
Comment #52
geerlingguy CreditAttribution: geerlingguy commentedComment #53
mariusz.slonina CreditAttribution: mariusz.slonina commentedHi, 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).
Comment #54
xjmTagging.
Comment #55
Gábor HojtsyPatch 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:
Comment #56
tsvenson CreditAttribution: tsvenson commentedReally looking forward to when this lands in D7. Going to make things so much easier to maintain.
Comment #57
interX CreditAttribution: interX commentedPatch #55 works nicely on 7.8. This is a nice change when using a lot of fields.
Comment #58
David D CreditAttribution: David D commentedI have applied 967566_50.patch to D7.8 with success as well. Thank you!
Comment #59
Dries CreditAttribution: Dries commentedWe're missing
t()
functions around thelabel
fields, no?Comment #60
bfroehle CreditAttribution: bfroehle commentedUser 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.
Comment #61
Gábor Hojtsy@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.
Comment #62
bfroehle CreditAttribution: bfroehle commentedYes, fixed very recently in #1253204: user_field_extra_fields() does not translate labels.
Comment #63
indytechcook CreditAttribution: indytechcook commentedUpdated patch with t()
Cheers.
Comment #64
tsvenson CreditAttribution: tsvenson commentedPatch 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.
Comment #65
catchThis 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.
Comment #66
xjmProbably need a newline at the end of the file here.
Not going to change the status over that though. :P
Comment #67
tsvenson CreditAttribution: tsvenson commented@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...
Comment #68
chx CreditAttribution: chx commentedThanks 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.
Comment #69
tsvenson CreditAttribution: tsvenson commentedDarn. 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...
Comment #70
andypostTaggin cos needs test
Comment #71
tsvenson CreditAttribution: tsvenson commentedIt 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...
Comment #72
adnasa CreditAttribution: adnasa commentedI 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.
Comment #74
tsvenson CreditAttribution: tsvenson commentedSetting back to Needs review for #71
Comment #75
joestewart CreditAttribution: joestewart commentedtest for "Personalize blocks" field element now works
Comment #76
tim.plunkettThis 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.
Comment #77
joestewart CreditAttribution: joestewart commentedIndeed 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?
Comment #78
xjmThanks folks! That test looks sufficient to me too. I reviewed the latest patch and found a few more minor things to fix:
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.
"People settings" doesn't really make sense to me. Can we come up with a slightly better name for this test case? :)
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.
I think the first argument needs to be translated with
t()
, because it is matching an actual UI string provided by the Block module. (The second string, the assertion message, can remain untranslated.) Reference: http://drupal.org/simpletest-tutorial-drupal7#tAdding the novice tag because these are easy cleanups. Thanks!
Comment #79
oriol_e9gComment #80
xjmAlright, this looks ready to me, and it's been around the block a few times. Thanks everyone!
Comment #81
sunWrong indentation (only one space on the return level).
Missing trailing comma.
I don't think that administer blocks is required for the extra field to appear?
access administration pages is probably not needed, too.
Missing phpDoc.
Comment #82
xjmWow, 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.
Comment #84
xjmSorry, testbot.
Comment #85
tim.plunkettNeeded reroll after #135464: Add language options to block visibility.
Comment #87
tim.plunkettI clearly missed the memo on s/DrupalWebTestCase/WebTestBase...
Comment #88
sunThanks. Good to go if bot comes back green.
Comment #89
Dries CreditAttribution: Dries commentedCommitted to 8.x. Thanks!
Comment #90
catchIt looks like #63 was never committed, so the patch committed in #87 was incomplete, or am I missing something?
Comment #91
David_Rothstein CreditAttribution: David_Rothstein commentedI think @catch is right - a lot of code disappeared between the patches in #63 and #71 and wasn't ever added anywhere else.
Also:
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.)
Comment #92
amyhribar CreditAttribution: amyhribar commentedworking on issue summary
Comment #92.0
amyhribar CreditAttribution: amyhribar commentedRewrite the issue summary to better explain what this patch is doing.
Comment #92.1
amyhribar CreditAttribution: amyhribar commentedupdated issue summary
Comment #93
amyhribar CreditAttribution: amyhribar commentedadded new issue summary
Comment #94
Uzo CreditAttribution: Uzo commented#87: drupal-967566-87-combined.patch queued for re-testing.
Comment #96
tsvenson CreditAttribution: tsvenson commentedSlightly improving the title.
Comment #97
valthebaldAttached patch contains differences between #63 (never committed) and #87 (committed)
Comment #99
valthebaldObvious type in block.module, rerun
Comment #100
tsvenson CreditAttribution: tsvenson commentedHere 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](/files/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.
Comment #101
tsvenson CreditAttribution: tsvenson commentedTo compare, here is a screenshot on how the manage fields tab looks without the patch in #100:
Comment #103
hass CreditAttribution: hass commentedPatch from #100 is working fine for me under D7. THX and I wonder why this issue has not yet been fixed :-(((.
Comment #104
oriol_e9gPatch for D8 in #99
Comment #105
Letharion CreditAttribution: Letharion commentedAccording 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. :)
Comment #107
Letharion CreditAttribution: Letharion commentedThat's what I get for running an out of date core. :(
Comment #108
Letharion CreditAttribution: Letharion commentedAn 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.
Comment #109
David_Rothstein CreditAttribution: David_Rothstein commentedHm, 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?
Comment #110
hass CreditAttribution: hass commentedCommitted adapted patch to D7 Google Analytics and Piwik module.
#100, D7 version is RTBC! Commit it, please. No need to wait another 2 years!
Comment #111
Letharion CreditAttribution: Letharion commented@#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.
Comment #112
Letharion CreditAttribution: Letharion commentedAdded "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.
Comment #113
hass CreditAttribution: hass commentedD7 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.
Comment #114
Letharion CreditAttribution: Letharion commentedI 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.
Comment #115
tsvenson CreditAttribution: tsvenson commented@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.
Comment #116
Gábor HojtsyThe 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).
Comment #117
Letharion CreditAttribution: Letharion commented@#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?
Comment #118
hass CreditAttribution: hass commented@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.
Comment #119
Gábor HojtsyRight, 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.
Comment #120
Letharion CreditAttribution: Letharion commentedMoving code from locale to language module, thanks Gábor.
Comment #121
hass CreditAttribution: hass commentedOne space is missing on the first sentences end. I'm not a native English speaker, but shouldn't we better say "may not present"?
Comment #122
Letharion CreditAttribution: Letharion commented"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.
Comment #123
Letharion CreditAttribution: Letharion commentedMostly blind copy/paste/replace of the blocks test for a new language test.
Comment #124
Letharion CreditAttribution: Letharion commentedBoth code and a new test together.
Comment #126
Letharion CreditAttribution: Letharion commented#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.
Comment #127
Letharion CreditAttribution: Letharion commentedThere 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. :(
Comment #128
sunBecause you have a typo in "Langauge" in both the phpDoc and the class name, whereas the file name is correct ;)
Comment #129
Letharion CreditAttribution: Letharion commentedI 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.
Comment #131
Letharion CreditAttribution: Letharion commentedTests 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.
Comment #132
Letharion CreditAttribution: Letharion commentedUpon a closer review, I noticed not all strings were correct, so some tests will fail even with the functionality.
Comment #134
tsvenson CreditAttribution: tsvenson commentedFirst, 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:
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.
Comment #135
Letharion CreditAttribution: Letharion commentedFor 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.
Comment #136
tsvenson CreditAttribution: tsvenson commentedIs 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:
For admin facing strings the proposal says:
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...
Comment #137
tsvenson CreditAttribution: tsvenson commented[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:
Thus, still wont be an issue for this patch in my book.
Comment #138
hass CreditAttribution: hass commentedI 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.
Comment #139
tsvenson CreditAttribution: tsvenson commented@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.
Comment #140
Letharion CreditAttribution: Letharion commentedI hadn't seen the backports policy. Now that I've read it, I agree that it's probably fine to backport.
Comment #141
Fabianx CreditAttribution: Fabianx commentedRe-upload to check if this still applies to HEAD.
Comment #142
sunUser account categories are gone, so I think we can drop these additions in all of the extra field descriptions?
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.
Comment #144
Letharion CreditAttribution: Letharion commentedWhat? Patches still apply locally. I'm gonna try again, before I address Sun's concerns in in #142.
Comment #145
Letharion CreditAttribution: Letharion commented#141: 967566-132-Implement-hook_field_extra_fields-to-provide-user-se_tests.patch queued for re-testing.
Comment #146
Letharion CreditAttribution: Letharion commented#141: 967566-132-Implement-hook_field_extra_fields-to-provide-user-se.patch queued for re-testing.
Comment #148
tsvenson CreditAttribution: tsvenson commentedFeature 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.
Comment #149
hass CreditAttribution: hass commentedhttp://buytaert.net/drupal-8-feature-freeze-extended
Comment #150
yoroy CreditAttribution: yoroy commentedtsvenson 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!
Comment #151
sunAs 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.
Comment #152
David_Rothstein CreditAttribution: David_Rothstein commentedI 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.
Comment #153
tsvenson CreditAttribution: tsvenson commented@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?
Comment #154
yoroy CreditAttribution: yoroy commentedCan 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.
Comment #155
sunre: #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.)
Comment #156
YesCT CreditAttribution: YesCT commented#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?
Comment #157
YesCT CreditAttribution: YesCT commented#151: drupal8.user-extra-fields.151.patch queued for re-testing.
Comment #159
sunWow! This is still not in?
Re-rolled against HEAD.
Comment #160
sunApparently, outdated, too. Added new Language settings extra field to User module (and also corrected existing definitions there).
Comment #161
tsvenson CreditAttribution: tsvenson commented@sun: Thanks for the re-roll and added feature. I will do an UX review of the new patch over the weekend.
Comment #162
tsvenson CreditAttribution: tsvenson commentedAlright, 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](/files/user_fields_stdinst_without_patch.png)
After patch was applied :
![user_fields_stdinst_with_patch.png](/files/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](/files/user_fields_stdinst_overlay_with_patch.png)
I'm not sure why, might have been a cache issue or something similar.
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.
Comment #163
sun1) 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".
Comment #164
tsvenson CreditAttribution: tsvenson commented1, 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](/files/user_fields_display_artifacts.png)
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.
Comment #165
andypost+1 to rtbc!
Comment #166
Gábor HojtsyLooks good to me too.
Comment #167
Dries CreditAttribution: Dries commentedCommitted to 8.x. Thanks.
Comment #168
tsvenson CreditAttribution: tsvenson commentedThank 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 ;)
Comment #169
Tor Arne Thune CreditAttribution: Tor Arne Thune commentedComment #170
tsvenson CreditAttribution: tsvenson commentedHere is a backport patch for Drupal 7. I have:
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).
Comment #171
andypostIt needs to care that having this defined does not break ds/panels implementation.
Overall patch great!
Comment #172
cosmicdreams CreditAttribution: cosmicdreams commentedWhat is this at the end of the patch? does dreditor add this now?
Comment #173
tsvenson CreditAttribution: tsvenson commentedYou mean:
That is the git version I used. The patch is created using "git format-patch" and then it looks slightly different.
Comment #174
tsvenson CreditAttribution: tsvenson commentedSpoke 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.
Comment #175
David_Rothstein CreditAttribution: David_Rothstein commentedWhy 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.
Offhand I'm not sure why it would, but has anyone looked into this?
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).
Comment #176
anonI 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.
Comment #178
tsvenson CreditAttribution: tsvenson commented#176: drupal7-user-extra-fields.176.patch queued for re-testing.
Comment #180
anon#176: drupal7-user-extra-fields.176.patch queued for re-testing.
Comment #182
andypostbots broken #2004496: Testbot cannot access git://git.drupal.org but can clone with http
Comment #183
tsvenson CreditAttribution: tsvenson commented#176: drupal7-user-extra-fields.176.patch queued for re-testing.
Comment #184
tsvenson CreditAttribution: tsvenson commentedGreat, patch in #176 is green and RTBC!
Comment #185
johnvJust 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.
Comment #186
David_Rothstein CreditAttribution: David_Rothstein commentedI 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?
Comment #187
David_Rothstein CreditAttribution: David_Rothstein commented@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.)
Comment #188
Fabianx CreditAttribution: Fabianx commentedI only see: #175.1, #175.2 and #175.4 there. Where is .3?
Thanks!
Comment #189
tsvenson CreditAttribution: tsvenson commented@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.
Comment #190
David_Rothstein CreditAttribution: David_Rothstein commentedIt 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:
(which was in reference to @andypost's comment in #171.
Comment #191
johnvThanks @David_Rothstein, my Google Chrome on Windows didn't show the #175.3 either.
Comment #192
Paul B CreditAttribution: Paul B commentedThe 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().
Comment #193
tsvenson CreditAttribution: tsvenson commented@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 ;)
Comment #194
fstrevisan_eZly CreditAttribution: fstrevisan_eZly commentedI 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!
Comment #195
David_Rothstein CreditAttribution: David_Rothstein commentedDid you clear caches after applying the patch? I think this patch would require that.
Comment #196
fstrevisan_eZly CreditAttribution: fstrevisan_eZly commentedSure, sorry that I forgot to mention.
Comment #196.0
fstrevisan_eZly CreditAttribution: fstrevisan_eZly commentedupdated issue summary
Comment #197
David_Rothstein CreditAttribution: David_Rothstein commentedIt 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.
Comment #198
andypostI 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
Comment #199
David_Rothstein CreditAttribution: David_Rothstein commentedIf 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:
(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)?
Comment #200
tsvenson CreditAttribution: tsvenson commentedBummer, 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.
Comment #201
GrimreaperHello,
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
Comment #202
matslats CreditAttribution: matslats as a volunteer commentedI 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.
Comment #203
lpalgarvio CreditAttribution: lpalgarvio commentedreviving
Comment #204
hass CreditAttribution: hass commentedI need to sort these 3 core fields. Can we commit this now?
Comment #205
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedI 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:
It _is_ a disruptive change - unfortunately.
Comment #206
hass CreditAttribution: hass commentedThere is no need for making this optional. No idea what this could break. Today it is broken.
Comment #207
cweaganshass, 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.
Comment #208
hass CreditAttribution: hass commentedAs 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.
Comment #209
cweagansYou 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.
Comment #210
andypost#199 said
I'm sure this is a bug and that should be fixed, core can't merge arrays?
Comment #211
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commented#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.
Comment #214
Sutharsan CreditAttribution: Sutharsan commentedRemoving 3 year old Novice tag.
Comment #215
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedActionable item:
Add:
around the code that adds the new extra fields.
Document setting in settings.php.
Comment #216
aerozeppelin CreditAttribution: aerozeppelin at California State University San Bernardino commentedUpdated patch according to comments in #215.
Comment #217
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commented(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.
Comment #220
hass CreditAttribution: hass commentedCan we commit this now, please?
Comment #221
ugintl CreditAttribution: ugintl commentedTesting
Comment #222
ugintl CreditAttribution: ugintl commentedI have applied the patch 967566-216.patch successfully, but the user picture field is not appearing in the manage display.
Comment #223
jollysolutionsComment #224
hass CreditAttribution: hass commentedI'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.
Comment #225
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedThis 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.
Comment #226
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedI 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:
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.
Comment #227
ugintl CreditAttribution: ugintl commentedThis 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.