The profile.module makes it possible to display custom fields on the registration form in addition to username, e-mail and password. In order to replace the profile.module with user fields (see #394720: Migrate profile module data to field API), we need to be able to add fields to the registration form.

If all user fields are added to the registration form by default, individual fields could be tweaked based on field-level permissions for anonymous users (the registration form is never displayed to authenticated users).

Files: 
CommentFileSizeAuthor
#93 field_user_register-501408-90.patch13.75 KByched
PASSED: [[SimpleTest]]: [MySQL] 28,838 pass(es).
[ View ]
#90 field_user_register-501408-90.patch13.75 KByched
PASSED: [[SimpleTest]]: [MySQL] 28,851 pass(es).
[ View ]
#79 field_user_register-501408-79.patch14.44 KByched
PASSED: [[SimpleTest]]: [MySQL] 28,851 pass(es).
[ View ]
#75 field_user_register-501408-75.patch9.73 KByched
PASSED: [[SimpleTest]]: [MySQL] 28,785 pass(es).
[ View ]
#75 user_field_setting.png16.49 KByched
#65 field_user_register-501408-65.patch8.99 KByched
PASSED: [[SimpleTest]]: [MySQL] 28,795 pass(es).
[ View ]
#55 field_user_register-501408-54.patch9.01 KByched
FAILED: [[SimpleTest]]: [MySQL] 28,786 pass(es), 3 fail(s), and 3 exception(es).
[ View ]
#49 account_edit_profie.png20.26 KBmgifford
#49 user_registration_page.png45.82 KBmgifford
#49 account_settings_list.png50.3 KBmgifford
#49 user_registration_page_edit.png39.29 KBmgifford
#49 account_settings_adding_field.png24.64 KBmgifford
#43 fields_user_register-501408-43.patch6.22 KByched
FAILED: [[SimpleTest]]: [MySQL] 28,681 pass(es), 6 fail(s), and 0 exception(es).
[ View ]
#43 user_registration_setting.png13.09 KByched
#43 user_registration_form.png14.36 KByched
#16 20100916_user_registration_fields.patch2.52 KBPieterDC
PASSED: [[SimpleTest]]: [MySQL] 27,339 pass(es).
[ View ]
#14 20100916_user_registration_fields.patch2.53 KBPieterDC
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 20100916_user_registration_fields.patch.
[ View ]
#13 20100915_user_registration_fields.patch707 bytesPieterDC
PASSED: [[SimpleTest]]: [MySQL] 24,803 pass(es).
[ View ]
#7 user_registration_fields_501408-7.patch1.99 KBfloretan
FAILED: [[SimpleTest]]: [MySQL] 24,793 pass(es), 6 fail(s), and 2 exception(es).
[ View ]
#3 user_registration_fields.patch3.61 KBfloretan
Passed: 12772 passes, 0 fails, 0 exceptions
[ View ]

Comments

For field-level permissions, see #501404: Field Permissions in Core.

Thanks for gettings things moving, flobruit.

Field API doesn't really make a distinction between forms. field_attach_form($obj_type, $object, &$form, &$form_state); adds the form elements for all fields in the object, that's it.

We're talking about 'build_modes for forms', here - and frankly, this is a little scary, both in terms of :
- additional complexity for the $instance array:

<?php
$instance
= array(
  ...
 
'widget' => array('type' => ..., 'settings' => ...),
);
// becomes
$instance = array(
  ...
 
'form' => array(
   
'edit' => array('type' => ..., 'settings' => ...),
   
'user_register' => array('type' => ..., 'settings' => ...),
  ),
);
?>

OK, not *that* bad...

- Field UI. Argh :-(

That feature (differentiate user edit form and user register form) really seems tied to user.module for now. I'm not sure there are actual use cases with other fieldable types that would justify this complex addition to the Field API+UI.

A minimal, one-off approach would be that the user_register form calls field_attach_form() and then manually removes or sets #access = FALSE on the ones it doesn't need.
Would also mean that user.module needs its own UI do specify which fields show up on registration (it's probably worth checking how D6 content_profile solves this)

Assigned:Unassigned» floretan
Status:Active» Needs review
StatusFileSize
new3.61 KB
Passed: 12772 passes, 0 fails, 0 exceptions
[ View ]

After talking with Dries and DamZ, it seems that if we remove the profile.module from core, the one feature that should be kept is this one.

Rather than having "build modes" for field forms, this patch simply adds user fields to the registration form, and hides any that haven't been specifically set to be displayed using #access = FALSE. To make a field display on the registration form, save the instance as:

$instance = array(
  ...
  'bundle' => 'user',
  'settings' => array(
    'user_register' => TRUE,
  ),
);

This setting is also added to the field instance settings form as a checkbox (only for instances on the "user" bundle).

This is just a few lines of code, so it makes more sense to keep it in user.module rather than putting it in a separate module.

Approach looks good to me.

However, supporting the 'add more' button for 'unlimited cardinality' fields requires a few structural changes in forms (basically: any forn with fields becomes potentially multistep). See #367006: [meta] Field attach API integration for entity forms is ungrokable, field_add_more_submit(), $form['#builder_function'] and the code pattern in node_form()/node_form_submit_build_node(), comment_form()/comment_form_submit_build_comment().

Note that the user edit form itself currently lacks those changes - still TODO, because we needed to have fields on users as a proof of concept for 'fields on non nodes', but the state of user.module, notably forms, was frankly scary back then. We were also waiting to see where #367567: Use AJAX framework for "Add more" / load include file containing form / introduce $form_state['build_info'] would take us. I'm not sure whether the recent user.module overhauls made things better in this area. So, with this patch, both user edit and user register form need to be refactored and made multiple.

Status:Needs review» Needs work

@yched - Does it mean that this issue needs work?

re roychri: yes, needs work ;-)

@flobruit:

+      if (!isset($field['instance']['settings']['user_register']) || !$field['instance']['settings']['user_register']) {

should be
+      if (!$instance['settings']['user_register']) {

and it should be added to the list of instance settings for all field types, using hook_field_info_alter()

Status:Needs work» Needs review
StatusFileSize
new1.99 KB
FAILED: [[SimpleTest]]: [MySQL] 24,793 pass(es), 6 fail(s), and 2 exception(es).
[ View ]

Updated the patch to use hook_field_info_alter(). Making the form multistep depends on a cleanup of the user forms and can be handled in a separate issue.

Status:Needs review» Needs work

The last submitted patch failed testing.

Tested the patch, doesn't work, think it will be quite a bit of work to make it work.

An interesting alternative is using the field permissions module + my patch to display fields marked for by anomyous users on the user registration form, see #598924: Port of Field Permissions to D7.

Subscribing.

Meanwhile, using berenddeboer his suggested alternative.

Status:Needs work» Needs review
Issue tags:-Fields in Core

Status:Needs review» Needs work
Issue tags:+Fields in Core

The last submitted patch, user_registration_fields_501408-7.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new707 bytes
PASSED: [[SimpleTest]]: [MySQL] 24,803 pass(es).
[ View ]

Why don't we leave permission handling to the Field Permissions module and focus this patch on making the user forms consistent with other entity bundle forms, being:

If the user edit form shows my additional fields, why shouldn't the user add/register form do this also?

See attached patch if you want to try it.

StatusFileSize
new2.53 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 20100916_user_registration_fields.patch.
[ View ]

Saving extra required text fields worked already, saving an empty optional file/image field caused some error.
This adjusted patch fixes that by lending code from the user edit form; user_profile_form* functions from user.pages.inc

Happy testing.

Status:Needs review» Needs work

The last submitted patch, 20100916_user_registration_fields.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new2.52 KB
PASSED: [[SimpleTest]]: [MySQL] 27,339 pass(es).
[ View ]

New attempt.

Category:task» feature

This is not just a task, it's a new feature that would enhance the project, thus marking as such.

Marked #316857: Requried fields in user profiles aren't really required! as duplicate and helping #608894: Resolve the conflict between the fieldable users UI and the profile module

Category:feature» bug

Marked #794220: Required user fields not mandatory for new user account creation as duplicate + marking this as bug report. The current behaviour is inconsistent, thus a bug we need to fix.

Priority:Normal» Major

Soobscreeb.

Version:7.x-dev» 8.x-dev
Category:bug» feature

This is clearly a feature, and it's too late for D7.

Too bad it's considered a feature just because this wasn't in D6 core... but Fields in Core wasn't either...

It's an inconsistency compared to all the other implementations..
I could almost cry for this.

Just saying, and not playing the always-changing-issue-priority game with someone I respect in D7 dev.

Well, this is a new feature, but it's the solution to the bug of #794220: Required user fields not mandatory for new user account creation. The bug is clearly d7, so I re-opened it.

I gotta say I'm rather shocked that this isn't possible in core in D7.

I went through and added my user fields to a new site I was setting up using D7. Then tried to figure out how to put them on the registration page.

What's the point of having user fields if you can't put them on the registration form where people fill them in?

Anyways, I had to re-do the work using http://drupal.org/project/profile2 (which didn't use the user fields I set up) and then they appeared here http://a11yyow.ca/user/register

But really, how are we supposed to have the new profile module for d7 do anything but negate the existing user fields.. The usability of this piece is going to be a real problem.

I'm disappointed that this issue was overlooked for a year while other development was being done. However, I didn't see it until now so really all I can say is thanks to those who identified it as an issue and were working on it.

Mike

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

Just stumbled over #794220: Required user fields not mandatory for new user account creation:

Steps to reproduce:
* add an required field to the user account
* register as a new user, the account now misses the required field value
* as administrator, try to edit the account. You cannot without providing the missing required field value.

Hate to say it, but I don't see a way to solve the problem without this patch, unless we remove built-in fields for user accounts in D7.

Maybe #942310: Field form cannot be attached more than once might help move this forward (allow fields in nested areas in form and form values).

What can be done here before rc1 - this seems like a pretty important missing feature, if we want user fields via Field API to be useful.

mgifford, are you saying that it was possible to add them to the user registration form via profile2, or was that not possible either?

Example use case for fields in registration: Legal module in D6 adds a required checkbox to the user registration form to indicate whether the user agrees to the site's terms and conditions. Is there a way to add that using the current Field API?

>mgifford, are you saying that it was possible to add them to the user registration form via profile2, or was that not possible either?

profile2 implements adding fields to the user registration form for its profiles, but not for user account fields.

I used the Profile 2 module instead of the core fields. I really don't see any way for the core user fields to be at all useful since they don't sync with a registration page. I ended up deleting the fields I'd created & re-creating them in Profile 2. I think that the Legal module can put stuff on the registration form just like Profile 2 does. However, I don't know if/how the Field API provides access to the registration page.

@fago, you're summarizing my statement correctly. The user account fields were not at all useful for registration (which is incredibly counter productive).

I really don't see any way for the core user fields to be at all useful since they don't sync with a registration page

Which is exactly the reason this issue exists and a patch has been proposed.

The patch looks OK. However :
- it would definitely need tests
- force including all fields on the user reg form is a bit violent and unflexible.
Maybe user module could form_alter a "display on registration form" checkbox into the field settings page (when editing an instance on user entity type).
user_register_form() calls field_attach_form() and then sets #access = FALSE on all fields where the checkbow wasn't checked.

Ok, I applied the patch here - http://drupal7.dev.openconcept.ca/user/register

It works well for me! Now we just need to write up some tests.
1) that verifies that a field is added to the registration form.
2) that verifies that mandatory fields cause the user registration to fail on login.

Anything else? And who can do this? I haven't been able to wrap my head around SimpleTest sadly.

@yched - regarding force, wouldn't that be a variation from how it's done now? You can make a field mandatory, and that applies on every form it's on. I haven't seen one where a mandatory field isn't mandatory in certain places.

Are you able to write up a patch for your enhancements so they can be properly considered?

You can make a field mandatory, and that applies on every form it's on. I haven't seen one where a mandatory field isn't mandatory in certain places

That applies on every form it's on, not on forms it's not on :-). "Required" is formAPI-level only. You can still programmatically save an entity with a 'required' field empty.

I can't promise I can tackle this in time, unfortunately. I'll try, but anyone feel free to at least give it a start.

force including all fields on the user reg form is a bit violent and unflexible

It may seem "violent", but it's at least logical.
Can't we point to Field Permissions for more flexibility? It already allows admins to set who can edit / view which fields.

I'm thankful you guys are helping to get this issue closer to a fixed state!

Ps: it's been 2 years since I've done a SimpleTest crash course (at Drupalcon Szeged). I'm afraid there's not much left of that.

I'm collecting some simpletest resources here - http://www.delicious.com/mgifford/simpletest

There's now a pretty big library of simple tests to draw on. I suppose at some point the library of tests should be bigger than the code base.

So search through the .test files for functionality that is similar.

It should be about looking to modify a file in modules/user/user.test

That test suite should be able to create user fields and we'd just need to test that if one is set to be part of the registration form it will also be tested.

with every $this->drupalGet() call there is a Debug page generated that you can see from the test results you find in admin/config/development/testing/results/1

You can there see exactly what the script is looking for. If you view the source of the results page you can see how you should frame your assertRaw() test.

Initially I found the assertRaw() tests to be rather complicated as they were bundled together with a lot of other code. I think if they were all written:

$look_for = l(t('The text we are looking for'), 'node/1');
assertRaw($look_for, t('Link not found'));

It would have been easier for me to get into.

Anyways, hope this helps.

Assigned:floretan» yched

I'm on it.

@yched - Thanks! I could be wrong on this, but to get this into D7 I do think it's going to have to be really simple.

It's embarrassing that right now we can't put user fields on the main registration page without a contrib module.

~

Subscribing. I just started building a site and this is the first blocking problem I've run into.

StatusFileSize
new14.36 KB
new13.09 KB
new6.22 KB
FAILED: [[SimpleTest]]: [MySQL] 28,681 pass(es), 6 fail(s), and 0 exception(es).
[ View ]

Here's a patch :

- adds a 'user_register_form' instance setting for all field types (defaults to FALSE)

- form_alters a checkbox for this setting into the 'edit instance' form for user entity type, and adds #states + JS to keep it checked + disabled when 'required' is checked :

user_registration_setting.png

- on user_register_form, call field_attach_form() to attach field widgets, and set #access = FALSE to those for which $instance['settings']['user_register_form'] is FALSE :

user_registration_form.png
(this reveals a bug in forms theming : #980144: Issues with "required, multiple" fields in forms)

- adds entity_form_field_validate() in user_register_validate() --> field validation

- adds entity_form_submit_build_entity() in user_register_submit() --> field values extraction, saved along with the user_save()

Still missing tests.

Further comments in the next post.

Issue : the patch only makes sure user field instances created in the UI cannot be 'required' and 'not displayed on user registration form'. You can still programmatically call field_create_instance() or field_update_field() and create such an instance, though. Field API has no hooks to alter fields or instances before they are saved :-/.
Possible workaround : in user_register_form(), show fields when 'user_register_form' || 'required'. Such instances would get 'user_register_form' forced to TRUE the next time they're edited in Field UI anyway.

Now the delicate part : how do we deal with existing user fields ? On IRC, webchick said she would support a patch to fix the #794220: Required user fields not mandatory for new user account creation WTF, but she's worried about having fields to suddenly appear on 'user register' forms on existing D7 sites and break custom theme or layouts.
Even if we omit to provide an upgrade function to set 'user_register_form' on existing required instances, the workaround mentioned above would still have them show up on register forms just because they're required.

Oooh....nice - looks like a quite small patch, considering the benefit. Will try to test soon.

How much of the install base of D7 would be affected by the problem of fields suddenly appearing? Seems like it's a small risk compared to the benefits...

I think that showing fields when 'user_register_form' || 'required' makes sense, since theoretically required fields should always be required.

I agree with EvanDonovan.

Something else:

how do we deal with existing user fields ?

Do you mean: as an upgrade path from Drupal 7 beta 3 to the next release?

I suppose you don't mean: as an upgrade path from Drupal 6. Because, in Drupal 6, users don't have fields besides the default ones. Unless you mean profile fields? Which are handled by the Profile module.

yes, I mean upgrade from Drupal 7 beta 3.

I think we should presume that existing user fields should not show up in registration, and just let people know who've already started development of D7 sites that the option is available to them now, but they will need to opt in to it.

That seems less risky that opting everyone in automatically.

I've applied this patch to my sandbox. Please feel free to sign up for an account here - http://drupal7.dev.openconcept.ca/user/register

This patch certainly allows me to add information to the registration page. In my testing though I ran into two ways to add content to the registration page:

Account settings - /admin/config/people/accounts/fields
Profiles - /admin/config/people/profile

Is this right? Seems confusing to people, but not sure where the bug lies.

I think it's fine to "hav[e] fields suddenly appear on 'user register' forms on existing D7 sites"

Might have to think about how they work with Profile2 - but it's still in beta. Folks can deal with this. It's to help resolve 3 years of painful user registrations.

Status:Needs review» Needs work

The last submitted patch, fields_user_register-501408-43.patch, failed testing.

@yched

yes, I mean upgrade from Drupal 7 beta 3

I didn't know there is such a thing as an upgrade path between beta's.

@EvanDonovan

That seems less risky that opting everyone in automatically.

But what would you do when someone has a required user field on his/her Drupal 7 site now? You wouldn't show that field on the registration form? While that's the exact problem / confusion this patch is trying to solve.

@mgifford
User fields and the Profile module are two different things.
In a previous phase, the Drupal maintainers have decided to automatically disable the Profile module, unless you're upgrading from a Drupal 6 site which uses that module. That makes the Profile module - as is in core - deprecated.
If you're starting a clean Drupal (7) site, you are able to use the default user fields or the Profile2 contrib module - if you want more options.

Sorry, I meant existing user fields shouldn't show in registration unless they were required.

Yes, there is an upgrade path starting with the first beta.

PieterDC - Thanks for the clarification. I've been pretty busy in D7 over the last 2 years, but there's really just so much I haven't even touched on. This whole piece is a good example.

Ok, so Profile module is depreciated and only there for legacy. That's good to know.

Right now in admin/modules it's listed as:
Profile 7.0-dev Supports configurable user profiles.

Sounds like this should be changed to:
Profile 7.0-dev Legacy Drupal 6 module to support configurable user profiles. New sites should use user fields.

That would be a separate issue (which I can start), but wanted to get clarification here first.

Ok, so other than fixing the tests, what else needs to happen to bring this to RTBC?

@mgifford: The Profile module doesn't even show in the admin/modules list unless you either have an old checkout of D7, or you upgraded from D6. It was decided to hide it from the other sites, to prevent the kind of confusion you mention. The "legacy" wording was considered as an alternative, but rejected. The actual issue is called something like "Resolve the conflict between user fields and profile.module".

I'm going to be away from the computer for the Thanksgiving holiday, so, unfortunately, I won't be able to test this patch. Hopefully, you can get it straightened out before the freeze.

Status:Needs work» Needs review
StatusFileSize
new9.01 KB
FAILED: [[SimpleTest]]: [MySQL] 28,786 pass(es), 3 fail(s), and 3 exception(es).
[ View ]

Spent 2 hours fixing those test fails :-/
Annoyingly enough, those are testing completely trivial code, and miserably fail to acknowledge the existance of things like hook_field_info_alter(). Turns out there's no other sane solution than mimicking the behavior of user_field_info_alter() inside the tests.

Anyway. Green.

So, summary for webchick :

Patch status is in #43 - #44.

If we want to avoid having existing 'required' fields suddenly appear on user_registration forms and break layouts on sites upgrading from beta3, the best I can propose is :

a) We don't provide any upgrade path with the patch. The 'user_register_form' setting defaults to FALSE for existing instances.

b) Unlike proposed in #44, we show fields on user_registration form only if $instance['settings']['user_register_form'] == TRUE (as opposed to "if $instance['settings']['user_register_form'] == TRUE || $instance['required'] == TRUE"). Legacy fields stay out of user_registration form, the change is fully transparent.

