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.Problem/Motivation
Once upon a time aggregator did its own RSS parsing.
During the D8 cycle we moved to using a third-party parsing library provided by Zend.
The error is no longer occurring but it is not know if we have test coverage (either in core, or in Zend) for invalid descriptions.
Undefined index: DESCRIPTION in aggregator.module on line 631.
Proposed resolution
Review test coverage in core and Zend and ascertain if there is handling for malformed RSS descriptions.
If so, we can mark this closed (duplicate) and link to the issue where we added Zend's feed parser
If not, we can revive the test coverage here.
Remaining tasks
Review test coverage in core and zend to ascertain if mangled descriptions are handled.
If not, revive the test coverage here.
User interface changes
API changes
Data model changes
Release notes snippet
This error appears if a feed's channel has no description element. Attached patch fixes the error.
| Comment | File | Size | Author |
|---|---|---|---|
| #12 | aggregator-test-no-description-293536-12.patch | 1.93 KB | mustafau |
| #10 | aggregator-test-no-description-293536-7.patch | 2.02 KB | cburschka |
| #8 | aggregator-test-no-description-293536-7.patch | 2.02 KB | cburschka |
| #2 | undefined_description_2.patch | 1.28 KB | mustafau |
| #1 | undefined_description_0814.patch | 846 bytes | obsidiandesign |
Comments
Comment #1
obsidiandesign CreditAttribution: obsidiandesign commentedRerolled the patch - got "fuzz 2 (offset 9 lines)" with the original. Tested with an RSS feed w/o description to confirm the error message, patch corrects the issue. All SimpleTests for Aggregator succeed after applying the patch. +1 for RTBC.
(patch re-uploaded so it's the full path)
Comment #2
mustafau CreditAttribution: mustafau commentedRe-roll.
Critical because aggregator is not working without this.
Comment #3
cburschkaI understand adding the array with empty default values.
Why are you moving the $etag line to above the foreach loop? Is this needed?
Comment #4
cburschkaMh, never mind, it does seem cleaner to put it together with the $modified line.
Testbot thinks this works, and I think the code looks good. RTBC?
Comment #5
Damien Tournoud CreditAttribution: Damien Tournoud commentedThis needs a test.
Comment #6
cburschkaTricky to come up with a meaningful test case there. description is a required field in RSS, so should the test case check for behavior of missing title/link as well?
Comment #7
Damien Tournoud CreditAttribution: Damien Tournoud commentedIf 'description' is a required field, wouldn't the sensible thing to do be to reject the feed altogether?
Comment #8
cburschkaI've added a test function to UpdateFeedTestCase that may catch this, but currently even running the unmodified test case crashes with a 500 on my server, so someone else will need to work on this.
Note: This one *only* adds the test function, you still need to apply the above patch. This is to make sure the unpatched site fails the test.
Comment #10
cburschkaWorks for me; the problem must be on your end, testbot.
Comment #12
mustafau CreditAttribution: mustafau commentedReroll of the test.
The issue is fixed in CVS HEAD. If this test gets committed, I will submit a patch for DRUPAL-6.
Comment #13
brianV CreditAttribution: brianV commentedTest looks correct to me, and covers untested code.
Comment #14
webchickAwesome. Three things:
1. Rather than encoding the invalid feed in a function body, could we create a separate .xml file for it under modules/aggregator/tests, and read it in, as our other aggregator feed tests work?
2. + function testUpdateInvalidFeed() { just needs a quick one-line PHPdoc.
3. // Set correct title so deleteFeed() will work. I don't understand this comment. Could you put it above the line it's referencing rather than beside, and expand it out a little to explain?
Comment #15
cburschkaI don't get that part either. Indeed, the $feed->title and $edit['title'] have to be equal in order for the assert in deleteFeed() to pass. But I don't see why they would not be equal already. I can't run the test right now to find out, alas.
Comment #16
cburschkaUpdate: Apparently I can run tests after all (Ubuntu has a lovely php5-curl package). So to test my theory, I simply replaced the assignment with an assertEqual to see what happened.
Here's what:
FAIL: $feed->title = "d7clean_s965079QOHBI2rXDQ", $edit['title'] = "d7clean_s965079CR5ILeCzhR";
Next: Why does this happen? This would be what the inline comment should answer...
Edit: OH. Sorry. createFeed() generates one name. Then getFeedEditArray() generates a new name. The feed is then updated to the new title. The $feed object is left as it is. When deleting the $feed, obviously deleteFeed() will look for $feed->title in the delete message, although the feed has since then been renamed to $edit['title'] in the database.
Now I understand. :)
Comment #17
sun.core CreditAttribution: sun.core commentedCan we quickly re-roll this sucker to get this done?
Comment #18
mr.baileysSince the bug itself has been fixed in HEAD, this is now a "needs tests" issue, and no longer critical.
Comment #19
Jody LynnComment #20
webchickComment #21
webchick.
Comment #22
webchickSheesh. One of these days I'll get it right. :)
Comment #23
jhedstromIs this still relevant with the move to Zend libraries for feed processing?
Comment #32
larowlanI reviewed this as part of the Bug Smash Initiative.
I think what we need to do here is confirm if there are tests for this type of error in Zend feed parsing and if so, we can close this.
I've updated the issue summary.
Comment #33
larowlanComment #34
quietone CreditAttribution: quietone as a volunteer commentedHere are tests for missing description.
Is that sufficient?
Comment #35
quietone CreditAttribution: quietone as a volunteer commentedComment #36
catch#34 seems fine. I think we can close this one.