geshifilter_filter should respond to $op=prepare

I just ran a few quick tests with your patch and it seemed to work as well as my patch on my test page with highlighted code, but it does get closer than my patch does at fixing the problems linked to in the original post of this issue: http://drupal.org/node/70904

Specifically, your patch fixes #1, #2, and #4 on the test page at http://www.dvessel.com/node/93

Also, I'm a little worried about your use of 0xFA-0xFD. My understanding is that technically those are valid UTF-8 characters, though they were never assigned to represent anything and thus are very unlikely to be used. This statement is based on the 3rd table of the Description topic at http://en.wikipedia.org/wiki/UTF-8

Once you're done with the patch I'll try to review it more closely.

I downloaded the newest 2.x-dev version and the patch applied, but with offsets. I guess you meant in your last comment that you created the patch before you made the other upgrades to the module? In any way, it applied successfully.

Hunk #2 succeeded at 685 (offset -4 lines).
Hunk #3 succeeded at 715 with fuzz 1 (offset -4 lines).
Hunk #4 succeeded at 839 (offset -46 lines).
Hunk #5 succeeded at 861 (offset -46 lines).
Hunk #6 succeeded at 916 (offset -46 lines).
Hunk #7 succeeded at 1013 (offset -46 lines).

I tested the patch and I had the same results as I indicated for the patch in comment #3.

As for the patch itself:

1. I still think it's best to use xFE and xFF. It's not just codefilter that uses these chars. The Drupal example filter module even suggests using these chars (see http://api.drupal.org/api/function/filter_example_filter/5). Given that the GeSHi filter module should only prepare sections of code that are enclosed by tags, where name is one of the languages that GeSHi has been configured to handle.

   The only potential problem I see is that both codefilter and GeSHi filter might respond to <lang></lang> tags (where "lang" = "code"). In that case those sections of code would be prepared identically (assuming GeSHi filter moves to using xFE and xFF), and so whatever filter was set to come first would do the filtering. The user could get around this by using [lang][/lang] tags, which codefilter doesn't respond to.

   Also, in my original patch I didn't change tags in square brackets (eg. [lang][/lang), but you've done that in your patches. In retrospect, I think you're right to also prepare those blocks.

   So, to summarize, I think it's best to prepare blocks of text that will be processed by GeSHi filter in the following way:

   <lang></lang> ==> xFElangxFFxFE/langxFF  and
   [lang][/lang] ==>  xFElangxFFxFE/langxFF
   

   If you wanted to make sure that only GeSHi filter touched blocks that were prepared by GeSHi filter, you could do this instead:

   <lang></lang> ==> xFEGFlangxFFxFE/langxGFFF  and
   [lang][/lang] ==>  xFEGFlangxFFxFE/langGFxFF
   

2. +function _geshifilter_prepare_callback($match) {
   +  return "\xFA". $match[1] . $match[2] ."\xFB"
   +    . str_replace(array("\r", "\n", '<', '>'), array('', '&#10;', "\xFC", "\xFD"), $match[3])
   +    ."\xFA/". $match[1]. "\xFB";
   

   In this block you're replacing any < and > chars within the block of code that will be filtered with xFC and xFD. I'm not sure why this is necessary or desirable. codefilter.module sends all text between the code tags through a function that replaces \r and \n (as the patch does above) but then sends the text through check_plain(), which then calls htmlentities() and encodes <, &, etc. in their safe form.

3. Likewise, in the process phase, the patch converts xFC to < and xFD to >:

   +  // Undo linebreak and angle bracket escaping from preparation phase
   +  $source_code = str_replace(array('&#10;', "\xFC", "\xFD"), array("\n", '<', '>'), $source_code);
   +
   

   Again, I don't think that's wise. For example, if there were a filter that processed after GeSHi filter, and if that filter did no escaping of text that it would process, it would potentially process the text returned by GeSHi filter because there are unescaped HTML entities left over.

Re:
#1: I think flagging it as conflicting is a good idea. Is it possible to only flag it as conflicting with codefilter if geshifilter is set up to handle <code></code> tags? And only if brackets are GESHIFILTER_BRACKETS_ANGLE or GESHIFILTER_BRACKETS_BOTH? I also didn't see anything in the patch in #7 that adds this warning. It wasn't clear to me if you had added that already or if you were just planning to add that.

#2: I understand. I forgot that GeSHi calls htmlentities() on its own, so I guess calling check_plain() isn't absolutely necessary, but it doesn't seem to hurt anything (now that you call decode_entities() in the process step.

#3:

This last point made me think of another problem however. With the GeSHi filter you have an option to do nothing with the source code by default (when there is no language attribute). This is only detected in the process phase, while it should be done in the preprocess phase, so that there would be no preparation and other filters (like html filter) could do their thing instead.

That's a good point. Does the new patch include detection of such a case? If so, where is it doing that? I must have missed it.

BTW, I tested the new patch and it still works as before. I also tested with highlighted code in a teaser and it was correctly highlighted (I hadn't tested this before).

I think this is getting close.

When I said the patch worked as before, that was a good thing. Sorry for not being clearer. Your responses in #8 all sound good to me. I have no further worries about the patch.

Thanks for working on this so much. This should really help geshifilter work more robustly.