c) The moment you edit those legacy fields, though, Field UI forces you to save them with user_register_form = TRUE if they are 'required' - and you *now* have new fields on your user register form. We might be able to detect this case and display a warning when first displaying the 'field edit' form, though.

d) The point b) above also means that we don't prevent programmatically setting an instance to "required and not shown on user_register_form" through direct calls to field_[create|update]_instance(). If you do that, too bad for you, welcome to #794220: Required user fields not mandatory for new user account creation. We can't babysit inconsistent settings.

That's basically what the current patch does, BTW. If we get an agreement, I'll reroll to add the warning message mentioned in c).

Subscribing.

This will allow the module I'm working on behave consistently when dealing with user, profile2 and core profile data.

Code looks great, will try it out now.
While I'm not that intimate with the Field API yet, this patch, and more specifically its simplicity, does show it was designed quite well.
If we can get away with this little a change for such a {key missing feature|horrible bug} there's really no question whether we should.
Since such a thing (altering all Field's edit forms) will show up in contrib eventually this also serves as a great example of how to do that.

Patch works great. Nothing unexpected.

One question: Should there be a seperate display mode for the user register form?

Sorry for the spam, just one last remark about the upgrade from beta3:

It's a beta. We pledge to not break anyone's data, which we don't. We try really hard not to break anyone's code (as in theme) unless there's a really good reason to. This is a really good reason. If 7.0 were out, this would be a different issue, but again, it's beta.
Also this is just a couple div's on the register form. It's not like we're breaking every theme out there, just those who chose to style their register form very delicately.

One question: Should there be a seperate display mode for the user register form?

As far as I know, display modes don't work for forms.
If you want them to? Feel free to open an issue for it.

I asked myself the same question last week ;-)

