Posted by Dave Reid on April 21, 2009 at 9:01pm
| Project: | Drupal core |
| Version: | 7.x-dev |
| Component: | contact.module |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Dave Reid |
| Status: | closed (fixed) |
| Issue tags: | API clean-up |
Issue Summary
The contact forms should attempt to re-use the cookie set by comment.module that has anonymous users' names and e-mail information. I will probably need to tinker a few things in comment.module as well so this can be a general cookie that can be re-used by all modules.
Comments
#1
#2
Attached patch that:
1. Standardizes the anonymous user details cookie into Drupal.user.field where field = name, email, etc.
2. Uses the built-in jQuery cookie library instead of coding our own cookie reading code in comment.js and contact.js
3. Automatically fills in anonymous user's info on the contact form if they're previously used a contact form or posted a comment, and vice-versa.
#3
Discovered that the comment.module cookie handling was broken as well as it used $form_state['#attached'] instead of $form['#attached'].
#4
+++ modules/comment/comment.module 6 Oct 2009 22:01:46 -0000@@ -1623,7 +1623,8 @@ function comment_form($form, &$form_stat
+ $form['#attached']['js'][] = array('data' => 'misc/jquery.cookie.js', 'weight' => JS_LIBRARY + 2);
+++ modules/contact/contact.pages.inc 6 Oct 2009 22:01:46 -0000
@@ -49,6 +49,11 @@ function contact_site_form() {
+ $form['#attached']['js'][] = array('data' => 'misc/jquery.cookie.js', 'weight' => JS_LIBRARY + 2);
jquery.cookie.js is registered as library, so we can use that here instead of manually including the js.
Basically, just replace 'js' with 'library', and make that array an indexed one that just lists ('system', 'cookie').
+++ modules/contact/contact.js 6 Oct 2009 22:01:46 -0000@@ -0,0 +1,16 @@
+ alert(cookie);
? 8)
I'm on crack. Are you, too?
#5
Tagging to get into my list.
#6
Revised patch that fixes some encoding problems, removes the errant alert() and uses #attached['library'].
#7
.
#8
+++ modules/comment/comment.module 6 Oct 2009 22:01:46 -0000@@ -1947,7 +1948,7 @@ function comment_form_validate($form, &$
foreach (array('name', 'homepage', 'mail') as $field) {
// Set cookie for 365 days.
if (isset($form_state['values'][$field])) {
- setcookie('comment_info_' . $field, $form_state['values'][$field], REQUEST_TIME + 31536000, '/');
+ setcookie('Drupal.user.' . $field, urlencode($form_state['values'][$field]), REQUEST_TIME + 31536000, '/');
}
}
+++ modules/contact/contact.js 6 Oct 2009 22:01:46 -0000
@@ -0,0 +1,15 @@
+ var cookie = unescape($.cookie('Drupal.user.' + this).replace(/\+/g, '%20'));
+++ modules/contact/contact.pages.inc 6 Oct 2009 22:01:46 -0000
@@ -102,12 +107,22 @@ function contact_site_form() {
function contact_site_form_validate($form, &$form_state) {
+ global $user;
...
+ if (!$user->uid) {
+ foreach (array('name', 'mail') as $field) {
+ // Set cookie for 365 days.
+ if (isset($form_state['values'][$field])) {
+ setcookie('Drupal.user.' . $field, urlencode($form_state['values'][$field]), REQUEST_TIME + 31536000, '/');
+ }
+ }
+ }
1) It seems illogical to me that we set this cookie in a validation handler. This means, we're very potentially setting a cookie with invalid values. We should really move those to the corresponding submit handlers instead.
2) Since we don't really need $user here and just want to access ->uid, we may use $GLOBALs['user'] instead.
3) If you'd use rawurlencode(), then you wouldn't need that .replace() in the JS.
This review is powered by Dreditor.
#9
Re-tagging.
#10
The main problem I'm encountering is the jquery.cookie library uses decodeURIComponent() when fetching cookies. Doesn't help things. Should I modify it to use unescape and escape()?
#11
Nevermind, those functions are deprecated in JS 1.5. I read through the original issue (#141131: Remember anonymous comment posters. (UTF-8 bug)) and noticed that they wanted to use setrawcookie() and rawurlencode() which works perfectly with jquery.cookie.js, but it wasn't available in PHP 4. That's no longer a problem now, so we should use that. Wee!
Moving the cookie setting to submit functions makes a lot of sense. The values will still be there if validation fails because of the Form API, so it seems good to only save a cookie if validation passed.
#12
Thank you! This looks great now.
#13
As a followup patch, what about re-using this info on the user registration page?
#14
The last submitted patch failed testing.
#15
Re-rolled for recent commits only.
#16
When committing this patch also remember to run
cvs add modules/contact/contact.js
#17
+++ modules/comment/comment.module 9 Oct 2009 02:51:24 -0000@@ -2054,6 +2047,18 @@ function comment_form_submit_build_comme
+ // Save the anonymous user information as a cookie so it can be reused.
+ if (!$user->uid) {
+ foreach (array('name', 'homepage', 'mail') as $field) {
+ if (isset($form_state['values'][$field])) {
+ // Set cookie for 365 days.
+ setrawcookie('Drupal.user.' . $field, rawurlencode($form_state['values'][$field]), REQUEST_TIME + 31536000, '/');
+ }
+ }
+ }
+++ modules/contact/contact.pages.inc 9 Oct 2009 02:51:24 -0000
@@ -117,7 +122,17 @@ function contact_site_form_validate($for
+ // Save the anonymous user information as a cookie so it can be reused.
+ if (!$user->uid) {
+ foreach (array('name', 'mail') as $field) {
+ if (isset($form_state['values'][$field])) {
+ // Set cookie for 365 days.
+ setrawcookie('Drupal.user.' . $field, rawurlencode($form_state['values'][$field]), REQUEST_TIME + 31536000, '/');
+ }
+ }
+ }
Would it make sense to pull this into another function? That's a lot of nearly identical code, and there might be places in contrib (such as ubercart) that want to do the same thing.
+++ modules/contact/contact.js 9 Oct 2009 02:51:24 -0000@@ -0,0 +1,15 @@
+Drupal.behaviors.contact = {
+ attach: function(context) {
+ $.each(['name', 'mail'], function () {
+ var cookie = $.cookie('Drupal.user.' + this);
+ if (cookie) {
+ $('#contact-site-form input[name=' + this + ']', context).once('comment').val(cookie);
+ }
+ });
+ }
+};
How come we were able to remove all the custom JS code from comment module but we still need this here?
I'm on crack. Are you, too?
#18
#19
@webchick: Why you gots to be all throughout with your reviews. You made a great point on a separate function for saving the anonymous cookie!
We're able to remove all the custom JS code from comments.js now that we're using the jquery.cookie.js library, which converts all that custom code into
var cookie = $.cookie('Drupal.user.' + this);. Neat huh? Essentially comment.js ends up looking nearly exactly the same as the new contact.js file.Revised patch with a user_save_anonymous_cookie().
#20
Could we rename the cookie from user to visitor or something like that. i am a bit worried about confusion with a real user object.
#21
Yeah, since this isn't an official 'user' type cookie, it makes sense to use 'Drupal.visitor' as the cookie 'name'. Added benefit this could possibly be nicer to play with http://drupal.org/project/visitor
#22
+++ modules/user/user.module 9 Oct 2009 13:31:32 -0000@@ -3226,3 +3226,23 @@ function user_login_destination() {
+function user_save_anonymous_cookie(array $values) {
+ if (!$GLOBALS['user']->uid) {
+ // Skip if the user is not anonymous.
uhm, the condition is reversed? ;)
This review is powered by Dreditor.
#23
That's what I get for trying to save some lines of code by not use $global user; Thanks sun. :)
#24
Re-rolled the function name to user_anonymous_cookie_save for our subject_verb pattern (thanks sun).
#25
sun made another good point that there was a lot of hard-coding going on here. Abstracted this to user_cookie_save(). I want to take a look at this again in D8 for further possible abstraction, but I think we're on the right path for this now.
#26
oh, cool, you found an even more clean way! :)
#27
Looks great. Committed to CVS HEAD. Thanks!
#28
Small followup. The contact.js file used the old Drupal.user name, and by request of moshe, we now fill in the the name and e-mail on the user registration form.
#29
Revised patch that uses Drupal.behaviors.userRegistration to help avoid potential JS conflicts.
EDIT: I double checked and it works as expected in all there places now (registration, contact, and comments)
#30
+++ modules/user/user.js 9 Oct 2009 16:07:42 -0000@@ -188,4 +188,15 @@ Drupal.behaviors.userSettings = {
+ $('#user-register input[name=' + this + ']', context).once('comment').val(cookie);
probably should be 'user' instead of comment.
thanks for adding reg form.
#31
+++ modules/user/user.js 9 Oct 2009 16:07:42 -0000@@ -188,4 +188,15 @@ Drupal.behaviors.userSettings = {
+ attach: function(context) {
+ $.each(['name', 'mail'], function () {
+ var cookie = $.cookie('Drupal.visitor.' + this);
+ if (cookie) {
+ $('#user-register input[name=' + this + ']', context).once('comment').val(cookie);
+ }
+ });
+ }
uh oh - we have a problem...
This should use:
$('form#user-register', context).once('comment', function () { ... }
because we want to have a primary condition for running the entire behavior. (also note the 'form' selector prefix to specifically target the form, not any arbitrary other element that happens to have a #user-register CSS ID)
The same change we also need to apply to the behaviors we patched previously here.
I'm on crack. Are you, too?
#32
I'm still too much of a jQuery noob to find those types of things, so much appriciated sun. Re-rolled to use once on the form element. Tested and works great on all three cases still.
#33
Correct patch this time...
#34
+++ modules/comment/comment.js 9 Oct 2009 17:13:48 -0000@@ -3,12 +3,14 @@
- $.each(['name', 'homepage', 'mail'], function () {
...
+ $('form#comment-form', context).once('comment', function() {
+++ modules/contact/contact.js 9 Oct 2009 17:13:49 -0000
@@ -2,13 +2,15 @@
- $.each(['name', 'mail'], function () {
...
+ $('form#contact-site-form', context).once('contact', function() {
+++ modules/user/user.js 9 Oct 2009 17:13:49 -0000
@@ -188,4 +188,17 @@ Drupal.behaviors.userSettings = {
+ $('form#user-register', context).once('userRegistration', function() {
Sorry to have to mention, but there should should be a space behind anonymous functions, as visible in the original code :-/
+++ modules/contact/contact.js 9 Oct 2009 17:13:49 -0000@@ -2,13 +2,15 @@
- attach: function(context) {
...
+ attach: function (context, settings) {
Funnily, here you did it right ;)
This review is powered by Dreditor.
#35
Revised patch for correct coding standards.
#36
Shiny! :)
#37
Once again, we have exactly the same code copy/pasted 3 different times here, with just minor changes. Shouldn't we pull that into an API function that's called from each place, and takes the dynamic bits as parameters?
#38
#39
Sorry, but no, this makes no sense. @webchick, please go ahead with the previous patch in #35.
#40
The last submitted patch failed testing.
#41
Both options re-rolled for head for committers to choose.
Option1: Uses custom JS to load cookie values and put them into form fields. We duplicate a little code, but don't hard-code anything.
Option2: Adds a Drupal.addVisitorCookieInfo(form_id) function to drupal.js to handle taking values from the Drupal.visitor cookie and putting them into form fields. Not as flexible and hard-coded.
#42
Because this is currently a broken feature, marking this as a bug report.
#43
Can we rename addVisitorCookieInfo() to something that suggest filling out a form? addVisitorCookieInfo() suggests that we're saving a cookie or something. I think that would make it a tad more intuitive.
#44
Brainstorming on option 2 function names...
(fill or add)FormVisitor(Info or Cookie)
#45
Maybe: populateVisitorInformation() or populateVisitorInformationFromCookie()?
#46
Function set (fill) form values from cookie ... so why it should it be different?
Tested ..option2 patch with (winXP ff35, safary4, google chrome, opera10) and it Works!
#47
+++ misc/drupal.js 10 Oct 2009 19:36:15 -0000@@ -109,6 +109,21 @@ Drupal.checkPlain = function (str) {
/**
+ * Prefill form fields with information from the visitor cookie informaiton.
+ */
+Drupal.addVisitorCookieInfo = function (form_id) {
formPrepopulateFromCookie
+++ modules/user/user.js 10 Oct 2009 19:36:16 -0000@@ -188,4 +188,10 @@ Drupal.behaviors.userSettings = {
+Drupal.behaviors.userRegistration = {
+ attach: function (context, settings) {
+ Drupal.addVisitorCookieInfo('user-register');
+ }
+};
That's the part I don't like in this approach, because it should be a JS behavior that is triggered by a class name, perhaps optionally accepting JS settings to alter the fields to search for and prepopulate - so we do not require JS in user.js, contact.js, and comment.js at all. But be it, this is at least a step forward.
This review is powered by Dreditor.
#48
It's not a good idea have a lot of .js files. A common JS.behavior would be great
#49
I agree with sun -- good thinking. Let's keep working on the issue some more then. This is a relatively small improvements so we might as well keep working on a proper solution.
#50
@andypost: The point isn't to reduce the amount of .js files, we still have to call the function from somewhere.
So can we agree that maybe the option 1 is ok to commit for now to get the functionality working and we can figure out the best way to handle this with a follow-up?
#51
This should hopefully get you somewhere. Skipped the JS settings part, because that's too abstract for now. Untested.
#52
oops, and apparently, it didn't remove contact.js and comment.js (which can be deleted with that patch)
#53
Quick note - when this patch makes it in, the Poll module's new patch should use it to. Noted here so we don't forget: #237213: Fixed poll code for anonymous voting
#54
Re-rolled for head. Note to committer(s):
cvs remove modules/contact/contact.jscvs remove modules/comment/comment.js
#55
Left out the user.module chunk.
#56
This looks good, but can we change 'form-prepopulate-from-cookie' to something like 'user-info-from-cookie' and formPrepopulateFromCookie to userInfoFromCookie? I think that would be more explanatory. Feel free to mark RTBC after that reroll.
#57
Form class is 'user-info-from-cookie' and JS function is fillUserInfoFromCookie.
#58
Alrighty. Fixed!
#59
FYI the misc/form.js change was missed.
And we still need to run:
cvs remove modules/contact/contact.js
cvs remove modules/comment/comment.js
And commit
#60
Got it now. Thanks!
#61
Automatically closed -- issue fixed for 2 weeks with no activity.
#62
So was this actually committed to Drupal 7 in CVS? If so, we'll use it to tidy up cookie use in #237213: Fixed poll code for anonymous voting - sorry to ask, I guess it was committed, but it isn't clear and no one actually says so.
#63
Follow-up issue: #749748: Contact and comment modules do not prefill with user info from cookie