URL filter breaks generated href tags

Here is the filter.module attached

And here is a file that can be used as testdata.

Attached is the same patch against the most recent HEAD / 6.0

So no you are all set to go to review and apply this on any system you wish.

Oh right, sorry about the patch. Here is a diff -up patch against head attached.

About the 6 vs 7 issue. I would like to point out that this patch contains absolutely no new features. It does the same replacements as the current urlfilter module (the core regexp patterns didn't change) but it does the replacements in some new places where the current one doesn't but imho should (inside many new tags) and otoh it doesn't touch several places the current one shouldn't but does. In this sense I would argue this fix is within the scope of "During the initial stage of the code freeze, documentation updates, usability improvements, and performance improvements will still be accepted. New functionality or API changes / additions, on the other hand, will not be unless deemed critical. As of today, the focus is to strengthen the code base's performance, usability, and stability. As we progress, focus will shift towards stability and, near the end of the code freeze, only bug fixes will be allowed, until no release critical bugs are left." (http://drupal.org/drupal-6.0-code-freeze)

I do realise this patch is not a simple one-liner bug fix and the URL filter has worked fine for many of you for many years now. So if you won't include this in 6.x I'm not going to whine about it more than this, but I do want to stress that nothing new is introduced in this patch, it is a bug fix.

I'm changing version back to 6.x-dev just to get attention from the right people. Please just change it back to 7.x if that is your judgement.

Hi again. Thanks for the good comments. It took me a couple of days to go through them, but now I'm actually back with two new patches.

Attached here is a patch 0.3 that corrects all of the minor problems you have pointed out for 0.2. In particular:

Also, the following is just not acceptable:
//note: and
, won't work. While valid xhtml their use is discouraged anyway

Actually, it seems that comment was wrong and those tags actually did work. The comment is now removed. However, the regexp at that position has now been reworked to be a bit more precise than before, since I realised I had tought about it the wrong way.

Also, from a code-style standpoint, please ensure there is one space between // and the comment and that the comment uses sentence capitalization. For example:

Done.

And here is the other patch, addressing the final suggestion made by Steven.

Please not that this patch is not intended to be reviewed or included anywhere. I'm just attaching it as an attachment to the explanation that follows. Please consider 0.3 for inclusion into Drupal core.

If the goal is to make it context-aware, then I don't see why we don't do a full tag split like other core code does (e.g. htmlcorrector, the search indexer, drupal_html_to_text). That way we can simplify the extremely complicated regular expression into a simpler, more readable heuristic. Ignoring any content between tags or ignoring any HTML attributes would be a simple if check at the right point.

In the attached patch I tried to follow this path instead. It is almost a complete rewrite of the whole _filter_url_filter() function. (Whereas patch 0.3. actually is more like additions onto the simple regexps in current HEAD.)

The problem is that URL filter actually makes 3 different replacements. It makes links out of

1) http://any.full.url
2) email@addresses.com
3) www.domains.com

The problem is that once you do the split (you can just split on all tags, it's really simple)
(1) works fine,
(2) works fine since it (practically) never confilcts with (1),
but then at step (3) you end up actually hitting all of the addresses that were converted in step (1), thus messing up the links created in (1). So the problem is that while we preg_split away all tags present in the input, (1) and (2) create new <a> tags and there is no new split function and then you mess everything up at (3).

Possible solutions to this dilemma:

1) Make the last regexp more intelligent/complex so it doesn't do this -> This would end up being the "complex" regexp used in patch v0.3, kind of defeating the whole point of the excercise: Then we'd have both more complex code and the complex regexp.

2) Separate the 3 regexps so that only one is done at a time. For instance have a loop that goes 3 times, doing the preg_split anew for each loop. This way the new <a> tags created in steps (1) and (2) would become protected at step (3).

I personally am of the opinion that while the regexps in v0.3 may be a bit wordy, the complexity of the if-else structures of v0.4 are not that simple either. Adding another loop or other logic doesn't seem worthwile to me - I'd rather vote for v0.3 where at least the code is straightforward, albeit the regexps may be on the long side.

(Another comment on patch v0.2)

The combined regular expressions that result from all this code seem incredibly convoluted, and contain slow, ungreedy operators.

Is the slowness of a regexp in a filter an issue, since the filter is cached? I do admit that URL filter is one of the most used ones, but nobody runs filters without having the caching on, right?

Whether the regexps are readable or not is of course a matter of taste, to the author everything always looks simple. But I'd argue the if-else logic in v0.4 takes about as much time to understand as the regexps in v0.3. (v.0.4 also took longer to get right than modding the regexps took for v0.3.)

So in summary, I'm still proposing the patch v0.3.

