Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.This is a continuation of the final comments in #947266: Consider how to handle comments.
It seems to me that "authentication" (by which I think you mean both authentication and authorization/permission-checking) should indeed happen at the parser level, since this is where it logically makes sense to parse out the identification tag and thus threading information, etc...
Also- this might sound crazy, so bear with me- I think that it would be better if the Mailhandler package stopped at the parser, and allowed the Node Processor included with Feeds to actually post the node (or the Feeds Comment Processor to post comments). It's a bit of a stretch, but the functions currently in MailhandlerNodeProcessor, even command-handling, could be moved to the parser. Then things like content type, published status, and taxonomy terms could be presented as sources for the Node Processor. The only problem is that Node Processor doesn't currently support any of these properties- so a patch would probably be needed. It would delay things, but I think it would be TDW (The Drupal Way) and improve Feeds in the process.
| Comment | File | Size | Author |
|---|---|---|---|
| #6 | mailhandler.tar_.gz | 27.57 KB | Dane Powell |
| #6 | mailcomment.tar_.gz | 15.04 KB | Dane Powell |
Comments
Comment #1
Ian Ward CreditAttribution: Ian Ward commentedDane, I think dropping MailhandlerNodeProcessor in favor of FeedsNodeProcessor is a fair goal. I believe there is just one small (if not insignificant) issue: It's not clear how you'd allow a "command" in the email body declare the node type, since FNP::buildNode uses the processor config value, and at the end runs node_object_prepare which will set some default values determined based on the node type. hook_feeds_node_processor_targets_alter can be used to add in more mapping callbacks, but FNP::map is called after the node is built.
That said, support for setting node type in commands could be dropped, or maybe re-running node_object_prepare could work.
Moving ::authenticate to the Parser seems ok. Ultimately, it seems the same authentication logic can be used, but, with the end goal of setting "uid" as a "source" which can then be mapped to an "author" target which then sets $node->uid or $comment->uid. Determining whether that uid can create the given node type may need to be done from w/in the parser because I think the way FeedsNodeProcessor works the user may be able to post no matter what (goes straight to node_save which I don't think checks access).
What do you see as needing a patch in Feeds?
Comment #2
Dane Powell CreditAttribution: Dane Powell commentedI do also see a problem with being able to command the node content type- for instance, in order to present a list of targets, FeedsNodeProcessor has to know a priori what type of content is being created, since different content types have different available fields. So I think this approach would have to sacrifice that ability as you suggest. However, in all other respects it seems very flexible and elegant.
Here are the list of available Mailhandler commands (that I know of) and whether they would be supported natively by FeedsNodeProcessor:
Anything that can't be configured as a target will need to be supported via a FeedsNodeProcessor patch, or dropped from the available commands (as with content type)
Comment #3
Dane Powell CreditAttribution: Dane Powell commentedAlso I guess this would logically lead to dumping the mailhandler_node submodule, and moving any required code into the core mailhandler module. I think this will make the installation and configuration process much more intuitive and probably isn't a bad thing. I am making good progress on this.
So just to recap and make sure we're on the same page, I need to...
if (!node_access('create'...) to the parser and rename the if_auth_fails description to "If authorization fails...". I view this has a temporary solution- ultimately, I think FeedsNodeProcessor and Feeds Comment Processor should be checking authorization before blindly saving nodes.Any complaints/comments/suggestions? We may lose some functionality here in the short term (as I describe in #2), but I think it will be a much better solution in the long-run.
Comment #4
Dane Powell CreditAttribution: Dane Powell commentedI'm slowly making progress on this- it's turning out to be a pretty massive restructuring of the module- pretty much no file in Mailhandler/Mailcomment is going untouched.
Another thing that bugs me is that the mailcomment functions (a bunch of them) that parse threading/in_reply_to information need to be moved into MailhandlerParser, so that they can provide the corresponding "Parent CID/NID" sources. Ideally, these would stay in the mailcomment module somewhere, and add the sources to the MailhandlerParser via an alter hook (similar to how hook_targets_alter works, but for adding sources to a parser instead of adding targets to a processor). However, as near as I can tell no such hook exists, and the only other way to keep those functions in mailcomment would be to override MailhandlerParser, which is also not ideal, as it's not really intuitive or extensible. So for now, I think I'll keep the functions in MailhandlerParser, and we may need to implement our own custom hook/plugins to make the parser extensible. But I figure that can come later.
I'll keep you up-to-date on progress.
Comment #5
Ian Ward CreditAttribution: Ian Ward commentedDane, this all seems fine. FYI check out feeds/feeds.api.php - there's a hook_feeds_parser_sources_alter for adding sources to parsers.
Comment #6
Dane Powell CreditAttribution: Dane Powell commentedThanks for the tip. Here's what I have so far- this is definitely not fully tested (and probably not complete), but it should give you an idea of where this is headed. It works with FeedsNodeProcessor and Feeds Comment Processor, and includes support for authentication and commands. Since a lot of the include files got shuffled around or deleted, you may get errors if you overwrite an existing installation- at least I did.
Comment #7
Dane Powell CreditAttribution: Dane Powell commentedJust looking ahead... it would probably be good to move the commands configuration (default commands) from the mailbox to the fetcher, parser, or Feeds source. Patches or issue requests need to go to FeedsNodeProcessor to handle the unsupported commands (#2). And the authorization issue needs to be tackled- FeedsNodeProcessor allows even anonymous users to post content.
Comment #8
Dane Powell CreditAttribution: Dane Powell commentedAlright, these changes have been committed! Below are the remaining issues that need to be tackled. I guess with that, this issue can be considered fixed?
#1041626: Move "default commands" and "signature separator" configuration from mailbox to parser
#1041634: Include additional node properties (format, promote, sticky...) as targets in FeedsNodeProcessor
#1041646: Check permissions when creating nodes (FeedsNodeProcessor)
Documentation will also need to be rewritten, but I assume that's sort of an ongoing issue at this point.
Comment #9
Dane Powell CreditAttribution: Dane Powell commentedTagging to keep track of issues related to the mailhandler-2.x/notifications-4.x workflow
Comment #10
tbenice CreditAttribution: tbenice commentedHi Dane, does authentication also need to be provided in FeedsCommentProcessor to prevent unauthorized commenting? For the MailHandlerParser, imports will still run with cron though authorization happens there right?
Thanks alot for this work!