OK, a simpler one, I hope. :-) This patch slices up the contact module into page handler files. contact.module itself is not quite 2/3 smaller when we're done, which is spiffy. :-)

CommentFileSizeAuthor
#9 contact_21.patch35.69 KBpwolanin
#1 contact_20.patch35.64 KBpwolanin
contact_19.patch33.7 KBCrell
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented
FileSize
contact_20.patch35.64 KB

Looks ok overall, but I think we can make the menu part more elegant. See attached. This standardizes the use of a function contact_load(), and also has the benefit that if a non-existent cid is put into the URL for edit or delete, a "not found" page is returned.

note that both the add/edit and delete forms have been updated this way. The confirm form callback is much simplified as a result, and all hackish uses of arg() are abolished.

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented

note - since my patch changes hook_menu, either apply this patch before enabling the module, or enable/disable some other module to force a menu rebuild.

bennybobw’s picture

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

I tested this patch. All the code style looks good, but I found the following bugs:

  • but when I went to a non-existant CID with edit, it gave me the 'add' form but with the title "HOME."
  • When I went to a non-existent CID with "delete" it asked me "Are you sure you want to delete?" with the error: notice: Uninitialized string offset: 0 in /home/bennybobw/www/drupalhead/drupal/modules/contact/contact.admin.inc on line 138. Sorry, I don't quite understand how the page arguments work to fix these two bugs, otherwise I would.
  • Also, if the contact menu item is disabled, if you go to /contact, the title reads "HOME."

That's all for now.

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented

@bennybobw did you force a menu rebuild? The behavior you describe should not occur, and do not occur for me.

bennybobw’s picture

bennybobw CreditAttribution: bennybobw commented

@pwolanin confirmed on clean version of HEAD. I applied the patch before enabling the contact module. Then, just to be sure, I installed everything clean again, enabled the contact module, applied the patch, then enabled the Actions module. I am still getting everything reported above.

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented

hmm, that's odd if I go to a path like "admin/build/contact/edit/99" I get "Page not found".
if I got to "admin/build/contact/delete/99", I also get "Page not found".

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented

however this behavior:

if the contact menu item is disabled, if you go to /contact, the title reads "HOME."

I see also. But this is a problem with the menu system: http://drupal.org/node/155624

Crell’s picture

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

I can duplicate what bennybobw is reporting using pwolanin's patch in #1. However, it looks like it's a case of this: http://drupal.org/node/155624, and is not directly related to this patch. For me, the navigation and title is broken before this patch anyway.

Setting back to CNR, and flagging the title issue as a dupe of the above.

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented
FileSize
contact_21.patch35.69 KB

ah, apparently found the problem - mysql vs mysqli returns FALSE vs. NULL if no data returns in db_fetch_array().

Attached patch adds one line to insure that contact_load returns FALSE if no data is loaded.

Dries’s picture

Dries CreditAttribution: Dries commented
Status: Needs review » Fixed

Tested and committed! :)

Anonymous’s picture

(not verified) CreditAttribution: commented
Status: Fixed » Closed (fixed)