Nicer messages on /contact when there are no contact form categories
Ian Ward - July 15, 2008 - 12:20
| Project: | Drupal |
| Version: | 7.x-dev |
| Component: | contact.module |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Dave Reid |
| Status: | closed |
Description
When you enable the contact form, but do not set up any categories, users without permission to administer the contact form will see "The contact form has not been configured. Add one or more categories to the form." When they click on the link, they get an access denied. I think the the ideal behavior would be to not show users this message at all, and instead show them a 404 (or 403), until the contact form is setup and ready for them to use. Attached is a patch for the 404 option. I put in a "die" right after the drupal_not_found() b/c otherwise it double-themes the page. Perhaps this is not the correct way to do it in this situation.
Ian
| Attachment | Size | Status | Test result | Operations |
|---|---|---|---|---|
| contact_enabled_no_categories.patch | 976 bytes | Idle | Unable to apply patch contact_enabled_no_categories.patch | View details | Re-test |
#1
Ok, I understand the issue there, but the patch needs work.
contact_mail_page()is not a page callback but a form function. We should not exit() from there. My suggestion is to display a slightly different message if the user doesn't have the administer site-wide contact form permission. For example:#2
Hi Damien, thanks for your feedback and pointer.
I feel like it would be good if nothing about the contact form is said to the user if they're unable to do anything with it. What do you think about this new patch, which returns false, which is checked in the page callback itself, which then sets the 404?
Ian
#3
Sorry, disregard that last patch (#2) I need to rework this again.
#4
Here is a new patch, which does what the patch in #2 was supposed to do. The checks on the # of categories is now done in the page callback, and the arguments are passed on to the form.
Ian
#5
Just fixing the status.
#6
The last submitted patch failed testing.
#7
Revised patch:
1. If there are no categories:
1a. and the current user has the 'administer site-wide contact form' permission, will show a blank page with the message "'The contact form has not been configured. Add one or more categories to the form." (with link to add a category)
1b. otherwise, return drupal_not_found(), or a 404 error. This will help search engines to not index this page when it is not ready.
2. Cleans up some of the code in the site-wide contact form:
- $result = db_select('contact')- ->fields('contact', array('cid', 'category', 'selected'))
- ->orderby('weight')
- ->orderby('category')
- ->execute();
-
- foreach ($result as $record) {
- $categories[$record->cid] = $record->category;
- if ($record->selected) {
- $default_category = $record->cid;
- }
- }
+ $categories = db_query("SELECT cid, category FROM {contact} ORDER BY weight, category")->fetchAllKeyed();
+ $default_category = db_query("SELECT cid FROM {contact} WHERE selected = 1")->fetchField();
- if (count($categories) > 1) {- // If there is more than one category available and no default category has been selected,
- // prepend a default placeholder value.
- if (!isset($default_category)) {
- $default_category = t('- Please choose -');
- $categories = array($default_category) + $categories;
- }
- $form['cid'] = array('#type' => 'select',
- '#title' => t('Category'),
- '#default_value' => $default_category,
- '#options' => $categories,
- '#required' => TRUE,
- );
- }
- else {
- // If there is only one category, store its cid.
- $category_keys = array_keys($categories);
- $form['cid'] = array('#type' => 'value',
- '#value' => array_shift($category_keys),
- );
- }
+ // If there is more than one category available and no default category has been selected,
+ // prepend a default placeholder value.
+ if (!$default_category) {
+ if (count($categories) > 1) {
+ $categories = array(0 => t('- Please choose -')) + $categories;
+ }
+ else {
+ $default_category = key($categories);
+ }
+ }
+ $form['cid'] = array(
+ '#type' => 'select',
+ '#title' => t('Category'),
+ '#default_value' => $default_category,
+ '#options' => $categories,
+ '#required' => TRUE,
+ '#access' => count($categories) > 1,
+ );
- // We do not allow anonymous users to send themselves a copy- // because it can be abused to spam people.
- if ($user->uid) {
- $form['copy'] = array('#type' => 'checkbox',
- '#title' => t('Send yourself a copy.'),
- );
- }
- else {
- $form['copy'] = array('#type' => 'value', '#value' => FALSE);
- }
+ // We do not allow anonymous users to send themselves a copy
+ // because it can be abused to spam people.
+ $form['copy'] = array(
+ '#type' => 'checkbox',
+ '#title' => t('Send yourself a copy.'),
+ '#access' => $user->uid,
+ );
Those are the only changes in the form code, and it really helps simplify the code while keeping the original functionality.
#8
#9
The last submitted patch failed testing.
#10
This one should pass all the tests.
#11
Committed to CVS HEAD. Thanks.
#12
Automatically closed -- issue fixed for 2 weeks with no activity.