Closed (fixed)
Project:
Mime Mail
Version:
7.x-1.x-dev
Component:
Code
Priority:
Normal
Category:
Task
Assigned:
Unassigned
Reporter:
Created:
13 May 2008 at 17:31 UTC
Updated:
20 Aug 2008 at 04:03 UTC
Jump to comment: Most recent file
As part of the port to D6, Allie is planning to use http://api.drupal.org/api/function/drupal_html_to_text/6 which has different functionality than the html_to_text code we've already got in here. Some things it handles much better. Others are a regression in functionality (#258007: Expose setting for the inline vs. footnote handling of links in plain-text emails). To help unify the D5 and D6 features, and to get a head start on D6 porting, should we just rip out all the html_to_text code in mimemail, add a dependency on http://drupal.org/project/html_to_text and use drupal_html_to_text() in the D5 version?
If you're ok with this plan, I'll work on a patch, since, in spite of the problems with the inline vs. footnote handling for links, some of the other formatting that html_to_text gets right is stuff I need in the short term.
| Comment | File | Size | Author |
|---|---|---|---|
| #4 | mimemail_outsource_html_to_text.2.patch | 6.81 KB | dww |
| #2 | mimemail_outsource_html_to_text.patch | 7.05 KB | dww |
Comments
Comment #1
allie mickaRather than tacking a new dependency into Mime Mail during its D5 sunset, I'd prefer to just steal the code from D6 and/or the html_to_text module. This satisfies your goal of near-term fixes without introducing new complexity and tasks for D5 maintainers.
For D6 we'll simply remove the function.
Comment #2
dww"Steal the code" sounds like a bad move -- fork-a-licious. :(
Let's just depend on it. It's not that much to ask. We can just document the dependency in the release notes. If you really want, I could add a hook_requirements() that makes sure it's enabled, and/or a DB update that warns you about it.
Anyway, here's a patch. Seems to work great, with 1 slightly weird thing (which I'm currently discussing with chx in #drupal-dev): The old mimemail_html_to_text() code had logic to manually strip out
<style><code> and <code><script>tags. drupal_html_to_text() doesn't do this, so I was getting partially munged CSS files in my test emails. :( So, I ended up leaving a tiny shell of mimemail_html_to_text() in place to just rip those two out, then hand off the rest of the work to drupal_html_to_text().Comment #3
allie mickaIn principle, I totally agree.
But in practice, why create a new dependency as the last thing we do with 5.x? This creates an upgrade/annoyance hurdle that will limit people's interest in upgrading a module and testing new changes. Then, to move to D6+, they have to figure out what this thing is and then deal with supplanting it.
While that's not a lot of work for someone who knows what they're doing, it's creating a disproportionate amount of work for such a small - and temporary - gain.
Meanwhile html_to_text is nothing more than a fork unto itself, except that it has all of the extra module baggage.
Comment #4
dwwOk, fork it again, then. ;)
I kinda see what you mean, though I personally don't care that much. If users don't want to upgrade, they don't have to. If they do, they can install this other module and get the new goodness.
If you want to re-fork html_to_text(), and be in the business of maintaining the other D5 backport of it, just so your users don't have to install another module, more power to you (though, by that logic, why force send users to install mimemail in the first place?). But, I've got what I need in the form of this patch, so I'm done here...
p.s. Latest patch here is because I didn't realize you were expanding URLs in the full HTML version, too, so I didn't see that in my initial testing. ;)
Comment #5
jerdavisBased on Allie's recommendation, I've taken the mail.inc for the html_to_text v5 backport and included it here in mimemail as html_to_text.inc. I've renamed the functions therein in order to prevent name-space collisions. While it may mean some additional work keeping things up to date having effectively forked that file from html_to_text, it should be negligible. The html_to_text backport is relatively straight forward, and chx has not updated it since February.
Should it receive additional work, I'll transfer that over to the v5 branch of mimemail.
For v6 and beyond, we'll just call drupal_html_to_text() directly.
Comment #6
Anonymous (not verified) commentedAutomatically closed -- issue fixed for two weeks with no activity.