Remove contact_site_page() and contact_personal_page() and use the forms directly [#601016]

Currently in contact.module for the contact form callbacks, we have a page callback which then calls drupal_get_form. This process is not really necessary as we can just move our flood control checks into the form building code and call drupal_get_form directly from the menu.

This also removes the e-mail validation from the code in contact_personal_page because e-mail address have to be validated whenever users are registered or edited, so checking here is ridiculous.

Comment	File	Size	Author
#2	601016-contact-remove-pages-D7.patch	7.04 KB	Dave Reid
#2

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

sun

German

Karlsruhe

CreditAttribution: sun commented 10 October 2009 at 17:09

Issue tags:

-D7 API clean-up

+API clean-up

Comment #2

Dave Reid

he/him

English

Nebraska USA

CreditAttribution: Dave Reid commented 10 October 2009 at 17:14

Status:	Active	» Needs review
Issue tags:	-API clean-up	+D7 API clean-up

File	Size
601016-contact-remove-pages-D7.patch	7.04 KB

And here is what the patch would look like.

Comment #3

Dave Reid

he/him

English

Nebraska USA

CreditAttribution: Dave Reid commented 10 October 2009 at 17:14

Issue tags:

-D7 API clean-up

+API clean-up

Fixing cross-post.

Comment #4

Dries CreditAttribution: Dries commented 10 October 2009 at 18:30

This looks like a reasonable clean-up. Some quick comments.

+++ modules/contact/contact.pages.inc	10 Oct 2009 17:13:50 -0000
@@ -6,40 +6,33 @@
+function contact_site_form($form, &$form_state) {
+  global $user;
+

The global $user seems unnecessary. It is not used in this function.

+++ modules/contact/contact.pages.inc	10 Oct 2009 17:13:50 -0000
@@ -162,57 +155,54 @@ function contact_site_form_submit($form,
+  if (!flood_is_allowed('contact', $limit, $window) && !user_access('administer contact forms') && !user_access('administer users')) {
+    drupal_set_message(t("You cannot send more than %limit messages in @interval. Please try again later.", array('%limit' => $limit, '@interval' => format_interval($window))), 'error');
+    return drupal_access_denied();
+  }

Ideally, the flood checking would be done in the _validate() callback. Can be done in a follow-up patch.

Comment #5

Dave Reid

he/him

English

Nebraska USA

CreditAttribution: Dave Reid commented 10 October 2009 at 18:36

Actually the $user global is used quite a lot in the contact_site_form builder:

  if (!$user->uid) {
    $form['#attached']['library'][] = array('system', 'cookie');
    $form['#attached']['js'][] = drupal_get_path('module', 'contact') . '/contact.js';
  }

  $form['#token'] = $user->uid ? $user->name . $user->mail : '';
  $form['name'] = array(
    '#type' => 'textfield',
    '#title' => t('Your name'),
    '#maxlength' => 255,
    '#default_value' => $user->uid ? $user->name : '',
    '#required' => TRUE,
  );
  $form['mail'] = array(
    '#type' => 'textfield',
    '#title' => t('Your e-mail address'),
    '#maxlength' => 255,
    '#default_value' => $user->uid ? $user->mail : '',
    '#required' => TRUE,
  );

I'm not sure about moving the flood checking to validation, because if it's enabled, the user would still see a form and think they could submit it, only to be stopped by validation. Definitely worth discussion an a new issue since it wouldn't change APIs.

Comment #6

andypost

he/him

Russian

CreditAttribution: andypost commented 10 October 2009 at 21:34

+1 to this patch.

Validation for flood should be done in form definition to not confusing user about his abilities.

Comment #7

andypost

he/him

Russian

CreditAttribution: andypost commented 10 October 2009 at 22:17

Status:

Needs review

» Reviewed & tested by the community

Tested and find very good cleanup.

Comment #8

sun

German

Karlsruhe

CreditAttribution: sun commented 10 October 2009 at 22:37

Please ignore this follow-up unless you are Dries.

+++ modules/contact/contact.module	10 Oct 2009 17:13:50 -0000
@@ -86,15 +86,16 @@ function contact_menu() {
   $items['contact'] = array(
     'title' => 'Contact',
-    'page callback' => 'contact_site_page',
+    'page callback' => 'drupal_get_form',
+    'page arguments' => array('contact_site_form'),
     'access arguments' => array('access site-wide contact form'),
     'type' => MENU_SUGGESTED_ITEM,
     'file' => 'contact.pages.inc',
   );
   $items['user/%user/contact'] = array(
     'title' => 'Contact',
-    'page callback' => 'contact_personal_page',
-    'page arguments' => array(1),
+    'page callback' => 'drupal_get_form',
+    'page arguments' => array('contact_personal_form', 1),
     'type' => MENU_LOCAL_TASK,

@Dries: hah, actually, this looks like another potential use-case for 'wrapper_callback' -- the same form used for two different locations in two different contexts, and the wrapper may be able to setup the context, so the actual form builder can handle it accordingly. Not sure whether that's doable. Just mentioning. We just don't know yet. ;)

#571086: Allow to specify a 'wrapper callback' before executing a form builder function

Comment #9

Dries CreditAttribution: Dries commented 11 October 2009 at 01:07

Status:

Reviewed & tested by the community

» Fixed

Dave, you're right. Committed!

sun, thanks for pointing that out. That helps.

Comment #10

Dave Reid

he/him

English

Nebraska USA

CreditAttribution: Dave Reid commented 11 October 2009 at 01:14

Awesome. FYI the next big contact issue is #58224: Allow anonymous users access to a members personal contact form now that all these cleanups are done. Much cleaner and smaller patch will be easier to review!