img/image links in text being rendered very oddly

This remains an issue even within 5.x-1.12.

Did a little exploring, found the links_find_links function on line 458. Taking a look at the $patterns['tag'] regular expression (line 467) made me see that the regular expression was not parsing out out HTML tags within the anchor tag. Changing it to exclude tags within the tag seems to have fixed the issue: [^<]

The original line:

  $patterns['tag'] = "%<a href=(?:\"([^\">]+)\"[^>]*|([^\" >]+?)[^>]*)>(.*)</a>%Uis";

The new line:

  $patterns['tag'] = "%<a href=(?:\"([^\">]+)\"[^>]*|([^\" >]+?)[^>]*)>([^<]*)</a>%Uis";

Close, but not quite. That link fixes *this* bug, but it creates another that I found with my regression testing. I think you're right about the source of the problem, but I still need to tweak your patch before committing. I am busy tonight but will try to get this resolved in the next couple of days.

Scott

BTW, I didn't mean to sound critical of your patch. It is much appreciated, because it does solve part of the problem. It's just not quite 100% of the fix. :-)

Scott

Request for comment:

I thought about this a bit more, and I think the correct behavior should this:

1. Strip out all HTML tags from the "title" that the existing regexp finds. Whatever text left is the title, if not empty.
2. If that yields no viable title, then look for an "alt" attribute within any HTML tag in the original title string, and use it.
3. If that still yields an empty string, proceed as if there is a title-less link, which the Links module already handles gracefully.

Comments?

Scott

Add one more:

2a. Look for a "title" attribute on the link tag itself.

Scott

Close, but not quite. That link fixes *this* bug, but it creates another that I found with my regression testing.

I haven't encountered any bugs from this change. What exactly have you encountered?

In my test harness, the patch seems to break the ability to find links like these:

<a href="http://test.example.com/page.php"><img src="/some/image.png" alt="image alt attribute one" />title text one</a>
<a href="http://test.example.com/page.php"><img src="/some/image.png" alt="image alt attribute one" /></a>
<a href="http://test.example.com/page.php"><img src="/some/image.png" alt="image alt attribute two">title text two</a>
<a href="http://test.example.com/page.php"><img src="/some/image.png" alt="image alt attribute two"></a>

All four of these are found in the existing release version, although as you point out the title behavior is, err..., suboptimal.

I'm working on an improved version that will find the alt attribute in image tags and use it, if there is no explicit title text.

Scott

I have just committed developer snapshots of Links for Drupal 5 and Drupal 6 that have my modified patch embedded. If you could please test these on your site and let me know how things are working for you now, I would appreciate it.

In the test cases I showed above (in the code block), the new regexp and additional logic find the title text outside the image tag but within the link tag, if present. Failing that, the new code falls back to the alt attribute from the image.

I will be fixing a couple of additional, unrelated bugs from other issues before releasing this patch to production.

@pianomansam: Thanks again for your patch. I *did* use it as the basis for the new code, and simply added the extra logic I found necessary to cover the additional test cases. Your patch wasn't rejected, just added to. :-)

Scott

Could you post a patch file for these changes, or post the code lines and changes? I tried running diff on the files and couldn't find any changes pertaining to this issue...

@pianomansam: Sorry about that. The developer snapshots don't update instantly; I believe it's twice per day on a cron job. Watch the modification dates in the download links and grab the dev release you need (both D5 and D6 versions have this change). You might just want to wait until Tuesday to be sure you get the latest.

Kind regards,

Scott

The Drupal 6 dev snapshot didn't update last night. I forgot to update the release settings so it would see the new 6.2 branch. It should update tonight.

Scott