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&#039;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.

CommentFileSizeAuthor
#11 857402-aggregator-block-test-d7.patch2.95 KBandypost
#10 857402-aggregator-tests-d7.patch2.94 KBandypost
#6 857402-2.patch5.24 KBcafuego
#5 aggregator_block_link_fix_and_test.20100719.patch5.59 KBdman
#3 aggregator_more_links.20100718.patch1.95 KBdman
#3 aggregator_block_display_test.20100719.patch3.11 KBdman
#1 aggregator_block_display_test.20100718.patch2.93 KBdman
aggregator_more_links.20100718.patch1.95 KBdman
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dman’s picture

dman CreditAttribution: dman commented
FileSize
aggregator_block_display_test.20100718.patch2.93 KB

Heres 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.

    # assertLinkByHref() won't work if we are looking for the bad link 
    # "//aggregator/sources/n" as seen in http://drupal.org/node/857402
    # $this->assertLinkByHref($href);

Test here now checks the conditions:

  • adding a new feed makes a block available
  • block can be positioned using block admin form
  • when positioned, the block shows up on the page
  • the block shows the correct "More" link
  • visiting that link takes you to the right place.

Status: Needs review » Needs work

The last submitted patch, aggregator_block_display_test.20100718.patch, failed testing.

dman’s picture

dman CreditAttribution: dman commented
Status: Needs work » Needs review
FileSize
aggregator_block_display_test.20100719.patch3.11 KB
aggregator_more_links.20100718.patch1.95 KB

"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?

Status: Needs review » Needs work

The last submitted patch, aggregator_block_display_test.20100719.patch, failed testing.

dman’s picture

dman CreditAttribution: dman commented
Status: Needs work » Needs review
FileSize
aggregator_block_link_fix_and_test.20100719.patch5.59 KB

... I guess not. That one failure just sits there advertising the error.
How about a combined patch then?

cafuego’s picture

cafuego CreditAttribution: cafuego commented
FileSize
857402-2.patch5.24 KB

Yeah, 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:

  • the commented out original call to theme('more_link') for aggregator sources and
  • $block['content'] = 'no items?';

I've removed these two lines and manually applied the aggregator.test patch. Updated patch attached.

dman’s picture

dman CreditAttribution: dman commented

Yeah 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!

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented

Could we incorporate this change with #866220-9: URL parameter for theme_more_link() should not pass url()

rfay’s picture

rfay
Primary language English
Location Palisade, CO, USA
CreditAttribution: rfay commented

I confirm that #6 fixes the described problem.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented
Status: Needs review » Reviewed & tested by the community
FileSize
857402-aggregator-tests-d7.patch2.94 KB

Issue 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

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented
FileSize
857402-aggregator-block-test-d7.patch2.95 KB

Fixed missed dots at the end of code comments.

Dries’s picture

Dries CreditAttribution: Dries commented
Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks.

dman’s picture

dman CreditAttribution: dman commented

Boo :-(
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

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.