Small restructuring of the regexp in $tags_begin variable. (Refinement based on patch 0.3, see previous comment.)

Changes line 1089 (line 10 in this patch)

  $tags_begin = '<\/?(?:' . $tags_begin . ')(?:\s[^>]|\/)*>';
  // This pattern will match <p>, </p>, <p class="foo">, <p /> and <p/>
  // The last ones are significant for xhtml standalone tags like <img/> and <br/>
  // Note: the pattern will obviously match many invalid tag-like constructs too 
  // like <p &&&>    

Removes the for-loop that did the same transformation around each tag separately.

This change makes the regexps a bit shorter, so should be slightly simpler to read and also to execute.

Oops, forgot to change status...

How should silence in a "needs review" situation be interpreted?

Where is 6.x at the moment, is it still realistic to target this for 6.x or are we close to RC time?

Hi shap

Thanks for testing this.

Unfortunately, I cannot reproduce what you are getting. I tested on a vanilla 6.x-dev, with 0.5 patch applied. For instance,

This is just a www.test.com. paragraph with person@test.com. some http://www.test.com. urls thrown in and also <code>using www.test.com the code tag<  /code>.

<blockquote>
This is just a www.test.com. paragraph with person@test.com. some http://www.test.com. urls thrown in and also <code>using www.test.com the code tag<  /code>.
</blockquote>

<code>So what's this about? http://www.test.com abc<  /code>

http://www.test.com
www.test.com
person@test.com
<code>www.test.com<  /code>

All of the other url's get modified, but the url's inside the code tags are not modified.