Equivalents of 'view modes' for forms definitely won't happen in D7.

Right. Wrong thinking on my part, sorry. No objections then.
Would be good to get some more reviews.

Status:Needs review» Needs work

The last submitted patch, field_user_register-501408-54.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new8.99 KB
PASSED: [[SimpleTest]]: [MySQL] 28,795 pass(es).
[ View ]

I swear I checked for the test to come green locally. Don't know what I foobared when rolling #54.
Really green this time ?

Status:Needs review» Needs work
Issue tags:-Fields in Core, -Needs usability testing

The last submitted patch, field_user_register-501408-65.patch, failed testing.

Status:Needs work» Needs review
Issue tags:+Fields in Core, +Needs usability testing

Gr. Fatal error in LocaleMultilingualFieldsFunctionalTest.
Didn't fail with the previous patch, and works fine locally.

#65: field_user_register-501408-65.patch queued for re-testing.

OK, green at last.

Since this can't really go in after UI freeze setting to RTBC.
As said before, another review couldn't hurt.

Status:Needs review» Reviewed & tested by the community

Right.

Looks great! Thanks for RTBC'ing it. I've got that path up here - http://drupal7.dev.openconcept.ca/user/register

Seems to be working just fine.

Status:Reviewed & tested by the community» Needs review

