RSS feed links throughout Drupal are broken (especially forum module)

we should probably add tests for this, huh?

So this is broken in most callers of drupal_add_feed() because of confusion over whether it takes a path or a URL. The docs say it takes a URL. So then most calls (not all - node was OK, and views had this right) need to do url() around it.

This patch fixes Forum's bug the same way as #0, but adds the url() in the other places where it's missing.

@bleen18, got ideas how to add tests for correctness on the feed URLs? Do a drupal_http_request() on them and parse the results to make sure it has related content?

This is a pretty serious brokenness, so I think it's correctly marked major.

/me almost deployed a D7 version of randyfay.com with a broken drupal planet taxo feed :-)

@David, there are already several callers who are already passing it through url() before calling, including node module.

If it's *not* going to be a URL that we're passing in, we'll have to do an API change... Or at least one that looks like that because it will certainly change the docs from "URL" to "path". And the problem with that is you might want a feed that is external (like a feedburner feed).

I would actually prefer to do it by changing the internals (and the defined API). But it seems a little late for that. The documentation clearly says that you're to pass a URL. It does *not* say that you should have to do an absolute URL.

I haven't gone down the path of looking carefully at how the icon is generated.

Sure do hate to be mucking with things. The least "official" mucking is to change drupal_add_feed's documentation to say it requires an absolute URL, and then fix the callers in core.

I guess I should go back and try your #0. I originally took a casual look and thought it only fixed forums. But if it fixes everything maybe it's fine (even though a minor API change).

#0 works for me; If we're going that way we'll have to clean up the callers so they don't call url() before calling. It will break views, so I assume it will break other callers. It is an API change.

Yeah, we can't do the API change at this point, IMO. This argument has been the way it is since Drupal 5, and this isn't a serious enough problem to warrant the breakage. But let's definitely do for Drupal 8.

Randy's patch reverts to the D6 code, and that's what we should do. Plus an amendment to documentation of the args in drupal_add_feed() here so it's clear what we expect there.

I can't quite fathom how this got broken, though. In Drupal 6 these calls are fine: http://api.drupal.org/api/function/aggregator_page_category/6 Weird.

Ironically, the change to theme_feed_icon() was done in #715142: [beta blocker blocker] Various URLs escaped twice, since check_url() resp. filter_xss_bad_protocol() calls check_plain()

The intent was to fix URLs being double-url'ed, apparently.

@webchick, we may be dealing here with an API change that was already done, but that we have to finish now.

I explained the contradiction here to webchick in IRC, and that we probably have to do with @David's option 1 in #9.

She sez "Documentation is good" and gave her hearty (distracted?) support to the original approach here. So let's fix this up to make clear what the API change is, fix the stuff around it, and see if we can make it fly.

IMO we still need to make sure that drupal_add_feed() can still add an external URL though. Hmm.

I think #877986: RSS link in Syndicate block has path of //rss.xml was an example of this in action.

OK, this patch fixes it in a way that I think resolves all the issues.

1. With the addition of a small test we can tell whether a URL was passed in or not. That resolves the API change problem: Now we can handle both URLs and paths. And this is important, because if we had gone with paths only we would not be able to use an external feed here (like a feedburner feed).

2. This patch adds a test for the various permutations of drupal_add_feed().

Removing the "Needs tests" and "API change" markers, as I don't think it needs either.

Issue summary:
The bugs:

• the feed icon and the HEAD feed link were relative to the current page if a path was passed to drupal_get_feed(); they were completely broken if a URL was passed. There wasn't a way to reconcile the behavior of theme_feed_icon() and drupal_get_feed() to fix this.
• There was a minor error in forum.module in doing the drupal_add_feed().

Diagnosis:
There was a (probably unintended) shift in the drupal_add_feed() and theme_feed_icon() functions to favor paths instead of URLs. However, this was not fully implemented, and both the feed icon and the HEAD feed link were broken (they were relative to the current page instead of being absolute).

The fix here is to allow both fully qualified URLs and paths to be passed into drupal_add_feed().

