Consider how to handle comments

I'm considering the following approach:

* Modify mailhandler_retrieve_message so it accepts a callback, where the callback is called with the $header as an argument and which should determine whether or not to "fetch" the message, or leave it in the mailbox, unread.
* Use ctools to let such "message filter" callbacks to be registered
* Add a setting to the default mailhandler parser which lets a "message filter" be selected on a feeds importer basis.
* Use separate processors, for nodes, for comments, for X.

Historically, there was a problem with actually "fetching" the messages using the fetcher, having to do with the use of the batch API. I will re-evaluate the possibility to make the fetcher really fetch, in which case this "message filter" setting would be selected as a setting on the fetcher, which makes more general sense.

What this means to the end-user. They'll create a mailbox, then create an importer. When they configure their importer they'll need to, besides map sources to targets, select the "message filter" to use. The defaults available will be:

* All messages, no filtering
* Messages with in-reply-to (considered comments, and would use the comment processor)
* Messages without in-reply-to (considered nodes, for example, and would use the node processor)

Based on this, if you want to process comments and nodes, you'll setup two importers which read from a single mailbox, but which are configured to use different "message filters" and optionally different processor mappings.

Sound sane? The ability to ship mailhandler with default/example importers already configured will ease a lot of the configuration process.

"and optionally different processor mappings." <--- should read "and would use different processors, and optionally different processor mappings"

To clarify ambiguity, "callback" would likely be this:

* MailhandlerParser calls mailhandler_retrieve. mailhandler_retrieve will take an additional argument to its present signature, which will likely be a function literal to be passed to mailhandler_retrieve_message and invoked with call_user_func_array. The key is, the parser (or the fetcher, if I can make the fetcher really fetch) would pass in which callback to use, which lets the message filter strategy differ on a per-importer basis, but still while using a single mailbox.

The fetcher now fetches and the parser parses, as of http://drupal.org/cvs?commit=479678 and seems to be working fine, including when there are file attachments, with a small fix. This change is mentioned above. A patch can now be made along the lines above.

Dane, just a quick post for now. Yes, right, they'll select "Node" mailbox and this mailbox will then have a feeds importer for handling the nodes, and a feeds importer for handling the comments. The usability on these forms is pretty bad right now because there's no form messages or redirection. Basic mailbox is like if someone wanted to write another module or feeds plugins which does something besides create nodes, but it's also the "base" for any mailbox type, containing the basic info needed to handle connections and reading of the mailbox.

For your first question, go to /import once you have an importer setup and you'll see you can choose a mailbox to import from. Or, if you attached the feeds importer to a node type, you'd create a new node of that type, on which you'll have a select box to choose which mailbox it imports from.

The concepts are all a little disconnected, but they're very powerful when combined.

Dane, this is great. My specific thoughts on your comments:

#1 I think the usage of functions instead of classes is fine, for the filter callbacks. It's simple.

#2 what if in mailhandler_retrieve, the $messages array checks whether mailhandler_retrieve_message returns a message or false? I'm not positive what actually marks a message as read in the imap functions - imap_delete will mark it for deletion, but, marking it simply as "read" I'm not sure, so will require some investigation. Keep in mind this should be tested for POP and IMAP connections because POP message ids versus imap id's work differently and behave differently between connections. For example, from http://php.net/manual/en/function.imap-delete.php "POP3 mailboxes do not have their message flags saved between connections, so imap_expunge() must be called during the same connection in order for messages marked for deletion to actually be purged. "

#3 I think they could stay in mailhandler.

#4 Yes, mailcomment can probably be converted to work with this pretty easily.

Additionally, what do you think about putting the select box for which filter to use on the configForm instead of the sourceForm, so that the settings is saved per feeds importer as opposed to per-instance of that feeds importer (like on a node which that feeds importer is attached to, which is what happens with sourceForm).

Another thought on your #2 in my comment #12 - the testing should also test when multiple sessions are used to import (where batch actually makes multiple connections to the mailbox) since this is where different behavior occurs between POP and IMAP.

Committed! Thanks Dane. I tested multiple sessions. What I meant by that was that multiple batch requests are made. I did this by turning down FEEDS_NODE_BATCH_SIZE to just 2, from 50, so that the multiple batch requests would be made when testing with 5 messages, 3 of which have in-reply-to headers and 2 which don't. Worked fine with both POP and IMAP. I believe the key is what you say above, that the stream is closed an messages read are expunged. There used to be problems with when a new stream was opened and closed for every single message.

About MailComment - see how in MailhandlerNodeProcessor uses mailhandler_node_feeds_node_processor_targets_alter and adds in_reply_to. Perhaps this can be removed, and worked into FeedsCommentProcessor. That said, MailComment has a particular way of checking permission to actually post the comment based on the headers in the signature, as you know.

It may be worth considering whether MailhandlerNodeProcessor could happen in MailhandlerParser, and whether that would let comment processing and node processing importers both use the ::authenticate() plugins. I'm a little distracted to think more about this right now, but will think about it later. Can you open a new issue w/ the path you're seeing?

Thinking about this a little more, I guess there are three options:

Option 1) Move authentication ::authenticate() into MailhandlerParser. Of all the existing authentication plugins, they all just need header information, and to be able to set uid. Uid could be set as a source, and then mapped in the processor mapping. Authenticating in the parser does feel a little strange, but it should work ok. I mean, you wouldn't prevent any processor from acting on all parsed messages - just turn off authentication in the parser if that's what you want. This would then let MailhandlerNodeProcessor and FeedsCommentParser perhaps use the same parser - MailhandlerParser, when trying to handle comments ala MailComment-style.

Option 2) Create a MailhandlerProcessor from which MailhandlerNodeProcessor and optionally FeedsCommentProcessor inherit. May not save much code or make a ton of sense.

Option 3) Implement an authenticate() into FeedsCommentProcessor, in which case it make sense to just make a separate MailhandlerCommentProcessor (or MailCommentCommentProcessor) since we'd be getting a little more implementation-specific than FeedsCommentProcessor should be.

I'm leaning toward #1, except for the "feels a little strange" part... but then again, I don't think any other feeds "source" type implements any kind of authentication based on parameters from parsed information, so, there's not much precedence.