How about reviewing and testing the patch before marking it RTBC?

Edit: Meaning, talk about how the code looks, talk about what specific steps you tested, talk about how the upgrade path looks, etc.

I really should have this link memorized.. Someone wrote up a really nice description about the process to test the code. Remembering that just because the bot gives it a green light that the community has to do due diligence to ensure that it's better than our existing tests. Arg, s/he who posted it had a nice 4 point approach.. Something that would be really nice to work into the issue queue.

We've got screenshots. Got a working demo.

On the code level, I like how the testFieldInfo() has been reformatted to bring the variable closer to the test which should make it easier to debug.

I verified that the "force the 'Display on registration form' checkbox checked whenever 'Required' is checked" javascript addition worked fine by adding a Last Name.

I like that it's presently bound to "Display just below the 'required' checkbox."

@yched's code is well documented, organized & concise.

I've added mandatory fields into my sandbox (after applying the patch of course). Viewed the results as an admin (and new user), changed the display so that the view user page displays differently (moving the new fields above/below the history. Changed/edited content (as a user & admin). Deleted content from required fields as a user and got the required error message "Last Name field is required."

As far as the upgrade path. I think that without the patch folks will look at the user fields, see that it doesn't offer them what they need and go to profiles to allow them to put information on the user registration form (where you'd expect it to be).

