Use PHP5 DOM extension if available (+performance)

I have just started trying to implement Glossify and this patch is very relevant as I'll need all the performance boost I can get.

However on applying #2 I immediately got WSOD errors due to a couple of problems:

• the use of 'exclude_tags' instead of 'excl_tags' as an array index in some places.
• the absence of the first required parameter in calls to _fetch_possible_keywords in a couple of places.

I am working on several bug fixes/enhancements for other issues with the module in general but it looks like they would best be applied to the changes proposed by this patch, so I am planning to take that approach, hence more comments will follow when I have devised them.

By way of preview, the issues I've found so far with the unpatched 6.x-3.x-dev are:

• Selecting Link and Hovertips together does not work if the only-first option is also enabled: the first match is only processed for Hovertip and then the "no further action" flag is set, preventing Link processing. The "no further action" flag needs to be handled in a more sophisticated way.
• Case-insensitive Link processing simply does not work, I think because strpos() is always used to test the presence of a match.
• Matching text is replaced with the term title causing unwanted capitalisation if the term's node title has capitals but a case-insensitive match is required; the original text should be retained within the inserted link.
• Once Link and Hovertips are working together, any matching words in the hovertip pop-up text still gets processed for links.
• My end user does not want comment text to be processed at all. Looking back at the issue history, this may have been added quite recently, hence a configurable option seems sensible.
• It also appears that although links are processed in comments, Hovertips do not appear to work fully, due I think to the relevant hidden divs not being added to the page. If the comment includes a term that is in the main node body, the hovertip pop-up works from the comment text.

As previously advised I have spent several days sorting out some problems with both the original 6.x-3.x-dev code and the above patch. I offer a somewhat reworked patch with which I believe I have advanced things in general by the following changes and fixes:

• Use a function to build the regular expression for keyword matching consistently (some preg_match() statements in above patch did not apply unicode mode or case-insensitivity even if selected by configuration).
• Use backreferences with preg_replace() so that the linked or hoverable text created by a case-insensitive match is not changed to the case of the matching term.
• Provide three operational modes: standard filter mode as default; a non-filter mode which operates directly on node and comment body text using the nodeapi and comment hooks; a variation of non-filter mode in which no processing of comment body text is done.
• Generate (hidden) hovertip output for matches within the comment body if using standard filtering or non-filter mode with comment processing selected. This is added to the comment body HTML but the existing hovertip jQuery repositions it as children of the body element along with the hovertip output for the node body itself. NOTE: to achieve this, a custom theme function glossify_theme_comment_view() has been introduced, so this could be an issue if your theme already overrides theme_comment_view.
• Allow the same keyword terms to be inserted for different configurations - this was already workable in all the code except the first query of the 'insert' operation of _keyword_table().
• Handle first-only setting correctly, applying the rule to each configuration separately.
• Use variable name $text_node instead of $node in some functions to avoid confusion between DOM text nodes and Drupal node object.
• Rebuild xpath node set if a phpdom replaceChild() has happened - otherwise any attempt to loop round the node set a second time appeared to causes WSOD error on re-referencing the replaced text node?
• Create $node->content array item "glossify_all_hovertips". This can be used instead of the separate $node->content['glossify_hovertip'][ ] array to theme the node's hovertips from a single template variable (useful in any node template that does not use the $content variable but themes a node field-by-field).
• Apply some basic caching in _fetch_possible_keywords() as that can be called more than once for the same config and node id combination when processing different styles etc.
• Avoid some unnecessary code repetition.

I have tested Links, Reference and Hovertips successfully but I realise there may be scenarios I did not hit.

Version changed and category changed in view of the fact that I believe I have fixed some bugs.

A problem was discovered in using appendXML() if the text being processed had unescaped ampersands, which I think is possible if using the "non-filter" mode or perhaps if the Glossify filter were sequenced before the HTML filter, which it is possible to do.

Revised patch attached.

Hmm... do you mean an existing "&" might get turned into "&&" ?

I attach another revision of the patch. It contains significant differences from the previous one. The main reason for this is that when testing "first-only" mode, I discovered that on certain versions of PHP and/or libxml, the sequence in which the xpath "//text()" query retrieved the text nodes was not consistent once the DOM had been changed. After a hovertip replacement had been performed for a keyword match (which creates child span elements), the next xpath query on the revised document (to perform a link replacement) was encountering the keyword (now within the new child) out of sequence, causing a second occurance of the keyword in a following text node to be replaced with a link instead.

I was not able to find a documented bug report for this libxml behaviour (guessing that libxml may be to blame), so my solution for this was to replace the main xpath usage with a routine to traverse the document using DOM functions. There is a new function to recurse down the DOM tree, although I retained the initial xpath query to see whether there are any text nodes to process at all. Note: in DOM mode I moved the test for iframe and object elements to a point in this traversal code so that all children of these elements are ignored. Previously these elements were just tacked on to the include / exclude functionality, which doesn't necessarily make much sense as they are unlikely to be direct parents of text nodes. This anomaly is still the case for simplehtmldom mode though, and I haven't got unlimited time to sort that out too.

Along the way I also noticed another issue with the include / exclude functionality. Briefly, to be able to enforce it correctly if performing hovertip and link replacement (which is one of my requirements), it is necessary to identify the "true parent" element of a text node, i.e. when a keyword has been given a span parent to achieve hovertip functionality, the span should not be used to check include / exclude. The test should use whatever was the parent of the text node before any replacement happened. Again this has only been factored in for DOM mode.

I have also tried to improve the check in comment 7 (above) with a negative lookahead assertion in the regex. I tested it as best I could and I think it's effective.

So hopefully this patch fulfils my aim which was to make all the declared functionality work fully, at least in DOM mode, while getting the performance benefits initiated by omerida.

I plan to be rolling out Glossify with this patch live on a UK commercial site within a day or so, because with these fix-ups, it's the best solution for the requirement.

Thanks, I've put my patched version live yesterday (see an example where the bold+italic links have been inserted by Glossify). It appears to be working OK with the desired settings (match first-only, generate hovertip and link, case insensitivity, unicode compatibility, don't break words, exclude a, b and strong tags).

Fortunately my boss is tolerant of the time it's taken to get it happening :)

An less-extensive change to the patch: I found that if a text DOM node in a body field had following siblings, they were being ignored if that text node was updated to process a keyword match. A typical scenario would be if first-only mode is disabled and a page contains two <p> elements both containing a matching keyword. If the first <p> has an <a> link before the occurrance of the keyword, the text containing the keyword was being skipped, even though matching text in the second <p> would be picked up.

This version now saves any following sibling of a text node before any replacement happens, and passes it back to the calling code in that situation. The calling code checks whether it should use that in preference.

guys, would be great if you'd like to co-maintain the module because currently Im concentrated on other projects...

I have thought about the above suggestion but despite using and developing with Drupal for 4 years I have yet to get into that and I am not sure I can commit the time to (co-)maintaining as I realise that means folowing-up all issues in a timely manner, not just the ones I'm personally interested in.

I like this alot-- and want to get rid of the simplehtmldom dependency anyway. I'm still familiarizing myself with the code, but it's a safe bet this will be one of the first things rolled into the current dev.

patch applied to 6.x-3.x-dev branch, let the bug fixing begin.