Feeds with proper etags can't be cleaned up and updated in a row. aggregator_remove() clears out all items, resets the hash flag, but does not reset the etag flag of a feed.

Later, when aggregator_refresh() is called, the etag is found and the feed is assumed to be up to date. As a result, aggregator does not parse the feed and create feed items as expected, but aborts the aggregation process and sets a "There is no new syndicated content from..." message.

Will post a patch shortly.

CommentFileSizeAuthor
#32 364467_clear_etag.patch13.61 KBalex_b
#28 364467_clear_etag_3.patch13.51 KBalex_b
#27 364467_clear_etag_3.patch13.53 KBalex_b
#24 364467_clear_etag_3.patch13.77 KBalex_b
#13 364467_clear_etag_3.patch13.52 KBalex_b
#7 364467_clear_etag.patch13.52 KBalex_b
#6 364467_clear_etag.patch13.53 KBalex_b
#5 364467_clear_etag.patch2.17 KBalex_b
#3 364467_clear_etag.patch588 bytesalex_b
#1 364467_clear_etag.patch531 bytesalex_b

Comments

alex_b’s picture

alex_b commented
Assigned: alex_b » Unassigned
Status: Active » Needs review
Issue tags: +Quick fix
StatusFileSize
new 364467_clear_etag.patch531 bytes

This patch fixes the problem.

Status: Needs review » Needs work

The last submitted patch failed testing.

alex_b’s picture

alex_b commented
Status: Needs work » Needs review
StatusFileSize
new 364467_clear_etag.patch588 bytes

fixed patch

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented

I don't suppose I could give you puppy dog eyes and ask you to supply a test with this that confirms this fixes the problem? HmmmmMMMmmmMmmM? ;)

alex_b’s picture

alex_b commented
StatusFileSize
new 364467_clear_etag.patch2.17 KB

Aggregator's tests are using the site's own feed (/rss.xml). For getting etag and a static last-modified headers on /rss.xml we need to go into cached mode (easy) - but for some reason, etags and modified headers on /rss.xml are changing when removing/updating feeds on the same site.

I am assuming that somewhere the page cache is being reset, I did some digging, but I couldn't found out where.

I added a test that updates/removes feed items two times in a row. While it does check whether feed items are updated and removed correctly in this operation, it does not check if etag and last-modified based checks work, because we don't have a suitable feed to test against.

alex_b’s picture

alex_b commented
Issue tags: -Quick fix
StatusFileSize
new 364467_clear_etag.patch13.53 KB

I addressed the problem outlined in #5 by adding an aggregator_test module that generates a test feed with etag, modified or etag and modified headers. (This test module will also come in hander later, when we write test implementations for the aggregator API).

For pulling in this feed I had to refactor parts of the aggregator test: createFeed() only generated feed based on /rss.xml and did not allow for feed urls to be passed in.

The tests attached reliably detect problems when etag or last-modified are not reset properly.

Beyond this patch: Nice side effect is that we now have a framework for adding local test feeds. At some point we should discuss how we would like to handle this. I think fixes for the parser should come in the future with test feeds. I don't think these test feeds should sit all in the modules directory though, but somewhere on drupal.org.

alex_b’s picture

alex_b commented
StatusFileSize
new 364467_clear_etag.patch13.52 KB

Minor fix.

Status: Needs review » Needs work

The last submitted patch failed testing.

alex_b’s picture

alex_b commented

Tests failed because the PHP syntax checker interprets the XML file in the patch: #365889: php -l interprets XML files

alex_b’s picture

alex_b commented

This issue is waiting on #366193: Testbed: Deploy syntax checking fix.

alex_b’s picture

alex_b commented
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch failed testing.

alex_b’s picture

alex_b commented
Status: Needs work » Needs review
StatusFileSize
new 364467_clear_etag_3.patch13.52 KB

Uploading patch again as #7 keeps failing on the testbed while OK on latest HEAD locally.

Status: Needs review » Needs work

The last submitted patch failed testing.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Needs work » Needs review

That time was the Fields in Core patch, which broke HEAD. Sigh...

However, #7 shows a failure with feed-related tests, so that's at least useful. What platform are you testing on?

alex_b’s picture

alex_b commented

#15: Mac OSX, apache2, PHP5.2.6, MySQL 5.0.45

Locally, I can't reproduce the single fail from #7 that also came up in #13 earlier...

Status: Needs review » Needs work

The last submitted patch failed testing.

alex_b’s picture

alex_b commented
Status: Needs work » Needs review

I tested again with configuration in #16 [edit: and] the following configuration:

Linux Fedora10 2.6.27.12-170.2.5.fc10.i686,
Apache2.2
PHP 5.2.8
mysql 5.1.30

No fails.

Status: Needs review » Needs work

The last submitted patch failed testing.

alex_b’s picture

alex_b commented

This patch (still) applies and tests fine on my local installation.

In order to find out why the test bed does find one error, webchick suggested running the test on the command line - like the test bed. But running from the command line results in a bunch of errors on each Aggregator test.

How can I get to the bottom of this issue? Looking at the patch, are there some suspect changes that have caused trouble elsewhere?

Any input much appreciated.

alex_b’s picture

alex_b commented
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch failed testing.

alex_b’s picture

alex_b commented

#20: I did not use the --url option when using run-tests.sh

alex_b’s picture

alex_b commented
Status: Needs work » Needs review
StatusFileSize
new 364467_clear_etag_3.patch13.77 KB

Fixed the error in #13, this is what was going on:

