All edits to contact module categories seem to be performed with a DELETE + INSERT rather than an UPDATE. This not only adds one extra query but also messes up the ID as the table uses AUTO_INCREMENTed IDs.

Also, atm multiple categories can be set to be "selected" in the site-wide contact form's Category drop down. This doesn't make sense and I've added an additional query in the patch to address this.

ah, and I'm sure that there'll be a couple of coding standards niggles :P

Please let me know,
-K

CommentFileSizeAuthor
#24 contact.module_21.patch27.24 KBZen
#20 contact.module.3.patch27.45 KBZen
#18 contact.module_15.patch27.63 KBZen
#17 contact.module.diff235.64 KBZen
#16 contact.module_14.patch26.77 KBZen
#14 contact.module_13.patch23.12 KBZen
#13 contact.module_12.patch23.26 KBZen
#12 contact.module_11.patch8.2 KBZen
#9 contact_8.patch2.23 KBZen
#7 contact_7.patch2.25 KBZen
#4 contact_6.patch2.38 KBZen
#3 contact_5.patch2.39 KBZen
#2 contact_4.patch2.39 KBZen
contact_3.patch1.15 KBZen
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Zen’s picture

Zen CreditAttribution: Zen commented
Status: Needs review » Needs work

Blatant error. Patch recalled :P

-K

Zen’s picture

Zen CreditAttribution: Zen commented
FileSize
contact_4.patch2.39 KB

Updated code.

Cheers
-K

Zen’s picture

Zen CreditAttribution: Zen commented
FileSize
contact_5.patch2.39 KB

Removed '' from around the %d..

-K
P.S and thanks cvbge :)

Zen’s picture

Zen CreditAttribution: Zen commented
FileSize
contact_6.patch2.38 KB

wrong patch submitted :/

-K

Zen’s picture

Zen CreditAttribution: Zen commented
Status: Needs work » Needs review
Dries’s picture

Dries CreditAttribution: Dries commented

Coding style needs work. We don't wrap queries like that.

Zen’s picture

Zen CreditAttribution: Zen commented
FileSize
contact_7.patch2.25 KB

Ah, but I had to try.. Patch attached. Please also look into the following URLs:

Example query with comment: http://drupal.org/node/2497
Pretty much every query: http://drupaldocs.org/api/head/file/database/updates.inc/source

Cheers,
Karthik.

Cvbge’s picture

Cvbge CreditAttribution: Cvbge commented

Still wrapped

Zen’s picture

Zen CreditAttribution: Zen commented
FileSize
contact_8.patch2.23 KB

Thanks :)

-K

Dries’s picture

Dries CreditAttribution: Dries commented

Better yet would be to convert the contact module to the forms API's _validate _execute model and fix this problem while you're at it. Is that something you can work on, Zen?

Zen’s picture

Zen CreditAttribution: Zen commented
Status: Needs review » Reviewed & tested by the community

Sure. Can you commit this now though? I will have to do some reading before getting this sorted.

Cheers
-K

Zen’s picture

Zen CreditAttribution: Zen commented
Title: Contact module category update issue » Contact module category update issue + _validate, _submit conversion
Status: Reviewed & tested by the community » Needs review
FileSize
contact.module_11.patch8.2 KB

Besides the previous fixes:

-Conversion to _validate + _submit model..

-Split replaced by explode - faster

-Fixed typos: Recipient

-Fixed weight defaulting to -10

-Popped mail subject formatting into a t()

-Popped '--' formatting into a t()

Cheers
-K

Zen’s picture

Zen CreditAttribution: Zen commented
FileSize
contact.module_12.patch23.26 KB

The weight issue has been fixed elsewhere.

Besides all the above fixes, this patch:
-does a lot of documentation fixing/rewriting.
-general formatting fixes (which makes the patch look very big :S).
-renamed contact_user_mail form functions to contact_mail_user for consistency.

I unfortunately still haven't got my head wrapped around the use of url() or l(), and the seemingly inconsistent use of the absolute_url flag everywhere. So I've left those as is, and will address them if needed later on.

Thanks
-K

Zen’s picture

Zen CreditAttribution: Zen commented
FileSize
contact.module_13.patch23.12 KB

Just noticed a missing comma. I've also removed the link to the ?q=profile page. This is dependant on the profile module and shouldn't really be here..

-K

Dries’s picture

Dries CreditAttribution: Dries commented
Status: Needs review » Fixed

Tested, reviewed and committed. Thanks.

Zen’s picture

Zen CreditAttribution: Zen commented
Status: Fixed » Needs review
FileSize
contact.module_14.patch26.77 KB