I didn't test an upgrade, but from reviewing the code, I don't see how it would be a problem. There'd just be an extra set of options available if someone had set these up.

Hopefully @tstoeckler @EvanDonovan or @PieterDC can provide some more details too. I'm tempted to mark this RTBC again, but will wait a bit to see if someone can provide a better description of their test process.

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

Thanks, Mike. That's more like it. :)

And sorry for being so touchy about this, but over the weekend, it seems people are blindly marking stuff RTBC without actually testing it, in an attempt to hurry up and cram them in before RC1. HEAD has broken twice in the past 2 days as a result. This is not the kind of state we want Drupal core to be in when we're so close to the biggest milestone of Drupal 7's lifetime. It needs to be rock-solid, and that means we need to do everything we can to break these patches before they're committed to core. This is always true, but is even more true right now more than ever.

My test plan:

I started by creating a required text field on user profiles, then applied the patch and reloaded the registration form. No new fields appearing out of nowhere. Good.

However, when I went back to edit the existing field, I got this:

Display on user profile checkbox force-unchecked on required field

So it looks like that should set a default value based on the value of the required checkbox.

If I save the field and go back and edit it, it's fine. When I created a new required image field, the field automatically checked for me, which was nice.

The actual registration form then looks like this:

Showing all fields marked for display on registration form

