Posted by csc4 on July 3, 2007 at 11:36pm
Jump to:
| Project: | Links Package |
| Version: | 6.x-2.x-dev |
| Component: | Code: API (links.inc) |
| Category: | bug report |
| Priority: | critical |
| Assigned: | syscrusher |
| Status: | needs review |
Issue Summary
Where I have images embedded in the node with the image being a link I'm getting some very odd links:
Links from Article Text
- <img src="http://www.example.com/images/intro_img1sml.jpg" border="0" height="102" width="100">
- <img src="http://www.example.com/images/intro_img2sml.jpg" border="0" height="100" width="100">
- <img src="http://www.example.com/images/intro_img3sml.jpg" border="0" height="100" width="100">
- <img src="http://www.example.com/images/intro_img4sml.jpg" border="0" height="102" width="101">
- <img src="http://www.example.com/images/intro_img5sml.jpg" border="0" height="100" width="100">
Shouldn't it either use the alt text if there is one or else skip the link? or put the image in??
Comments
#1
Bump - I still seem to have this problem? am I the only one?
Does anyone have a workaround?
#2
This remains an issue even within 5.x-1.12.
#3
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";#4
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
#5
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
#6
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
#7
Add one more:
2a. Look for a "title" attribute on the link tag itself.
Scott
#8
I haven't encountered any bugs from this change. What exactly have you encountered?
#9
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
#10
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
#11
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...
#12
@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
#13
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
#14
Thank you all for all your efforts.
The new branch still doesn't seem to be on the project page but I found it at http://drupal.org/node/24719/release
Sadly, it doesn't seem to have solved my problem.
For
<a class="lightbox-processed" rel="lightbox[triggerlightbox][Bilbo adds his support to K9 Crusaders fundraising]" href="/sites/default/files/shared/events/2008ChristmasFayre/2008-12-01-013%7E1_0.jpg"><img src="/sites/default/files/shared/events/2008ChristmasFayre/2008-12-01-013%7E1.jpg" alt="Bilbo adds his support to K9 Crusaders fundraising" title="Bilbo adds his support to K9 Crusaders fundraising" style="vertical-align: middle;" class="triggerlightbox" height="400" width="386"></a>I still have
Links from Article Text
* <img src="/sites/default/files/shared/events/2008ChristmasFayre/2008-12-01-013%7E1.jpg" alt="Bilbo adds his support to K9 Crusaders fundraising" title="Bilbo adds his support to K9 Crusaders fundraising" width="386" height="400" style="vertical-align: middle;" class="triggerlightbox" />