It is folklore that using regexps to parse html is error-prone.
There are several parts in mimemail that do just this.

Example: mimemail_extract_files converts any text

src="somefile"

which points to an existing file to an embedded file, even if this is user text, not markup.

Similar with mimemail_html_to_text.

We should change this to do real parsing
See eg http://htmlparsing.icenine.ca/

Comments

dman’s picture

dman commented

While we can agree in theory that regexps are limited, and generally the wrong thing to do to SGML etc... parsing uncontrolled HTML properly is a non-trivial task.

Can you suggest which alternative approach you would use?
- That's php4-all safe on hosted servers without optional PHP (like PEAR) extensions installed?
- And will round-trip existing code without doing weirdness with content-encoding and whitespace?
- Without killing performance 20-fold?

Me, I'm a huge XML fan, and have fought with dozens of hosting environments to enable semantic parsing within Drupal. But I don't know the answer.

I'd welcome a workable answer.

geek-merlin’s picture

geek-merlin
Primary language German
Location Freiburg, Germany
commented

To be honest i dont know much about php solutions for this.
I come from the java and python worlds where those parsers are part of the standard libraries.
Some googling around for "php mimple html parser" tells me there is code enough, but i dont know much about its performance.

Another idea could be to do "enough parsing":
While a simple regexp is bound to break, sometimes simple twostep process is enough:
First separate tags, comments and text; then make further processing.

The problem looks similar to js compression: there are working two step processes that do the trick, but i dont know a singlestep process without bugs.

dman’s picture

dman commented

Because PHP4 basically predates widespread support for DOM manipulation (unlike newer languages) it's a bit tricky to wrap this process into Drupal with its support base.
There are indeed many libraries and snippets available ... but that illustrates that everyone has had to come up with their own approach. You may find that most of them are regexp jam on the inside, not input tokenizers (which is what I'd call 'real' parsing)

But something on that scale is not really mime-mail's job, I'd think. Including such a library (assuming a good one could be selected) would sorta be wrong for the module.
Good news is that PHP5 has a stable-enough DOM model you can use instead - once everyone's on 5 and Drupal drops support for PHP4. It still may be overkill to launch up a DOM object, query it through xPath to change an attribute string and then re-serialize it though!

Yes, a two-step process is sometimes better than a simple regexp. I use regexp callbacks a lot.
However, a good regexp is still just a 1-liner, and if it works well enough, many times that's enough.
...compared to a dozen lines of code to 'do it right'.

If replacing the src safely is all you want to do, a less naive regexp could be

/(<[^<]*src=['"])([^'"]*)(['"][^>]*>)/

untested, and will not fire on unquoted attributes (bad code) ... but basically ensures the src was inside a tag. It's all about how many situations the author was able to predict at the time or for their purposes.

Yeah, I know, we just end up piling special cases on top of special cases this way, but if it works in all but extreme edge cases, and the alternative is daunting, I'd advise being lazy ...

geek-merlin’s picture

geek-merlin
Primary language German
Location Freiburg, Germany
commented

an example:

    $split=preg_split('/<(.*)>/Us',$html,-1,PREG_SPLIT_DELIM_CAPTURE);
    // we now have pairwise text / tag (tags maybe comments)
    foreach($split as $index=>$fragment)
    {
        if($index%2==0)
        {
            //Text
        }
        else
        {
            //Tag
            if($fragment{0}=='!')
                // comment
            elseif($fragment{0}=='/')
                // closing tag
            else
            {
                //Extract attributes
                $parts=explode(' ',$fragment);
                $tag=strtoupper(array_shift($part));
                $attributes=array();
                foreach($parts as $part)
                    if(ereg('^([^=]*)=["\']?([^"\']*)["\']?$',$part,$attval))
                        $attributes[strtoupper($attval[1])]=$attval[2];
                // now we can process $tag, $attributes
            }
        }
    }
ericnielsen’s picture

ericnielsen commented

What if HTML to text processing is delegated to the HTML to text module? At least the HTML to text processing does not appear to be, IMHO, the main scope of this module.

Greetings.

allie micka’s picture

allie micka
she/her
commented

Can someone, in clear terms, explain how this is a bug? What are the adverse/negative effects introduced by the current parsing? If none, then I suspect this is rather a feature request. If this is a feature request; can someone, in clear terms, explain the desired outcome and benefits?

FWIW, The HTML to Text module and D6 implementation have the same roots as the code in Mime Mail.

geek-merlin’s picture

geek-merlin
Primary language German
Location Freiburg, Germany
commented

dman, you are right in this case.
i am quite sure one can prove that for valid xhtml this is as strong as a full parser.
+1 for your code.

dww’s picture

dww
we/he/they
commented

The bug is that the current regexp is too permissive, and any occurrence of src="somefile" gets munged by the link converter. One solution is to make the regexp more precise, as suggested by dman in #3.

However, I'd be inclined to agree with ericnielsen in #5 and say that mimemail should get out of the business of trying to do html_to_text conversion, and farm that job out to http://drupal.org/project/html_to_text, since a) that module already does a better job of it and b) that's exactly how this is going to work in the D6 port of mimemail.

Therefore, if I were Allie, I'd mark this particular issue "won't fix" and bump the priority on #258026: Rip out html_to_text code and depend on http://drupal.org/project/html_to_text to critical. ;) But, I'll leave that decision in her able hands...

Cheers,
-Derek

allie micka’s picture

allie micka
she/her
commented
Status: Active » Closed (won't fix)

Per dww's ( and my ) call, wont-fixing this and focusing on http://drupal.org/node/258026

Thanks!