On the other hand, the behaviour you describe matches the current drupal url filter. (And yes it is wrong and that is exactly the reason I'm trying to fix it.) So could you please check if:

* you really are running filter.module with the patch applied
* you are not seeing a cached version of your content. For instance, write foo123 at the end of the text to make sure it is different from some old version.

I found one instance where actually url's are modified when they shouldn't:

<code>In this example www.test.com the first url is not modified, but <em>the second www.test.com is</em> even if it shouldn't. </ code>

The problem is that v0.5 only pays attention to the innermost tags. I haven't thought of a case in normal html where this would lead to problems, but indeed the code tag is different.

I'll work on this and see how to solve it. Thanks shap for bringin this to my attention - I don't know if this is exactly the bug you were describing, but thanks still.

Well, to fix the problem with tags nested inside code tags, I had to revert to the preg_split/implode strategy from 0.4. I've completed and refined that approach and this patch is a refined version of 0.4.

I would appreciate if you can test this out too. I'm pretty sure this is close to perfect, I dare you to come up with examples to break this :-)

Attached is the testdata I'm currently using.

Since 6.0 has now reached beta (congratulations, btw) I'm changing this to target 7.x-dev.

Great! I certainly agree with classifying this as a bug, if there are more than me who will agree.

I was trying to be cautious, since I'm under the impression that the URL filter is "good enough" for many people and some may not agree that what has now become a complete rewrite would be considered for 6.x. But as this thread too shows, there are planty of things the old URL filter does wrong, and this patch should fix all of them.

Sorry about that, I didn't realise there were any characters where that would have mattered. Here is the same patch but now in UTF-8 format.

Forgot to change status again...

...

As someone who doesn't follow the dev mailing list, I'll ask here: Is HEAD already taking in new patches or is activity still focused on D6?

Hi jpsalter

You've found the right place, this is THE thread and it is still active.

6.0 is now out and this wasn't applied. I'm actually wondering myself what the correct Version tag for this would be? 6.0 (since this bug exists in 6.0) or 6.x (where it should be applied) or 6.1 or 7.x?

I doubt it will be applied to 5.x, but you should be pretty safe applying it yourself. From what I can tell the _filter_url() functionality has been untouched through the whole 5.x series to 6.x. Even if the patch wouldn't apply you could still just replace the relevant functions with copy pasting.

Thanks Keith

Old habits die hard... I already had the periods in place, but rewriting parts of the code it seems I introduced periodless comments again.

This patch 0.8
- fixes style issues with comments (periods)
- diff against 7.x-dev (these functions untouched but file changed so line numbers changed from 6.x-dev)

Could someone please see if there are any more punctuation or style issues with the comments and help get this committed to 7.x.

I first submitted this patch during my last years Summer vacation, and now the snow is melting and the grass is green again, I would be disappointed not to have such a straightforward fix committed before my next Summer holiday due in 3 months...

Thanks Keith

Would you find in yourself enough energy to see if there is still something that needs to be corrected. I'm afraid next you'll tell me that www should also be uppercase? Speaking of which, is there an official style guide where I could get all this advice in one shot?

v0.9
* style: uppercase HTML and URL

Thanks. Looking at it now I realise I already read through it last summer. But it isn't as detailed as some of the advise I got here, which is more like grammatical stuff almost. *Which I'm completely ok with*, btw, it just seems there is an additional "oral tradition" to style issues at Drupal, so it seems I will have to continue relying on the grace of core developers giving some feedback every once in a while.

Ah, Thanks Kev. Had not seen that one.

In any case, we are digressing now a bit from the main question? What should yet be done so that this can be committed to 7.x?

Attached is my most developed "test case". The patch works good with all of these. The current url-filter fails at least:
* urls inside a dl list (not processed, should be)
* urls in a title attribute in a link (are processed, shouldn't)
* urls in html comments (are processed, shouldn't)
* urls inside some other html tags also are not processed but should
(This is out of my memory, I haven't used the official urlfilter in a while :-)

ping

Thanks, flobruit. I'm immediately going to take the benefit of not being the submitter of the last patch anymore and put a reviewer hat on. Let's see if we can finally get this committed!

/me reviewing... Looks good!

We've had several people looking at this without complaints for months now, and now we also have the unit tests committed in filter.test that show the improvement over the existing url filter. I vote for committing the patch by flobruit in #46.

Wow, this looks like a real review! I have to admit I had given up hope...

Sun,

Please remove line-break here:

+    // Note: PHP ensures the array consists of alternating delimiters and literals
+    // and begins and ends with a literal (inserting NULL as required).
+
+    // If an ignoretag is found, it is stored here and removed when the
+    // closing tag is found. Until then no replacements are made.
+    // Think of this as a stack that always has 0 or 1 items.

Please don't! Those are not the same comment. The 2 first lines refer to the previous line, while the three last ones to the following line. (And since next you are going to say that comments cannot appear after the statement they are a comment on, I might as well add right now that this code is a copy paste from existing code in Drupal core and is like that in 4 other places too.)

Please read coding standards again:

This is a rather unhelpful comment (I'm being polite here) and no, I won't read them again. If there is a style issue, say what it is.

We do not use such "reminders" in Drupal: (if that's needed, the function body is likely too large)

Sure, they can be removed. (This must be the first time anyone complained of there being too many comments in a patch I wrote :-)

...probably more, but please fix those first.

Sorry, this won't work. We've had several people now flying in and "helpfully" pointing out punctuation, capitalisation and whatnot issues. I'm not interested in that game anymore. This patch fixes a known bug in Drupal which several people have complained about. Since September 2007 (that's 9 months, I have made a baby in less!) there hasn't been any code updates, just grammar/style discussion. If we get a list of the style/comment issues that should be fixed we will fix them. But at least for my part I'm not gonna participate in a never ending game of "fix these first" reviews. This is a short patch, it is not too hard for you to give a finite list of problems and we'll fix them and then the patch can be committed.

Thanks.

(FWIW, your comments I didn't comment on are perfectly good comments and I'll be happy to improve on those together with the complete list of remarks that I hope to get soon. I also want to point out that it's obviously not your fault that we've already had too many unhelpful helpful reviewers pass by without any progress, you just happened unluckily to be the last one - in fact your review was the best so far.)

Sun, I have nothing against peer review. We have been waiting for soon a year now for it to happen, and for nine months this patch has been *nothing but* waiting for peer review to happen. So I'm not objecting to the issues you pointed out, I'm objecting to you saying that there are more that you didn't point out! Please do so.

I'm moving this back to Code Needs Review because that's what we really need here. When we know that we have a final list of issues to correct, I'll correct them.

(Thanks for clarification on that one point.)

jpsalter, catch: Thanks.

@catch: Sure, and I understand this is how projects like Drupal works, and I'm not in a hurry, for myself I can apply the patch anyway and if no-one else cares then it is ok to focus on higher priority issues.

What is unproductive imho, is that there are people passing by, pretending to do a review when it's just really very very superficial. So for instance in this last instance, 1) the reviewer himself says that his review is incomplete and he may come back for more comments later and 2) he later also says "I didn't even look at the code yet" (which I didn't want to unnecessarily point out, but was also apparent from the first comments, sorry to say). From my experience if those are fixed, then for 2 months it will be quiet again, then someone else will pop by saying "Here are a few more comments but this is not all..." And at no point in these reviews are we really moving closer to actually committing anything.

So by all means, feel free to leave even more such comments, but there is no need to change the Status to "code needs work" when you know yourself you didn't really do a proper review. What this patch needs is more review, and when we've had a proper review it's just 1 minutes work to incorporate all the issues also from the partial reviews, such as those made by Sun.

So what I'm waiting for now is for someone to say: "I have really looked at the code *and* comments and here are *all the issues as far as I can see*." Even better if we get someone to say: "After fixing these I can (or I will propose) commit this patch."

Guys, I'm sorry to say, but this "cleanup work" has broken quite many things. It was nice to see so many genuinly helpful inputs and I'm sorry I wasn't able to follow up instantly on changes you made. However, looking at this patch now, there are several things that have been broken, you ahve each contributed some of it. To simplify this, could we just go back to #62 which is the last working patch as far as I can see.

Things broken in the new patch:

* A crucial } else { has inexplicably disappeared, this is a major bug of course. (missing at line 77 in patch).

* Removing $length parameter from _filter_url_trim() has broken that function ($format is now undefined). Probably buggy at least for long URL's?

Further, I strongly disagree with moving the fetching of the length parameter into the function in the first place. _filter_url_trim() is a generic utility function which should just take an input and return an output, it shouldn't have any drupal stuff inside of it. Utility functions should be generic, fetching a drupal variable from db belongs to the main code.

* I don't agree that moving the patterns into an array and away from their actual regexps calls help readability at all. If you want to be clever, go all the way and make the switch-case statement go away completely (email task needs to be modified to use callback too), that would make code shorter and at least arguably easier to read.

* Minor: in the callback functions I find the new comment "+ // Find the parenthesis in the regexp containing the URL." misleading, since this is not about finding anything. My original comment was just supposed to be a description of what variable $i is, not any actual action.

***

So, back to main topic, attached is the patch from #62 which we need to get back to. This contains the whitespace fixes but none of the restructuring of code which all introduces bugs. (It contains the new comment in the callback functions I don't like as listed above in the last minor point, so that could be reverted in the next edit.)

I'm switching back to code needs review and we can continue discussion from where it was at #62. (ie, shouldn't be any bugs, you may have style opinions you want to re-introduce...)

I'm thankful for more help on this patch, but it is not tolerable to introduce real bugs while discussing style issues. Please be more careful and remember that you are editing a working patch.

@chx: I tested your patch, with the particular input I had in mind. You're almost right, cause you didn't break what I thought. However, you are to blame for the 3 new notices we get:

# notice: Undefined offset: 13 in /var/www/drupal7/modules/filter/filter.module on line 771.
# notice: Undefined offset: 17 in /var/www/drupal7/modules/filter/filter.module on line 771.
# notice: Undefined offset: 21 in /var/www/drupal7/modules/filter/filter.module on line 771.

(The fourth notice is because of the _filter_url_trim() change introducing a bug.)

This is because the $chunks array always ends with a text chunk, but in your patch the code always ends with trying to process a tag, so these notices are the PHP equivalent of ArrayIndexOutOfBounds complaints. It's not fatal (output actually is still ok) but it is obviously not acceptable either. In a more general description: Your trick doesn't work if there is an odd number of array elements, and in this case there often (always?) is.

I agree that the if($i % 2 == 0) thing may not be the nicest code either. What do you think about the following:
1) Keep my original code hierarcy
2) Replace my modulo stuff with a variable that takes text values "text" and "tag" instead.
I mean:

// $chunks now contains a series of text and tags (literals and delimiters)
// always beginning with text.
$chunk_type = "text";

....

for ($i = 0; $i < count($chunks); $i++) {
  if($chunk_type == "text") {

  ...

   // Done processing text chunk, so next chunk is a tag.
   $chunk_type == "tag";
  }
  else {

  ...

   // Done processing tag chunk, so next chunk is text.
   $chunk_type == "text";  
  }
}

What do you say? Btw, this introduces an "unnecessary" variable just for readability, is that considered good or bad in the Drupal community?

***

Going forward: I need to go to sleep now and will be on a business trip the whole next week. If nothing happens during that time, I will update the patch with the below changes. If someone is still bold enough, feel free to do these updates and continue the debate, then I'll come back a week from now and tell you what I think again.

FWIW, I've been a little cranky here earlier, I am in fact happy now that the last contributions have been addressing code and other substance, not just whitespace and grammar. (Note to self: check whitespace when done, I'd hate to be caught a fourth time for it!)

TODO, wrt patch in #68:

1) Agree with chx (and others) how to replace if($i % 2 ... and replace it

2) Make the switch statement go away completely. Email regexp processing needs to be made into using a callback function too, and then use the name of each callback function as $task (See $patterns array in#65). (This is another "clever" thing I'm still lukewarm about, but it will make code significantly shorter and less indented, so if we do it with good comments I hope it will increase readability. I really never liked that switch, have to admit.)

3) Do NOT make any changes to _filter_url_trim().