Fails with clean urls turned off :-(

The problem was that '?' is a meta-character the regular expression used to match, so had to be escaped. The problem was with the test when not using clean urls, not with the code.

This #fail was caused by #808560: Node comment statistics are only partially exposed in node_load() and are missing last_comment_uid and doesn't have to do with this issue. Retesting.

#17 is ready for review - The details, proposed resolution, and issue summary are in #14.

Whoops, noticed a debug() in there.
Details and issue summary in #14.

@David, thanks for taking the time to look at this.

The (only) problem with the original patch is that it allows *only* drupal paths to be passed in, or they get url() called twice on them. But that doesn't allow both path and URL to work.

In D6, this was supposed to be a URL. In D7, it was mostly converted to just be a path. Your patch made the path approach work completely and correctly, but the slight change in this patch allows both path and URL to work, meaning that the various callers won't be broken and that you can still do a drupal_add_feed() of an external URL (like a feedburner URL).

Why not use both relative and absolute links? Because it's easier to have one goal :-)

The whole purpose (I think) of the patch that introduced the problem we're trying to fix here is that url() was being called twice on paths in some cases. So that's why I was trying to finesse the choice of when to use it.

As far as the test: I actually tried to switch to assertPattern(), but had some problem I don't remember and switched back to just testing the HEAD. I can't remember why or think of any good reason, but I believe the test is correct and tests what it ought to test.

Sorry - hoped to get to this on the plane today and didn't. One of these is going in :-) They'll work. I'm not holding #24 back. Just wanted to give it a spin before RTBC'ing it.

OK, #24 works fine and let's go with it. I tried it out manually in addition to the tests provided. Tried it with clean URLs and without. Works.

Views still uses a relative url() before calling drupal_add_feed() so it breaks with that, so we'll have to follow up with a views3 patch when this goes in.

Let's go for it.

The issue summary in #14 still holds.

Just for the sake of not having to do this over, attaching the views patch that will need to be done. (or at least this is one way of doing it).

It's always better when one attaches a longer-than-zero-length file. Intended vews future patch for #29 is here.

Issue summary is still in #14. Committable patch is still in #24.

Remove the argument name change. $url is either an internal path or external URL, everywhere in Drupal. The actual fix is to pass $url through url().

Sounds like this still needs discussion.

Sorry - I can't agree that taking explicit documentation about a sensitive difference out improves this patch. One *must* pass in to drupal_add_feed *either* a path *or* a fully-qualified external URL (or one that looks external, with a scheme on it). The documentation here and variable naming were to make this explicit.

@David: #31 also removes your forum patch; sun says it was correct the way it was.

Attached is the interdiff between #24 and #31

It may be that we are discovering an additional bug in the taxonomy/term feed URL. Sun's opinion was that it should *have* the /0 because that "has worked since Drupal 4". But maybe it doesn't work any more.

I propose:

* Use $url (sigh)
* Keep the appropriate documentation that goes with it explaining what the allowed uses are.
* Determine whether the taxonomy/term issue is a new bug in D7 and we're accidentally working around that bug. (The /0 is supposed to be a depth selector, according to sun)

One option is to just make #24 RTBC again. Sun had his own personal opinions, but nobody else seemed to object at all to what's in #24. @sun, can you live with that, now that this has played out?

OK, then, I propose #24 as RTBC again. Nobody but sun had an issue with it, and some of that didn't pan out.

Clarified @param $url description.

OK, works for me, thanks, @sun. Marking #39 RTBC. Interdiff from 24 to 39 is attached.

+++ modules/node/node.module	27 Nov 2010 16:48:38 -0000
@@ -2552,8 +2552,8 @@ function node_page_default() {
+    // @see node_menu()

I wasn't quite sure where this comment came from, but fine with me.

Powered by Dreditor

Added that @see in order to clarify that rss.xml is registered menu path and not an actual file in the filesystem. If it was a real file, then it would have to be

url($GLOBALS['base_url'] . '/rss.xml', array('external' => TRUE))

I did not immediately understood the @see without reading comment #41. Can we either take out the @see or explain it better in the code? Thanks!

OK, here it is with the comment rewritten as

+    // 'rss.xml' is a path, not a file, registered in node_menu().
+    drupal_add_feed('rss.xml', variable_get('site_name', 'Drupal') . ' ' . t('RSS'));

Committed to HEAD. Thanks!