1. It seems like mailhandler 1.x was quite a while ago, you could update the README section "What's new in 2.x" to just a list of cool features available.
  2. The INSTALL refers to docs at the bottom on the file, which takes you to the handbook topic list. The actual doc on PHP IMAP is here https://drupal.org/node/300794.
  3. The object default and export keys in hook_schema() are new to me! Are those documented somewhere? I couldn't find anything at https://drupal.org/node/146939 or https://api.drupal.org/api/drupal/includes%21database%21schema.inc/group.... Quite nifty.
  4. Do you need the mailhandler_init() function? That gets checked on every page request - consider moving it somewhere else?
  5. Should hook_mailhandler_auth_failed() be documented in a mailhandler.api.php file?
  6. Why does mailhandler_default::mailhandler_default_ctools_plugin_api() call func_get_args() 3 times? I don't think they can be different.
  7. In MailhandlerPhpImapRetrieve::get_part() can you use drupal_convert_to_utf8() instead of mb_convert_encoding?

That's it for mailhandler, mailhandler_default, and mailhandler_php_imap, more to come :)

Comments

danepowell’s picture

danepowell commented

1) Good idea, committed.
2) It seems like the handbook overview page is the most logical place to link people to from the INSTALL file. You think it should link directly to a child page?
3) Those are ctools constructs. There's an overview at https://drupal.org/node/928026 , although oddly it doesn't mention the "object defaults" property. That is documented at http://drupalcontrib.org/api/drupal/contributions%21ctools%21includes%21... .
4) This has to be included due to a longstanding bug with ctools. You can witness my 2 years of frustration and capitulation at #1364184: AJAX error when adding a mailbox. Here's the actual bug in ctools: #1365524: Fatal error when using export_ui with AJAX

Let me save this so I don't lose my progress and respond to the other issues in a new comment.

danepowell’s picture

danepowell commented

5) #2129211: Document hook_mailhandler_auth_failed
6) That file was auto-generated by Features, so Features must have been buggy. I re-exported it.
7) Thanks, I didn't know Drupal provided a library-agnostic version of that function: #2129225: Use drupal_convert_to_utf8 instead of mb_convert_encoding

danepowell’s picture

danepowell commented
Status: Active » Fixed

Any remaining issues should have been spun off.

kscheirer’s picture

kscheirer
Primary language English
Location Vallejo
commented

oh ctools :) I updated that issue as well. If there's no action on it, I'll test it out and rtbc it assuming it is still required and still fixes the issue.

2) I just meant that I read the INSTALL as saying "here's a link to help you with PHP Imap", not "here's a link to general mailhandler docs". Not a big issue either way :)

Thanks for the doc links, happy to learn something new!

kscheirer’s picture

kscheirer
Primary language English
Location Vallejo
commented
Status: Fixed » Needs review

In MailhandlerAuthenticateTokenauth::authenticate() you can use user_load_by_mail() instead of array shifting the user_load_multiple.

That's all I could find, very nicely written module! I thought there would be too much code to understand, but all the abstract classes and example implementations made it easy to follow :)

danepowell’s picture

danepowell commented

2) I see what you mean- I updated the INSTALL.txt to clarify this.

I also fixed the tokenauth function- I had already fixed that in the default authentication class, I guess I just forgot to port it over.

Thanks for the compliments, I always enjoy getting positive feedback :)

danepowell’s picture

danepowell commented
Status: Needs review » Fixed

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.