On the user registration form, the username and email fields are enclosed in a fieldset. If there are no other elements in the form, the fieldset border is hidden in order to make the form appear less crowded.

This feature broke and was fixed in #460706: Layout of registration form depends on timezone setting (see the screenshot from that issue).

But now it is broken again. This time it is due to #118345: Revamp hook_user_form/_register/_validate/_submit/_insert/_update that introduced some fields that are hidden using $element[#access] = FALSE, e.g. $form['signature_settings'].

This patch tries to fix it again. The clean-up is now done in user_register_form_pre_render(). This is really a generic function and could perhaps be moved to form.inc, but for now I have just added it to user.module.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

c960657’s picture

Issue tags: +D7 UX freeze
Bojhan’s picture

O jeez, lol Nice catch.

ohh, and Hey c960657 :)

Bojhan’s picture

Status: Needs review » Reviewed & tested by the community

Jeez it was green, just not showing green

chx’s picture

Status: Reviewed & tested by the community » Needs work

+ if ($visible_children == array('account', 'submit')) { this will break, php -r "var_dump(array('foo', 'bar') == array('bar', 'foo'));" gives me FALSE. Add a sort or array_diff and check for empty.

c960657’s picture

Status: Needs work » Needs review
FileSize
5.38 KB

Done.

c960657’s picture

Reroll.

Status: Needs review » Needs work
Issue tags: -D7 UX freeze

The last submitted patch failed testing.

Status: Needs work » Needs review

c960657 requested that failed test be re-tested.

Status: Needs review » Needs work

The last submitted patch, , failed testing.

Status: Needs work » Needs review
Issue tags: +D7 UX freeze

Re-test of from comment #2377628 was requested by @user.

Bojhan’s picture

Status: Needs review » Reviewed & tested by the community

Works

webchick’s picture

Status: Reviewed & tested by the community » Needs work

This code makes baby kittens cry. :(

Let's please at least get some inline comments in there to explain what's happening. But it'd be lovely for sun, chx, effulgentsia or one of our other FAPI gurus to give this a once-over and see if there's a more elegant/readable way to do it.

Dave Reid’s picture

+++ modules/user/user.module	15 Dec 2009 18:05:09 -0000
@@ -3108,48 +3108,62 @@ function user_register_form($form, &$for
+  foreach (element_children($form, TRUE) as $key) {
+    $element = $form[$key];
+    if ((!isset($element['#access']) || $element['#access']) && !(isset($element['#type']) && in_array($element['#type'], array('value', 'hidden')))) {
+      $visible_children[] = $key;
+    }
+  }

Can we please, please extract this into a element_get_visible_children() function in form.inc?

+++ modules/user/user.test	15 Dec 2009 18:05:09 -0000
@@ -105,38 +105,46 @@ class UserRegistrationTestCase extends D
+
+    variable_set('user_default_timezone', DRUPAL_USER_TIMEZONE_SELECT);
+    $this->drupalLogout();
+    $this->drupalGet('user/register');
+    $this->assertRaw('<fieldset id="edit-account"><legend>Account information</legend>', t('Account settings fieldset was not hidden.'));

Please add inline comments for the tests.

I'm on crack. Are you, too?

Dave Reid’s picture

Status: Needs work » Needs review
FileSize
5.12 KB
Dave Reid’s picture

Fixed minor grammar errors in comments.

atheneus’s picture

Last submitted patch passed test.

Bojhan’s picture

Status: Needs review » Reviewed & tested by the community

RTBC?

webchick’s picture

Great, this is much easier on the eyes now.

I just want to try this once more with one small adjustment:

if (count(array_diff($visible_children, array('account', 'submit'))) == 0) {

to:

  if (!count(array_diff($visible_children, array('account', 'submit')))) {

The latter is more consistent with what we do elsewhere in core.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Ok, cool. Committed #19 to HEAD!

Status: Fixed » Closed (fixed)
Issue tags: -D7 UX freeze

Automatically closed -- issue fixed for 2 weeks with no activity.