This patch introduces a test feed as a seperate file, getRSS091Sample() exposed the URL to the file using url(). When clean urls are disabled, url() produces an invalid URL (http://host/drupal7?q=modules/aggregator/aggregator_test_rss091.xml instead of http://host/drupal7/modules/aggregator/aggregator_test_rss091.xml)

When clean urls are disabled and tests are run from the command line, this invalid URL causes an error when aggregator.test checks for its availability. However, it does not cause an error if run from the web interface. Interesting lesson learned.

SUMMARY

Problem description: see initial post.

Patch

* Adds aggregator_test.module that exposes a test feed with etag, modified or etag and modified headers.
* Moves test feed from aggregator.test to its own file aggregator_test_rss091.xml so that it is accessible from aggregator_test.module, too.
* Makes createFeed() accept an optional $feed_url (in order to pass in the aggregator_test_rss091.xml url).
* Adds tests for correct etag and last-modified caching to testRemoveFeedItem()

boombatower’s picture

boombatower commented

/me congratulates alex_b

chx’s picture

chx commented

Let's make this clear: url() is NOT for file, it's for Drupal paths. url(NULL) is an ugly hack. Just use $GLOBALS['base_url']

alex_b’s picture

alex_b commented
StatusFileSize
new 364467_clear_etag_3.patch13.53 KB

Addresses issue in #26, should have been part of #24.

alex_b’s picture

alex_b commented
StatusFileSize
new 364467_clear_etag_3.patch13.51 KB

Replaces usage of url(NULL) for grabbing the rss.xml URL with $GLOBALS['base_url'].

chx’s picture

chx commented
Status: Needs review » Reviewed & tested by the community

I am OK with the patch and so is the bot.

alex_b’s picture

alex_b commented

I echo: This is ready to go in.

Once in, I can use aggregator_test.module to write an API + settings UI test for #293318: Convert Aggregator feeds into entities :)

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Needs work

Cool, I have no problem committing this once the following changes are made:

@param URL URL

It's not URL, it's $feed_url

The PHPDoc of the functions you touch should be laid out like this example from file_load():

/**
 * Load a file object from the database.
 *
 * @param $fid
 *  A file ID.
 * @return
 *   A file object.
 *
 * @see hook_file_load()
 * @see file_load_multiple()
 */

So basically, rather than:

@param datatype variablename description all on one line where it's really hard to read.

It should be:

@param $name_of_variable
  Two spaces and then a definition.

+    // @todo: remove db_rewrite_sql() when possible

Are you sure this isn't possible now? http://drupal.org/node/299176#comment-1166241 was committed in December.

alex_b’s picture

alex_b commented
StatusFileSize
new 364467_clear_etag.patch13.61 KB

* Cleaned up comments on functions that this patch touches.
* Removed db_rewrite_sql().

alex_b’s picture

alex_b commented
Status: Needs work » Needs review

setting to 'needs review'

alex_b’s picture

alex_b commented
Status: Needs review » Reviewed & tested by the community

webchick: input in #31 was very helpful, thanks for clarifications :-)

Addressed issues in #31, patch passes automated tests (#32): setting to RTBC again.

dries’s picture

dries commented

This looks good but we need to discuss the location of the test modules.

$ find . -name "*_test.module"
./modules/locale/tests/locale_test.module
./modules/node/node_test.module
./modules/simpletest/tests/database_test.module
./modules/simpletest/tests/field_test.module
./modules/simpletest/tests/file_test.module
./modules/simpletest/tests/form_test.module
./modules/simpletest/tests/menu_test.module
./modules/simpletest/tests/session_test.module
./modules/simpletest/tests/system_test.module
./modules/simpletest/tests/taxonomy_test.module
./modules/simpletest/tests/xmlrpc_test.module

1. Sometimes the modules are in ./modules/simpletest/tests, sometimes not.
2. Sometimes the modules are in ./modules/FOO/tests, somethines in ./modules/FOO.

alex_b’s picture

alex_b commented

#35:

Good point. I am not happy with the sprawl of test files in aggregator/ (there are 4 now).

I think it's funny that in the case of locale module the tests/ subdirectory only contains the test module files, but not the .test file.

As a general rule, I would store all tests in a tests/ directory when there is more than a single .test file. Would that work?

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented

@Dries: The idea was that any module that defined tests would put them in modules/X/(tests/)module.test. To simulate what contrib will do. It used to force tests to be located in a "tests" subdirectory of the module, which was really nice for consistency, but for some reason that restriction was removed when SimpleTest module was added to core.

The test modules in modules/simpletest/tests/* are for testing files in the includes/* directory, which aren't scanned for modules/tests. An argument could be made that these should be moved to system module instead, for consistency.

But ./modules/simpletest/tests/taxonomy_test.module is definitely misplaced, that's true.

IMO we should go back to the old SimpleTest module way of always putting tests/test related files and modules in modules/X/tests/. Because:

a) I dislike that our module directories have turned into "file soup" after we did the page split and created .tpl.php files in Drupal 6. This just makes it dramatically worse.
b) Hiding this stuff in a "tests" sub-directory both means we can feel free to add all kinds of wacky random test-related files in there (as this patch does)
c) We can train our end users that they can just ignore anything in the 'tests' folder because it's 'scary' and for developers only. As opposed to "tum te tum.. looking or a template file I can copy to my theme.. what the?!? What is aggregator_test.module? Do I need to enable that?? Better post a support request on the forums!"

However, would it be okay to commit this patch as-is and handle moving around various test-related files in a separate patch, since this consistency issue affects a lot more than just the aggregator module's tests?

alex_b’s picture

alex_b commented

Dries: It would be great if we could commit this patch as-is, independently from the test file location question.

To that end I created a separate issue for test file locations: #383600: Move tests into subdirectory.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Fixed

Ok, this has been hanging around a long time, so I went ahead and committed it. Let's discuss at #383600: Move tests into subdirectory on the best way forward wrt where files should go.

Thanks a lot! :)

Status: Fixed » Closed (fixed)

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