Remove category system from user edit and view

Yay, great stuff! Agreed on the goal and approach. Removing cruft is good!

Nice diffstat you've got there.

Yes. Thanks for starting this, @moshe! Might even be something someone less experienced can complete?

+++ b/modules/system/system.module
@@ -1989,12 +1988,6 @@ function system_user_timezone(&$form, &$form_state) {
@@ -2002,8 +1995,8 @@ function system_user_timezone(&$form, &$form_state) {

@@ -2002,8 +1995,8 @@ function system_user_timezone(&$form, &$form_state) {
     '#description' => t('Select the desired local time and time zone. Dates and times throughout this site will be displayed using this time zone.'),
   );
   if (!isset($account->timezone) && $account->uid == $user->uid && empty($form_state['input']['timezone'])) {
-    $form['timezone']['#description'] = t('Your time zone setting will be automatically detected if possible. Confirm the selection and click save.');
-    $form['timezone']['timezone']['#attributes'] = array('class' => array('timezone-detect'));
+    $form['#description'] = t('Your time zone setting will be automatically detected if possible. Confirm the selection and click save.');
+    $form['timezone']['#attributes'] = array('class' => array('timezone-detect'));
     drupal_add_js('misc/timezone.js');
   }
 }

Do we want to remove the fieldset for timezone? Even without profile categories it's still its own chunk of "stuff", and not directly related to removing the legacy $category nonsense. I'd like one of the UX folks to weigh in here.

And oh yeah, huge +1 subscribe. Less code in user.module++!

2 days to next Drupal core point release.

profile2 is using this in D7, so this could use feedback from fago before full nukage.

Following, I can chime in but not sure what to chime in on. The timezone functionality is primary there for more international sites, like the one we are on - to have useful time notations (especially on community sites this is important).

I think its best to keep this issue for removing categories and do other tweaks in other issues, so we have less to argue about.

@Bojhan the question is whether the single timezone form item should have a fieldset around it or not. Moshe would you mind doing a before/after screenshot?

The user edit page is already very guilty of this. Contact module, system module (timezone), overlay module, locale module all put their one-form-item textareas in there. It gets unwieldy pretty fast. Once again I think this is not something we should resolve here. @moshe: I see http://api.drupal.org/api/drupal/modules--user--user.module/function/use... mentions categories, and that can be removed independent of other non-profile category related restructring, right?

This is how a normal user edit page looks like now with contact and overlay module enabled:

I agree with @Gábor Hojtsy -- it's better to keep this issue and patch focused on the technical issue, for which we still need @fago's feedback, it seems.

The user view and edit pages didn't gain much love during the D7 cycle, and I think we should review and discuss them more carefully with the usability team to improve the UX. Many possible solutions come to mind there, and for a couple of them, we'd actually keep the fieldsets.

profile2 is using this in D7, so this could use feedback from fago before full nukage.

profile2 is using it, because its goal was to do what profile.module does, but with using entities and fields. Still, if it goes away profile2 could easily implement the same UI by providing its own menu items.

Unrelated to profile2, I'd really really love to see $category die. It's a weird user.module thing which doesn't make any sense in the area of entities. Next, removing it is going to save us quite some headaches when implementing the unified crud interface for the user entity.

Thus, huge +1 from my side.

The user view and edit pages didn't gain much love during the D7 cycle, and I think we should review and discuss them more carefully with the usability team to improve the UX. Many possible solutions come to mind there, and for a couple of them, we'd actually keep the fieldsets.

Regardless how they are going to like, let's scrap $category and implement it in a sane way.

Personally I am totally fine with losing the fieldsets. There are many ways we can make this form more beautiful, but that should not be the aim of this issue. I am not sure which good looking ideas sun has in mind that are about retaining these fieldsets, but even then we should do that in a separate issue. Anyway, with or without I don't care - lets do any UX stuff in a new issue when this technical cleanup is done.

Instead of removing the fieldsets, maybe #993592: Vertical Tabs on User Account Edit page is a better way to go? (In which case the fieldsets would still be there, technically.)

Also, the screenshot in #19 looks good, but what would we do about the other elements that are still in fieldsets even after this patch (user signature, user picture, etc.); shouldn't they be converted also? And what happens when a contrib module adds some actual (legitimate) fieldsets to this page? A mix of fieldsets and non-fieldsets sandwiched between each other doesn't seem like a good idea.

+++ b/modules/user/user-profile.tpl.php
@@ -25,8 +25,6 @@
  * @see user-profile-item.tpl.php
  *   Where the html is handled for each item in the group.

This needs to be removed too.
Apart from that and the remaining indentation fixes the patch looks good to me.

This and user_view() reminds from another issue: We should fix our terminology and always refer to the user account as user account, not sometimes as profile. There should be no profile stuff left in user.module - it's the job of a profile module to build that. But that deserves its own user.module cleanup issue.

Also reviewed this patch in-depth, and what @fago said is indeed the only remaining task (next to adjustment of indentations).

I saw no issues other than the fieldset question, which is now resolved, so this gets a +1 from me to fix up #23 and then roll a with-indent-fixes version to commit.

+++ b/modules/locale/locale.module
@@ -275,20 +275,14 @@ function locale_language_selector_form(&$form, &$form_state, $user) {
-  $form['locale']['language'] = array(
+  $form['language'] = array(

As long as we don't have a UI concept for this form (like we have for node or node type edit forms), I think we should "still" enforce at least a pattern of module namespaces to prevent an infinite disaster of modules declaring arbitrary keys. At this point in time, the 'language' field is added by Locale module, so a top-level 'locale' form structure key seems to be required for me. (but doesn't require a fieldset, just the key)

+++ b/modules/system/system.module
@@ -1989,12 +1987,6 @@ function system_user_timezone(&$form, &$form_state) {
-  $form['timezone']['timezone'] = array(

Glad we're removing this anti-pattern ;) http://drupal.org/project/timezone would be happy if it would exist. :)

+++ b/modules/user/user.module
@@ -1149,7 +1113,6 @@ function user_validate_current_pass(&$form, &$form_state) {
 function user_account_form_validate($form, &$form_state) {
-  if ($form['#user_category'] == 'account' || $form['#user_category'] == 'register') {
     $account = $form['#user'];

@@ -1193,14 +1156,13 @@ function user_account_form_validate($form, &$form_state) {
-function user_user_presave(&$edit, $account, $category) {
-  if ($category == 'account' || $category == 'register') {
+function user_user_presave(&$edit, $account) {
+
     if (!empty($edit['picture_upload'])) {

Missing indentation change here :)

-2 days to next Drupal core point release.

-  if (!isset($account->content['summary'])) {
-    $account->content['summary'] = array();
-  }
-  $account->content['summary'] += array(
-    '#type' => 'user_profile_category',
-    '#attributes' => array('class' => array('user-member')),
-    '#weight' => 5,
-    '#title' => t('History'),
-  );
-  $account->content['summary']['member_for'] = array(
-    '#type' => 'user_profile_item',
+  $account->content['member_for'] = array(
+    '#type' => 'item',
     '#title' => t('Member for'),

1. If you're changing this, you need to change user module's hook_field_extra_fields() implementation accordingly. Otherwise the Manage Display functionality will be broken.
2. Why is this being themed as a form item when it's not displayed inside a form? (Side effect: The "Member for" pseudo-field now has a noticeably different font size than an actual user field, when they are displayed on the user's profile. Not that the previous theming was great either, though.)

-function _trigger_user($hook, &$edit, $account, $category = NULL) {
+function _trigger_user($hook, &$edit, $account) {

There are still some callers passing a 4th parameter to this function after the patch is applied. In at least one case (trigger_user_cancel()) it looks like it's still important.

@@ -275,20 +275,14 @@ function locale_language_selector_form(&$form, &$form_state, $user) {
...
+    '#access' => $form['#form_id']['#value'] == 'user-profile-form' || !($form['#form_id']['#value'] == 'user-register-form' && !user_access('administer users')),

It would be a lot cleaner to just pass the $form_id as a parameter to the function and compare against that instead.

****

Also, I still have questions about the fieldset change but apparently I didn't express them well. I'm going to try again, this time with a screenshot.

Let me try my fieldset question again.

The attached screenshot shows what the user profile form looks like with the patch applied and lots of core features enabled.

Whereas before, we had the "important" stuff (e.g. username and password) at the top of the form outside of a fieldset, and a bunch of other stuff in fieldsets neatly at the bottom (thereby following our UI guidelines; see http://drupal.org/node/1087106), now we have a mishmash. There are fieldsets in the middle of the page, with some things outside of fieldsets above it and some things outside of fieldsets below it. And this doesn't even take into account what happens when there are contrib module installed.

I don't think I've seen this pattern anywhere else in Drupal, and it looks like it would be really confusing. If we're going against the UI standards, we need a reason for it.

In short: Getting rid of fieldsets is good if we actually got rid of all them. But getting rid of some fieldsets and leaving others in the middle of the page (sandwiched between other elements) is worse than just leaving everything in a fieldset like we have now (where it's at least consistent).

Just to be extra clear, here's a screenshot of what the full page looks like in core now (without the patch). While it's not beautiful, at least it's consistent, and it basically follows the UI guidelines. I don't see how the UI changes in the patch are an improvement over this.

But to clarify, I do really love the rest of the patch, though :)

@David is right that fixing this partially makes little sense. However lets keep all talk around the actual UX of this form to a new issue and just commit this with no UI change. I do want people to keep in mind we can make partial fix's now, we are not at the end of the cycle.

Looks pretty close, but found a few remaining issues reading through it:

 function overlay_form_user_profile_form_alter(&$form, &$form_state) {
...
-    if (user_access('access overlay', $account)) {
...
+    '#access' => user_access('access overlay', $account),

I think the previous behavior of leaving it off the form entirely was intentional (to avoid polluting $user->data for all users, since that's where this overlay setting gets stored). Probably best not to change it here.

-    $form['timezone']['#description'] = t('Your time zone setting will be automatically detected if possible. Confirm the selection and click save.');
+    $form['#description'] = t('Your time zone setting will be automatically detected if possible. Confirm the selection and click save.');

Is this accidentally left over from the fieldset changes?

-      actions_do($aid, $objects[$type], $context);
+      actions_do($aid, $objects[$type], $context, $method);
     }
     else {
-      actions_do($aid, $account, $context, $category);
+      actions_do($aid, $account, $context);

Why is $method passed only in the first case, whereas before $category was only passed in the second case?

 function user_account_form_validate($form, &$form_state) {
-  if ($form['#user_category'] == 'account' || $form['#user_category'] == 'register') {
     $account = $form['#user'];
     // Validate new or changing username.
     if (isset($form_state['values']['name'])) {
@@ -1193,25 +1156,22 @@ function user_account_form_validate($form, &$form_state) {
         form_set_error('signature', t('The signature is too long: it must be %max characters or less.', array('%max' => $user_schema['fields']['signature']['length'])));
       }
     }
-  }

Looks like this is still missing an indentation change.

@@ -222,7 +210,7 @@ function user_field_extra_fields() {
       ),
     ),
     'display' => array(
-      'summary' => array(
+      'member-for' => array(
         'label' => t('History'),


@@ -924,17 +897,8 @@ function user_user_view($account) {
....
-  $account->content['summary'] += array(
-    '#type' => 'user_profile_category',
-    '#attributes' => array('class' => array('user-member')),
-    '#weight' => 5,
-    '#title' => t('History'),
-  );
-  $account->content['summary']['member_for'] = array(
-    '#type' => 'user_profile_item',
+  $account->content['member_for'] = array(
+    '#type' => 'item',
     '#title' => t('Member for'),

1. Still doesn't look like it will work correctly ('member-for' vs. 'member_for')...
2. Should we also change the label in user_field_extra_fields()? ("History" doesn't really make sense after this patch)
3. Just realized, I think we need an update function for this change (since the extra field weights are stored in the 'field_bundle_settings' variable in the database).

I think '#type' => 'item' is also still wrong, but not sure there's a good alternative currently, so we can leave it for a followup.

So close....

I read through again (mainly just read the new parts). Fix these and I think it's good to go.

1. +function user_update_8000() {
   +  $settings = field_bundle_settings('user', 'user');
   +  if (isset($settings['extra_fields']['display']['summary']['member_for'])) {
   +    $settings['extra_fields']['display']['member_for'] = $settings['extra_fields']['display']['summary']['member_for'];
   +    unset($settings['extra_fields']['display']['summary']['member_for']);
   

   I don't think this will work? $settings[...][...]['summary'] should contain an array of the view modes, so it doesn't seem like the 'member_for' key will ever be set.

   What this needs to do instead is check for $settings[..][..]['summary'], move it to $settings[...][...]['member_for'], and then unset $settings[...][...]['summary']. That should hopefully do it.

2. +/**
   + * @defgroup updates-7.x-to-8.x Updates from 7.x to 8.x
   + * @{
   + * Update functions from 7.x to 8.x.
   + */
   ....
   +/**
   + * @} End of "defgroup updates-7.x-to-8.x"
   + * The next series of updates should start at 9000.
   + */
   

   I believe this should be 'addtogroup' rather than defgroup (since the group is already defined elsewhere). So something like:

   /**
    * @addtogroup updates-7.x-to-8.x
    * @{
    */
   ....
   /**
    * @} End of "addtogroup updates-7.x-to-8.x"
    */
   

3. -  $account->content['summary'] += array(
   -    '#type' => 'user_profile_category',
   -    '#attributes' => array('class' => array('user-member')),
   -    '#weight' => 5,
   -    '#title' => t('History'),
   -  );
   -  $account->content['summary']['member_for'] = array(
   -    '#type' => 'user_profile_item',
   +  $account->content['member_for'] = array(
   +    '#type' => 'item',
        '#title' => t('Member for'),
        '#markup' => format_interval(REQUEST_TIME - $account->created),
   

   Didn't notice this before, but did now - I think we need to keep the #weight = 5 around. This will keep it at the bottom of the profile by default (and match the default weight from the extra fields implementation).

4. @@ -275,15 +275,15 @@ function locale_language_selector_form(&$form, &$form_state, $user) {
   ...
   +  $form_id = $form['#form_id']['#value'];
      $form['locale'] = array(
        '#type' => 'fieldset',
        '#title' => t('Language settings'),
        '#weight' => 1,
   -    '#access' => ($form['#user_category'] == 'account' || ($form['#user_category'] == 'register' && user_access('administer users'))),
   +    '#access' => $form_id == 'user-profile-form' || !($form_id == 'user-register-form' && !user_access('administer users')),
   

   Is it me or did the logical condition here change (lots of !s seem to have moved around)?

   While we're at it, I still think it would be better to pass $form_id as a parameter to the function (it's just a helper function that appears to only be called from one place in the codebase anyway). Then you could check actual form IDs, with underscores rather than hyphens. But most important thing is to double check that logical condition.

These changes all look good to me, and I think it's ready to go. Sorry for the delay in following up on this.

I think the patch might not apply any more though. I'll trigger the testbot to find out.

#40: category.diff queued for re-testing.

Hm, I'm not sure why I had trouble applying the patch before. It works fine now.

This is good to go. I noticed a couple pieces of stale documentation left over (maybe they snuck into core in the meantime) and I've made those changes in the attached patch.

I'm marking this RTBC, since the only changes I made were pretty trivial documentation fixes. I'm also attaching the interdiff from #40 in case anyone wants to review those.

When David Rothstein reviews a patch this throughly, I don't need to. Thanks. Nonetheless, I took a cursory look and things look OK. This is a gigantic step ahead because right now, saving a user is pretty much anyone's guess whether it works or not because categories could be dynamic and you have no idea what to pass to user_save to completely save back a user.

Looks good to me as well.

On the Locale module changes discussed above, normally I'm strictly following consistency and use all of the regular $form, $form_state(, and $form_id) arguments, since any kind of form building function possibly needs them (e.g., considering AJAX or any other required write access, especially to $form_state).

However, in this case, I think we're fine, since we're going to revamp the language selector through another issue in the queue anyway - don't want to hold up this patch further.

Speaking of, this patch removes a couple of files, so should wait after the core/ patch has landed - re-rolling should be easy.

#43: category.diff queued for re-testing.

Thanks! Committed and pushed to 8.x. this will need a change notice.

reprogrammer is working on a change notice for this issue. (Assigning to myself for the moment to avoid duplicated effort.)

@xjm suggested that I write a change notice for this issue during a Drupal core office hour. Therefore, I assigned this issue to myself.

I wrote up a change notice for this issue and @xjm reviewed it.

Thanks @reprogrammer!

I was porting a module and came across the removal of #type user_profile_item which was not really mentioned in the change notice. Rather than adjusting that one, I made a separate one here http://drupal.org/node/1893032 because it's what I would've found when searching. I did xref this issue there, however.

Also, updated http://drupal.org/node/1393236 with a reference to #user_category specifically (since that's what I hit in my module), and included a before/after code snippet.