4) Revert the changed comment inside the callback functions so that it is a comment introducing variable $i. If there is need to comment further on what the function is doing, do that too.

5) Double check, but I believe all issues in #50 have been addressed. (Either included or rejected.)

No patch attached, I'll be back in a week (plus perhaps a few days).

Attached patch implements changes as discussed in comment #70. There are no outstanding issues so patch is ready to be reviewed and committed.

Just a short note to everyone following this thread. I have today committed the above version of URL filter into the Footnotes module, of which I'm the maintainer. You can now easily install and use a working version of URL filter by

1. Download Footnotes 6.x-2.x-dev snapshot (it is safe, there is no wild development going on.
2. Activate the Footnotes module
3. Replace "URL filter" with "Better URL filter" everywhere
4. Note: You can take advantage of the Better URL filter whether you use Footnotes or not.
5. Note: Even though this is a Drupal 6.x module, it can be installed into earlier versions too.

Please see #281143: Better URL filter now available within Footnotes for more info. Please comment on Better URL filter in that thread and not here!

I hope this will resolve the standstill we have here and users who need a working URL filter can finally have it without letting this discussion hold them hostage for another year or two.

@cwgordon

I originally removed the +3 since I don't understand the logic of it. If anything, it should be -3.

Now, if user has set max length for URL's to be 72 (for instance), then we should cut all URL's that are longer than 72, not longer than 75. Even so, this is not a major issue for me, so I'm letting your patch stand. I'm just saying I don't understand it and I don't see it doing what the comment says it is doing.

Indeed, patch #80 doesn't seem to remove much lines at all, same problem also with the other *_parse_*_links() functions for starters. Very unhelpful.

As I'm the original proponent of this patch, I guess it is fair to announce publicly that what happened in #260484: Regression: Filter tests fail made me loose whatever little faith I had left in this project and I don't intend to lift another finger working on this patch anymore.

(The good news is I will release version 2.0 of Footnotes during the weekend and users can just enable a working URL filter (this patch) from there instead, as it will be included.)

HTML comments are properly skipped. Not because of that line, but because of line 70 in the most recent patch.

+    // Split at all tags.
+    // This ensures that nothing that is a tagname or attribute will be processed.
+    $chunks = preg_split('/(<.+?>)/i', $text, -1, PREG_SPLIT_DELIM_CAPTURE);

It could be added to the preceding code comment of course, if wanting to be pedantic, but it does also exclude html comments.

@sun

Note that this patch will fix almost half of the other URL filter bugs since most of them are just various symptoms of what this patch fixes. So it certainly makes sense to take this first, then see how many other bugs you can immediately close. #190466, #168227, #191744, #728380, #555280... from the first page of the list should all go away. (This is also the reason this was marked critical, the old URL filter fails in quite many ways and this addresses the root problem.)

For tests passing, it is because tests were removed, not because bugs were fixed. Add back the tests from comment #91 and you'll see a different picture. (How critical the issues are is of course another thing, but as you can see, there are quite many bugs filed against URL filter that are various permutations of this one.)

PRE tag: Personally, I don't see a reason why URL's inside a PRE tag couldn't be made clickable, but don't have a strong opinion either way. I agree that a strict interpretation of "preformatted" means there should be no other formatting inside the tag.

Only thing to still change:

Please use the tests from #91. The current "one liner" tests will not actually catch many of the bugs this patch addresses. It is important to test on data that contains multiple strings to make clickable, and multiple rows, and multiple paragraphs. If HEREDOC syntax is frowned upon, please separate the big test data chunk into a separate file and use it from there.

On a quick reading the last committed patch is good and would pass all tests from #91, so committing it is ok. But since the tests were written, I wouldn't mind adding them to the test harness too. This seems to be an area where some contributors may easily break things.

PS: I'm glad to see this patch is finally getting some serious attention. I'm happy to help if there are any questions. Unfortunately I have been affected by the European flight chaos, and will now embark on a 5 day trip back home, but will be back online mid next week.

(This is based on a quick read of HTML Purifier code, not actually installing it.)

HTML Purify mainly does the same job as HTML filter (making HTML safe). But indeed, it contains also the "Linkify" class as some kind of addon service.

In general HTML Purify seems to be a rather complex beast comparing with Drupal's very simple core modules for doing the same thing. Also it is (probably?) based on actually validating the HTML with an HTML parser (correct, but slower), whereas Drupal filters tend to just use some regexps to get their job done (good enough if done right, faster).

As for the URL filter vs Linkify comparison, it is surprising to note that my patch here addresses more URI types and the Linkify class in HTML Purifier turns out to be rather simple. In fact also the original URL filter in Drupal core does more than the one in HTML Purify. This can probably be explained by the fact that this is not the main task of HTML Purify, so the implementation has been left as rather simplistic.

HTML Purify will only convert into links strings beginning with
http://|https://|ftp://

This patch is more complete and will catch all of:
http://|https://|ftp://|mailto:|smb://|afp://|file://|gopher://|news://|ssl://|sslv2://|sslv3://|tls://|tcp://|udp://

Also the original Drupal URL filter was more versatile than the one in HTML purify.

In addition both this patch and the old URL filter also convert email addresses (...@...com) and "plain" URL's without the "http://" (www.example.com or ftp.example.com).

In summary, Drupal's URL filter does more than the one in HTML Purify, and is (very probably) simpler and faster.

(And to those just tuning in: there is no difference with this patch vs the forked URL filter, they are the same. The difference between this patch and the old Drupal URL filter is that the old filter is rather buggy, it will miss many URL's that should be converted (especially if other HTML is involved) and it will mangle some places it shouldn't touch.)

I didn't notice earlier that this was downgraded from critical in #110. As I explain in comment #112, the statement that "tests pass" is not correct and the argument that there are many other bugs in the queue is questionable too, since this patch will fix about half of those bugs! I apologize for not setting back to critical at that point, I was stuck in an airport somewhere with bad connectivity.

If there is some other reason not to regard this as critical, feel free to revisit, I'm just saying the earlier downgrade seems to have been done based on wrong assumptions.

Hi sun, and thanks for the additional review. (Would've forgotten about the title attribute otherwise, is *that* checked by a test? :-)

So just for the public record, we did a thorough review and out of 9 open bugs, following are fixed by this patch:

#168227: URL filter fails on URLs surrounded by HTML entities
#877050: URL filter does not convert a link inside a div
#555280: Filter "Convert URLs into links" converts URLS within inline css background-image property

I can work on this during next week.

A few questions to sun:

1)
It would be easy to also fix
#190466: URL filter fails on square brackets
#728380: URL filter fails on parenthesis (RFC2396 Unreserved Characters; i.e., ( and ))
...should I alter the regexps here to cover entire unicode alphabet, or do you want to do that in a separate issue?

2)
When those other bugs have test cased posted, but they are not in D7 CVS yet, should I copy them into this patch?