Just fields on the form below username/email. This is actually great, since it's something I'm often asked by clients to produce out of the gunked up crap that Profile module outputs.

So other than that glitch with the default state of the checkbox (which is hopefully a one-line fix), I couldn't find any problems clicking through. I'm too tired for a code review tonight, and need to review that tomorrow. I do notice a distinct lack of tests for this new functionality, though. Tagging.

Status:Needs review» Needs work
StatusFileSize
new16.49 KB
new9.73 KB
PASSED: [[SimpleTest]]: [MySQL] 28,785 pass(es).
[ View ]

Fixed webchick's remark in #74 :
On existing user 'required' fields, that currently don't have the 'user_registration_form' setting on,
- the "Display on user registration form" checkbox appears disabled and *checked*,
- we display a message so that we don't just silently make the field pop out on user reg forms.

The message is only displayed for fields created before the patch was applied.

user_field_setting.png

Status:Needs work» Needs review

The last submitted patch, field_user_register-501408-75.patch, failed testing.

Status:Needs work» Needs review
Issue tags:+Needs tests, +Fields in Core, +Needs usability testing

Come on. This passes locally.

#75: field_user_register-501408-75.patch queued for re-testing.

StatusFileSize
new14.44 KB
PASSED: [[SimpleTest]]: [MySQL] 28,851 pass(es).
[ View ]