Reopening for related issue. This is also an amalgam of fixes for some of the contact module issues reported + others.. I thought it best to just maintain one patch rather than hop around here and there..

Attached patch:

  • Adds missing validation etc. for contact_admin_delete.
  • Fixes a bug in contact_admin_edit where the edited category name wasn't being displayed.
  • Adds separate status messages for "Category added" and "Category updated".
  • Redirects the user to the home page after making a site-wide contact form submission. [31002]
  • Adds a breadcrumb (that just says "Home" by default) to the site-wide contact page. [38357 - Jax]
  • Shifts settings to a tab in admin/contact. [28060]
  • Reorganises menus as a result.
  • Adds a prominent link on the admin/contact page to the menu module, indicating the presence of a disabled menu link. Since the menu module is optional, I've added a module_exist() that lets the user know that he has to enable the module (if false).. I couldn't think of any _clean_ solutions to add the menu configuration to the same section..
  • Comments all functions.
  • Re-orders all functions in a logical manner (resulting in the monster diff..).
  • Adds typo/language/formatting fixes.

I believe that covers it. Please let me know.

Thanks :)
-K

Zen’s picture

Zen CreditAttribution: Zen commented
FileSize
contact.module.diff235.64 KB

This column-wise diff might ease some of the pain of reviewing :)

Thanks
-K

Zen’s picture

Zen CreditAttribution: Zen commented
FileSize
contact.module_15.patch27.63 KB

Besides the above fixes, attached patch:

  • Removes unnecessary $user global
  • Adds watchdog messages for category addition, updates and deletion. The watchdog messages use the 'mail' category.
  • Fixes the settings link in help.
  • Fixes my erroneous // style.

Please review.
Thanks
-K

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented

Patch applies cleanly and makes lots of nice improvements. Tested the module, everything still works.

However, -1 to the hook_help changes. Your way may be better (looks like it, but not sure, we went around and around on this -- see http://drupal.org/node/26139 for the original patch), however it breaks consistency with all the other core modules' hook_help functions which were auto-generated from the handbook pages. It would be interesting to start another issue to discuss whether sprintf()ing the big long lines of text with HTML is a better approach; if so, a patch will need to be rolled for all core modules.

Also:

- '#value' => $user->name .' <'. $user->mail .'>',
+ '#value' => sprintf('%s <%s>', $user->name, $user->mail),

Why is the second there an improvement? It seems harder to read and is not standard (Drupal tends to use concatenation everywhere).

- // Set a status message:subject
+ // Update user.
drupal_set_message(t('Your message has been sent.'));

Same here.. I get that the first comment is not very clear, but don't see that "update user" accurately explains how that works.

Zen’s picture

Zen CreditAttribution: Zen commented
FileSize
contact.module.3.patch27.45 KB

Thanks for the review webchick :)

Attached patch:
-keeps up with HEAD
-replaces drupal_goto with return (where applicable) as per Adrian's patch. I've used return ''; to implement a return to the frontpage. I trust this is fine.
-removes all sprintfs and replaces with . notation.

I've not done anything about the "update user" comment as it's imo fine as it is. The nature of the 'update' is made evident by the following statement.

Please re-review - thanks :)
-K

Dries’s picture

Dries CreditAttribution: Dries commented

I'd like these fixes to go in. If people can test/review this patch, that would be much appreciated!

Zen’s picture

Zen CreditAttribution: Zen commented
Status: Needs review » Reviewed & tested by the community

I'm going to set this to RTC:

a) Webchick has reviewed the code end of things and was only unhappy about a couple of cosmetic issues.
b) I've fixed and reviewed these issues: i.e. the use of sprintfs.

Thanks
-K

killes@www.drop.org’s picture

killes@www.drop.org CreditAttribution: killes@www.drop.org commented
Status: Reviewed & tested by the community » Needs work

most recent patch doesn't apply.

Zen’s picture

Zen CreditAttribution: Zen commented
FileSize
contact.module_21.patch27.24 KB

ah. My cvs version was stuck at 1.41. I've added in the 1.42 'www.drupal.org' -> 'drupal.org' patch.

Thanks
-K

Zen’s picture

Zen CreditAttribution: Zen commented
Status: Needs work » Reviewed & tested by the community

(patches cleanly against 1.42).

Dries’s picture

Dries CreditAttribution: Dries commented
Status: Reviewed & tested by the community » Fixed

Committed to HEAD. Thanks.

Zen’s picture

Zen CreditAttribution: Zen commented
Status: Fixed » Closed (fixed)