I'm willing to do both if you answer yes.

Hi sun

Good work pushing this forward and getting the patch to apply again. Here's a review. I have errands today, but can work on these myself too if nobody else does it before me.

+  // Prepare protocols pattern for absolute URLs.
+  // check_url() will replace any bad protocols with HTTP, so we need to support
+  // the identical list. While '//' is technically optional for MAILTO only,
+  // we cannot cleanly differ between protocols here without hard-coding MAILTO,
+  // so '//' is optional for all protocols.
+  // @see filter_xss_bad_protocol()
+  $protocols = variable_get('filter_allowed_protocols', array('http', 'https', 'ftp', 'news', 'nntp', 'telnet', 'mailto', 'irc', 'ssh', 'sftp', 'webcal', 'rtsp'));
+  $protocols = implode(':(?://)?|', $protocols) . ':(?://)?';
+

As you note in #257193: URL filter: Use filter_allowed_protocols variable, / and : are valid characters anywhere in the URL. So we could simply omit the addition of (?://)? and allow those characters liberally in the next part of the URL. The current regexp doesn't however allow them. So we would need to back to the plan of allowing most unicode characters anywhere in a URL. It can either be done right now, or we let the above stay as it is and then remember to remove it once we change the other regexp so that the above (?://)? isn't needed anymore.

