To replicate:
- Enable aggregator
- Add a feed. (update it to load with items)
- Place that feeds block on the page.
- Click 'More' as shown in that block.
Result: http://aggregator/categories/1 : This webpage is not available..
What is happening is that when aggregator creates the $read_more text, it runs the path through url().
Then it runs the result through theme_more_link(), which also runs url() on the $url.
$read_more = theme('more_link', array('url' => url('aggregator/categories/' . $category->cid), 'title' => t("View this category's recent news.")));
This produces:
<div class="more-link"><a href="//aggregator/categories/1" title="View this category's recent news.">More</a></div>
Hence the error.
(I think url() should be more robust than that, but can't fix it from here)
Fix attached - remove the redundant url() call. I'm looking to see what I can do about making a test for it (though the fix is pretty obvious). I scanned all open & closed aggregator issues and couldn't find another issue like this one. I hope it's not a dupe.
Comment | File | Size | Author |
---|---|---|---|
#11 | 857402-aggregator-block-test-d7.patch | 2.95 KB | andypost |
#10 | 857402-aggregator-tests-d7.patch | 2.94 KB | andypost |
#6 | 857402-2.patch | 5.24 KB | cafuego |
#5 | aggregator_block_link_fix_and_test.20100719.patch | 5.59 KB | dman |
#3 | aggregator_more_links.20100718.patch | 1.95 KB | dman |
Comments
Comment #1
dman CreditAttribution: dman commentedHeres a test for it. 20x the size of the problem that needs fixing :-)
I needed to do a slightly manual check for that link value, as the test API wouldn't throw an error in this case for a bad link.
Test here now checks the conditions:
Comment #3
dman CreditAttribution: dman commented"failed testing".
um... Yeah, failure is good. It demonstrates the current existing problem. Thankyou testbot!
So if I put the two patches together, will you test them as one?
Comment #5
dman CreditAttribution: dman commented... I guess not. That one failure just sits there advertising the error.
How about a combined patch then?
Comment #6
cafuego CreditAttribution: cafuego commentedYeah, the problem is definitely passing output from url() into l(). Just Don't Do It™ ;-)
However, the above patch didn't cleanly apply to my fresh D7 CVS checkout. There were also 2 lines in aggregator.module that seemed out of place or at least not relevant to the issue at hand:
I've removed these two lines and manually applied the aggregator.test patch. Updated patch attached.
Comment #7
dman CreditAttribution: dman commentedYeah good catch. Looks like those two extra lines were debug I re-introduced at some point.
The earlier attempt was clean, By the time I had my fourth bash at testbot I was getting sloppy.Thanks for catching that!
Comment #8
andypostCould we incorporate this change with #866220-9: URL parameter for theme_more_link() should not pass url()
Comment #9
rfayI confirm that #6 fixes the described problem.
Comment #10
andypostIssue was fixed in #866220: URL parameter for theme_more_link() should not pass url()
This patch just adds tests, RTBC because it's already get reviews
Comment #11
andypostFixed missed dots at the end of code comments.
Comment #12
Dries CreditAttribution: Dries commentedCommitted to CVS HEAD. Thanks.
Comment #13
dman CreditAttribution: dman commentedBoo :-(
Andypost gets commit credits for my test suite by adding a full stop to a comment?
Not that I've been counting credits, nor expect dries to follow a full issues history, but jeez. :-b
- don't take this seriously, I'm only fake-peeved