Added tests for fields appearing on user_register forms.

Status:Needs review» Needs work
Issue tags:-Needs tests, -Fields in Core, -Needs usability testing

The last submitted patch, field_user_register-501408-79.patch, failed testing.

Status:Needs work» Needs review

same old, same old...

#79: field_user_register-501408-79.patch queued for re-testing.

Status:Needs review» Needs work

The last submitted patch, field_user_register-501408-79.patch, failed testing.

Status:Needs work» Needs review

Someone please put test client #32 out of its misery ?

#79: field_user_register-501408-79.patch queued for re-testing.

Status:Needs review» Needs work

The last submitted patch, field_user_register-501408-79.patch, failed testing.

Status:Needs work» Needs review
Issue tags:+Needs tests, +Fields in Core, +Needs usability testing

#79: field_user_register-501408-79.patch queued for re-testing.

Status:Needs review» Reviewed & tested by the community
Issue tags:-Needs tests

@yched - how did you know that one of the bot's was the problem? Where does it say "test client #32" - I didn't see any similar identifier with http://qa.drupal.org/pifr/test/109959

@webchick - regarding comment #74 - You've got full rights to be touchy about folks RTBC'ing things without proper evaluation. You've seen enough by folks, including myself, who RTBC things before they are ready. There are some folks like @sun who will look at these and flip them back to needs work before you get them I'm sure.