(For clarity, the above needn't necessarily be fixed in this path.)

+  $domain = '(?:[A-Za-z0-9._+-]+\.)?[A-Za-z]{2,64}\b';
+  $ip = '(?:[0-9]{1,3}\.){3}[0-9]{1,3}';
...
+  $url_pattern = "[A-Za-z0-9._-]+@(?:$domain)";

Good call to add $ip (whoever did it).

Same comment here: All of this will be much simpler if we in the future change this regexp to something that just matches a string of unicode characters until next whitespace. (Nothing to change here.)

From #120 above:

Also, I wonder why *? instead of just ? ? ... Or am I missing something? If I do, then the code is missing an inline comment.

The *? is supposed to catch multiple trailing punctuation such as www.example.com... or (blablabla www.example.com.) or example.com!!! Note that it may not always repeat the same character, eg .) must match too.

+  $pattern = "`($url_pattern)`i";
+  $tasks['_filter_url_parse_email_links'] = $pattern;

It seems the email task doesn't contain that regexp at all so me@example.com. wouldn't be matched correctly. (This omission exists already in my patches and original code.)

+    // Split at all tags; ensures that no tags or attributes are processed.
+    $chunks = preg_split('/(<.+?>)/i', $text, -1, PREG_SPLIT_DELIM_CAPTURE);

