Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.Use any Blogger ATOM feed. Here's a random one:
http://www.librarything.com/thingology/atom.xml
Add it to Aggregator. Create a block. Roll your mouse over the headlines in the block.
You'll see that the links are all the same. That's a bug in Aggregator. The problem
happens because when aggregator parses the "CONTENT" tag, it sets $element=''
That happens in the parser routine aggregator_element_end();
The fix:
1) Remove this line from the function aggregator_element_end()
case 'CONTENT':
2) In the function aggregator_element_start() remove this line:
case 'CONTENT':
and then add it back in near the bottom of the switch statement like this:
case 'CONTENT':
if ($element != 'ITEM') {
$element = $name;
}
break;
| Comment | File | Size | Author |
|---|---|---|---|
| #44 | aggregator-rt-ij.patch | 5.24 KB | Ian Johannesen |
| #31 | aggregator-atom-130344-31.patch | 6.61 KB | David_Rothstein |
| #29 | aggregator-atom-130344-29.patch | 16.33 KB | David_Rothstein |
| #18 | aggregator_atom_fix-18.patch | 18.18 KB | mustafau |
| #11 | test-feed-parser-atom-11.patch | 5.13 KB | mustafau |
Comments
Comment #1
slimandslam CreditAttribution: slimandslam commentedNote: I have verified this bug in Drupal 4.7.6 and 5.1
Comment #2
buddaWrong project?
Comment #3
Boris Mann CreditAttribution: Boris Mann commentedThis seems to have correctly identified the issue and fix for Atom feeds (aka Blogspot in many cases). Setting to 5.x as bug fix, need a patch rolled.
Comment #4
Zen CreditAttribution: Zen commentedPlease provide your solution in patch form, preferably against HEAD / 6-dev.
Comment #5
csevb10 CreditAttribution: csevb10 commentedPatched against HEAD / 6-dev according to the post by slimandslam
Comment #6
catchThis still applies with offset. Small fix. I don't use aggregator.module so could do with review by someone who does.
Comment #7
mustafau CreditAttribution: mustafau commentedRe-rolling.
Aggregator tests are passing. Also did not break tests I have written for #72915: Move feed parser to includes/feed.inc
Comment #8
Dries CreditAttribution: Dries commentedWould be great to have a test case for Atom feeds that illustrate this bug. With a test case, this patch will go in with the blink of an eye.
Comment #9
mustafau CreditAttribution: mustafau commentedA test case is attached.
I have created more tests than this however some of my tests are failing. I will open another issue after this issue is fixed.
Comment #10
Dries CreditAttribution: Dries commentedI think this looks good, but I'd prefer to trim down the feed and to make it link to example.com, etc. I'm not sure it is a good idea to include that feed "as is". Let's clean it up a little so it is actually a fake feed but with the same features. Almost there ...
Comment #11
mustafau CreditAttribution: mustafau commentedSample feed links to example.com now.
Comment #12
mustafau CreditAttribution: mustafau commentedComment #13
mcd CreditAttribution: mcd commentedI tried the patch against drupal 5.10, and while it worked for the blogspot feed that was giving me issues, I later added a new blogspot feed which is currently duplicating link targets as described:
http://walterjonwilliams.blogspot.com/feeds/posts/default
Comment #14
Anonymous (not verified) CreditAttribution: Anonymous commentedThe last submitted patch failed testing.
Comment #15
naudefj CreditAttribution: naudefj commentedSubscribe - also an issue in Drupal 6.8.
Comment #16
yogayak CreditAttribution: yogayak commentedsubscribe... still an issue
Comment #17
Leeteq CreditAttribution: Leeteq commentedSubscribing.
Comment #18
mustafau CreditAttribution: mustafau commentedFeed parser is seriously broken. Attached patch has a number of tests to show those broken areas. I do not know how to fix this.
* image tag in RSS 0.91 is not being parsed.
* items from RSS 2.0 are not being parsed.
* Atom feeds are not being parsed at all.
* Every resulting item from an atom feed has $item['entry'] as an offset.
* $feed->items had actual + 1 number of items.
Comment #19
shark CreditAttribution: shark commentedsubscribing
Comment #20
sun.core CreditAttribution: sun.core commentedThis is annoying, but not critical.
Comment #21
abamarus CreditAttribution: abamarus commentedI applied the patch in #7 to my install. I didn't think it was working then realised that I needed to truncate the table with the feed items in - once I'd got rid of the items and re-parsed the feed everything worked fine.
Comment #22
abamarus CreditAttribution: abamarus commentedAddendum - I was using the you tube atom feed for a channel. Just in case you wanted to know!
Comment #23
Sturmey CreditAttribution: Sturmey commentedThis has been listed as an issue since 4.x and it is still an issue in 6.17 (the latest). Is Drupal this useless at pulling in atom feeds?
Is anyone doing anything useful about this? trying to parse an atom.xml feed through feedburner is NOT a valid solution. wordpress, joomla, firefox, igoogle, all seem to be able to pull in a blogspot/atom.xml feed. Drupal must resolve this. This may be what switches my installations over to Joomla.
Comment #24
Boris Mann CreditAttribution: Boris Mann commentedWHy yes, I *am* adding to critical bugs in D7! Let's not ship broken for another release, and at least get the tests in.
Comment #25
catchNo let's not promote completely normal bugs to critical ones just because they're annoying.
Comment #26
buddaHow about we just ditch the crappy old aggregator.module and let people who want to aggregate stuff use new contrib modules like http://drupal.org/project/feeds?
Comment #27
Boris Mann CreditAttribution: Boris Mann commentedcatch: so what's your plan for fixing this? Or, shall we just ship with aggregator critically broken? Shouldn't we at least get the tests in?
Setting back to critical. A core module doesn't do what it's supposed to do. Isn't that the definition of critical?
Comment #28
catchNo, that's the definition of a bug, we have lots of those.
Comment #29
David_Rothstein CreditAttribution: David_Rothstein commentedThis issue seems to have a confusing history, but as per #18 things are certainly very broken in Drupal 7 (big regression from Drupal 6). ATOM feed item titles don't work at all; clicking on them reloads the current page rather than linking back to the original source.
Linking back to the original source is one of the main points of feed aggregation, so this seems extremely broken. Let's settle on "priority-major" for now :)
Why did someone set this back to active when there was a working patch??? Here is a reroll of #18 to chase HEAD; this fixes the bug described above. It sounds like the tests revealed other bugs as well, but we can split those off to other issues if necessary. For now, let's see what the testbot does with this.
Comment #31
David_Rothstein CreditAttribution: David_Rothstein commentedOK, @mustafau in #18 is certainly right that a lot of things are broken in the aggregator module (as seen by all those failing tests), but the attached patch narrows it down so that we tackle the main problem we've been discussing here. With this patch, most Atom feeds seem to be parsed correctly and link back to the original source. This includes the original feed posted at the top of this issue, as well as some other ones I've tried (although probably not all of them).
By getting this patch in, we fix one of the major bugs and get some basic feed parsing tests in place that we can build off of in followups.
Comment #32
edvanleeuwenA work-around as suggested in http://www.google.com/support/blogger/bin/answer.py?hl=en&answer=97933 is to add ?alt=rss to specify it being RSS as opposed to ATOM.
Comment #33
marcingy CreditAttribution: marcingy commentedChanging to major as per tag.
Comment #34
MustangGB CreditAttribution: MustangGB commentedTag update
Comment #35
dman CreditAttribution: dman commentedSubscribe. This is a surprising and easily fixed real bug I encountered today.
I too went through this whole process, and even created a set of tests that look a lot like those posted here at #31, so if nothing else, I can vouch for me reaching almost identical code independently.
(Should have searched harder before solving it myself, gah)
(I even used the same test file and strings to test :)
... anyway.
These tests were needed - put 'em in.
And the fix works great in manual testing also. Happy!
Comment #36
Dries CreditAttribution: Dries commentedLooks good and fixes an annoying non-critical bug. Committed to CVS HEAD. Thanks for the tests too, David.
Comment #37
Sturmey CreditAttribution: Sturmey commentedJust to be clear, What do we as common users do to solve this issue?
apply a patch? - great where and how?
no longer use atom feeds, but the rss option? - Hmmn, doesn't really solve the original issue, but if it works, I can live with it I guess.
Does this solve all the atom feed problems that drupal has?
Comment #38
dman CreditAttribution: dman commented@sturmey
The fix has been reviewed and now has been applied to Drupal7-dev
Your options are
- wait for the next Drupal7 alpha version roll (yay)
- download and run the current Drupal7 dev version (if you really want)
- patch your current code using the instructions found in the usual place. (probably no point, that's only for development testers and code reviewers)
If you are using Drupal 7 already, you are not a common user. This is just a small issue that was introduced and now is later fixed in the D7 upgrade. There are no known side effects or other Atom issues relating to this one that we know of. If you know of others, open a new issue.
Comment #39
Damien Tournoud CreditAttribution: Damien Tournoud commentedI guess this needs to be somehow back-ported to D6 and D5.
Comment #40
edvanleeuwen@sturmey
If you are currently using 6.x and you only need to have the feed working properly, you could add the "?alt=rss" option. dman has pointed out that this is solved without this workaround, but only for 7.x for now.
Comment #41
David_Rothstein CreditAttribution: David_Rothstein commentedSturmey:
I'd suggest creating separate issues for other Atom feed problems. Evidently there are some...
(If doing so, it would be worth cross-linking them here, especially since mustafau's patch in #18 already contains some tests for a number of other things that are apparently broken. Those tests could be moved to the other issues so that they can be used as a starting point for fixing each bug.)
Comment #42
GreyHawk CreditAttribution: GreyHawk commentedDavid -- was that patch in #31 for D6, D6-dev or D7?
Comment #43
David_Rothstein CreditAttribution: David_Rothstein commented@GreyHawk: #31 is against Drupal 7. It would need to be rerolled for Drupal 6.
Comment #44
Ian Johannesen CreditAttribution: Ian Johannesen commentedHaving looked through a gazillion bugs I think this one hits most close to home. So here goes...
Firstly, all links in current D6 version 6.20. I've found that if there's a relative link set (like facebook does on all their feeds be it Atom or RSS) and they specify a relative link for each item. Then the aggregator module doesn't correctly pick this up.
Secondly, not to be cross posting the patch, it also fails at using the created/published time from the feed, which I've also included the fix for in the patch.
I've enclosed a unified diff against the file modules/aggregator/aggregator.module
Hopefully this, or a permanent fix for those two problems, can make it into the source, so I won't have to keep on applying the patch (or even worse manually applying it line by line as something else changes in the source).
Comment #45
evanion CreditAttribution: evanion commentedI would like to confirm this against other Drupal Feeds.
For instance http://www.swtor.com/feed/news/all that is generated by Drupal over at swtor.com.
I have tried it on 2 d7 sites.
Comment #46
Albert Volkman CreditAttribution: Albert Volkman commentedComment #47
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedThanks. This should go against 8.x (now) first.
So, as for the patch: It needs a reroll. Also please use spaces instead of tabs. And we need a test case, probably.
Comment #48
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedAh ... wait ... looks like it already got into 7.x and thus 8.x. Maybe just fix the spacing then.
Comment #49
David_Rothstein CreditAttribution: David_Rothstein commentedIf I remember right, things were quite different between D6 and D7 in this issue... and the patch in #44 certainly looks a lot different than the patch that was committed to D7.
So if there are additional bug fixes being addressed here, we do need to make sure they are already fixed in D7 and D8 before adding them to a D6 patch.