Now, I tried to repeat the glitch, but I can't seem to reproduce it. I've added forms now and had no problem clicking on the "Display on user registration form" page.

So, since @yched's added the tests I'm willing to mark this RTBC.

EDIT - Oh ya, I re-applied this to my sandbox, cleared the caches, and looked over the new code added by @yched.

Status:Reviewed & tested by the community» Needs work

Actually, let's not put that warning in there. I get what you're trying to do, but it breaks UX freeze, and we don't do that in other places in Drupal.

@webchick : ok, will reroll later tonight without the warning message

@mgifford : client #32 has been my nemesis all throughout yesterday :-). You can see which client runs the tests by expanding the 'event log' fieldset at the top of http://qa.drupal.org/pifr/test/109959.

Thanks. So good to see such fast progress on this issue!

Status:Needs work» Needs review
StatusFileSize
new13.75 KB
PASSED: [[SimpleTest]]: [MySQL] 28,851 pass(es).
[ View ]

Removed the warning message, as per #87.

Status:Needs review» Needs work
Issue tags:-Fields in Core, -Needs usability testing

The last submitted patch, field_user_register-501408-90.patch, failed testing.

Status:Needs work» Needs review
Issue tags:+Fields in Core, +Needs usability testing

#90: field_user_register-501408-90.patch queued for re-testing.

StatusFileSize
new13.75 KB
PASSED: [[SimpleTest]]: [MySQL] 28,838 pass(es).
[ View ]

Straight re-upload of #90.

test bot frozen ?

Status:Needs review» Reviewed & tested by the community

Great! I installed this again in my test environment. Ran through some basic tests with the user registration fields. Think we're good to go!

Status:Reviewed & tested by the community» Fixed

Ok, cool.

This patch breaks feature and UX freeze both, so normally would be a clear D8 contender. However, fairly convincing arguments have been made that without this feature, required fields on users make no sense, and will result in data anomalies. So for this reason and only this reason...

Committed #93 to HEAD.

Great work on this, folks! Sorry about my initial skepticism. Just tryin' to do my job. ;)

Status:Fixed» Closed (fixed)
Issue tags:-Fields in Core, -Needs usability testing

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