The above code correctly ignores also multi-line comments. A separate test for this could be added though, or rather just add \n to the existing test.

That is all for the functional code. I have a meeting now, will also review test code separately (look forward to what you've done :-). Hopefully tonight.

Heh, funny to watch how drupal.org itself behaves when I give examples of strings that should or shouldn't be converted.

In this case, this comment:

It seems the email task doesn't contain that regexp at all so me@example.com. wouldn't be matched correctly. (This omission exists already in my patches and original code.)

...is wrong, it works correctly for other reasons. Please ignore.

I'm not at my home computer at the moment, so cannot test, but since you are so active on this, a few quickie comments from my head. (...which may be wrong).

Problem: URLs in multi-line comments are not ignored. @hingo: Can you fix that?

I can take a look later, but more importantly I actually tested a multi-line comment before I wrote my previous review and it worked correctly. I only had one newline, is your problem with more than that?

Adding yet another test with html markup in comments is a very good idea. (That may actually break, I realize now! The URL should be after the html...)

The *? is supposed to catch multiple trailing punctuation such as www.example.com... or (blablabla www.example.com.) or example.com!!!

That's what I figured, too. However, we are not interested in further trailing text; just want to optionally exclude a certain (single) trailing punctuation character from the URL, if there happens to be one.

Yes, but now if you have such multicharacter punctuation, it stops on the last punctuation character, not the first? Ie it produces <a href="http://www.example.com..">www.example.com..</a>.

I agree that e-mails should get that punctuation treatment, too, and we should move the pattern into a new common $punctuation variable.

Turns out emails never needed them, since they always end in a letter anyway. (like the m in .com)

Your comment about domain names is correct. But I was thinking there could be other protocols than http which don't have a domain name at the start. Even in mailto, I could see someone having "//@example.com" as their email address (yeah, probably illegal, need to check)

Looked at the failed test now. I think a multiline comment works, it's your p tag that breaks it. The problem is the > character of the p tag matches here:

$chunks = preg_split('/(<.+?>)/i', $text, -1, PREG_SPLIT_DELIM_CAPTURE);

My first reaction is that we need to handle html comments as a special case. Probably the cleanest solution is to add a step that escapes all html inside comments, like some filters do in the $op="prepare" phase.

Here is a review of the tests, finally. As before, if nobody else is faster, I will also implement these myself asap.

Since I haven't really looked at these tests before, I'm reviewing the filter.test file itself, not just the most recent patch.

Line 996

    // Filter selection/pattern matching.
    $tests = array(
      // HTTP URLs.
      '
http://example.com or www.example.com
' => array(
        '<a href="http://example.com">http://example.com</a>' => TRUE,
        '<a href="http://example.com">example.com</a>' => FALSE,
        '<a href="http://www.example.com">www.example.com</a>' => TRUE,
        '<a href="http://www.example.com">http://www.example.com</a>' => FALSE,
      ),

There is no reason to test both positively and negatively the same test. If the first test passes, then the second test automatically passes. Same for 3 and 4.

Also it is in general not a very useful approach to test some specific ways things could go wrong, since there are always a million ways things go wrong. We should just assert that the result is correct and that's enough. If it is known to be correct, we also know it is not wrong.

In other words this is sufficient:

    // Filter selection/pattern matching.
    $tests = array(
      // HTTP URLs.
      '
http://example.com or www.example.com
' => array(
        '<a href="http://example.com">http://example.com</a>' => TRUE,
        '<a href="http://www.example.com">www.example.com</a>' => TRUE,
      ),

This comment applies to many of the following tests too.

Line 1050:

      // Absolute URL protocols.
      '
https://example.com,
ftp://ftp.example.com,
news://example.net,
telnet://example,
irc://example.host,
ssh://odd.geek,
sftp://secure.host?,
webcal://calendar,
rtsp://127.0.0.1,
not foo://disallowed.com.

Maybe add cross referencing comments here and line 1325 in filter.module so that one remembers to update the test if the list of supported protocls changes.

Good that you also test for a not supported protocol.

Line 1078:

    // Surrounding text/punctuation.
    $tests = array(
      '
Partial URL with trailing period www.partial.com.
E-mail with trailing period person@example.com.
Absolute URL with trailing period http://www.absolute.com.
Query string with trailing period www.query.com/index.php?a=.
'

Need to add back examples with multiple trailing punctuation characters from comment #128.

Line 1194:

      '
<!--
Skip any URLs like www.comment.com in comments.
<p>Also ignore http://commented.out/markup.</p>
-->

Learning from your previous comment, this test is ambiguous since when it fails you don't know if it fails on the HTML tag or because of the newlines. My favorite solution would be to also add newline to the previous test. Alternatively remove newline here and have a separate third test for it.

Ok, so now we have good test coverage.

From the email discussion we had, the approach you showed in building tests suggests it would be possible to easily test all HTML tags by iterating over an array that contains them. (You seem to have further developed that approach here so now it's slightly different.) Now we just test for DL list, P and some other random picks. But for me it is perhaps better to postpone that exercise to a separate issue and close this with the tests we have.

Even if I've already objected to it many times, you have now removed the one test I actually provided. The current tests do cover the various issues well in isolation, however it is also important to test on a larger document that combines many of the cases into one document. With the work you have done, it needn't be as complex as the one I did earlier, but I want to add back one test which would start with the following comment:

// It is important to also test on a more complex document than just short one 
// liners testing one function in isolation.  Note that common regexp errors 
// include: accidental (greedy)* match instead of (minimal)*? match, only 
// match first occurrence rather than all, and newlines not matching .*.
// Additionally early versions of URL filter had bugs in cases where you have
// combinations of the 3 types of URLs to convert.
// This test covers:
//  * Document with multiple newlines and paragraphs (two newlines).
//  * Mix of several HTML tags, invalid non-HTML tags, tags to ignore and
//    HTML comments.
//  * Empty HTML tags (br, img)
//  * Mix of http://... email@... and www... strings in same document.

PS:

But I was thinking there could be other protocols than http which don't have a domain name at the start.

Let's not overcomplicate things here. ;)

That's why I'm interested in the unicode support: the reason that is a tempting path to explore is that the regexps would be much simpler, like ($protocol:\S)$punctuation*?\s

Ok, here's a patch that
- reverts email handling to not use the punctuation stuff (I tried to say so in #129)
- includes fixes for the tests from my previous comment, including the test with large document
- adds back support for multiple trailing punctuation (and tests)
- correctly handles commented out HTML
- since I needed to touch the code anyway, finally fixes _filter_url_trim(). See http://drupal.org/node/260484#comment-966659

The code added to handle HTML comments definitively needs review.

I didn't use anything from #222926: HTML Corrector filter escapes HTML comments since it seems they don't have a working patch themselves. Maybe they can borrow my solution :-)

(Hmm... I cannot add new files with a diff, so I'm attaching a tar file separately that contains a new test/ directory.)

Note that automated test will obviously fail now that 2 new files are missing from the patch. Tests naturally pass on my own machine.

Line numbers refer to the latest patch, not the source file:

Line 230:

@@ -1350,7 +1506,7 @@ function _filter_url_trim($text, $length
   }
 
   // Use +3 for '...' string length.
-  if (strlen($text) > $_length + 3) {
+  if ($_length && strlen($text) > $_length + 3) {
     $text = substr($text, 0, $_length) . '...';
   }

Line 552:

+    $filter->settings['filter_url_length'] = 20;
+    $tests = array(
+      'www.trimmed.com/d/ff.ext?a=1&b=2#a1' => array(
+        '<a href="http://www.trimmed.com/d/ff.ext?a=1&amp;b=2#a1">www.trimmed.com/d/ff...</a>' => TRUE,
+      ),
+    );

Do you want to elaborate on why you have switched back to the broken version of _filter_url_trim()? In #141 the test was correct and the code was wrong. Now you have changed the code to match the test and they are both wrong.

In particular, you set filter_url_length to 20 characters, but now it cuts after 23 characters. (For instance, try with a URL with 22 characters and you see what I mean.) This is a minor issue, I'm just curious why you took the extra effort to remove my correct code?

**

FWIW, I'm ok with changes to _filter_url_escape_comments(). Using boolean variable doesn't semantically contain information about everything that the argument does, but I guess it saves so many bytes it is ok.

736:

Index: modules/simpletest/drupal_web_test_case.php
===================================================================
RCS file: /cvs/drupal/drupal/modules/simpletest/drupal_web_test_case.php,v
retrieving revision 1.226
diff -u -p -r1.226 drupal_web_test_case.php
--- modules/simpletest/drupal_web_test_case.php	22 Aug 2010 15:31:18 -0000	1.226
+++ modules/simpletest/drupal_web_test_case.php	22 Aug 2010 20:23:44 -0000

This is probably here by mistake? Also, it just moves a function a little bit, doesn't seem to do anything.

1) Ok. It is a different issue and can be handled separately. I mentioned it in #139. I'm ok with taking it away, just wanted to raise a flag since you reverted it without commenting.

2) If it was intentional, I have no comment, I'll focus on filter.module only.

+1 for #144 then.