Hash check fails when running multiple importers on cron [#1848726]

I have several importers in my site, which is set to run "as often as possible". When cron is executed, some of them seems to be updating every item even though nothing has changed. If I run the importers manually from the standalone form, everything works as intended, returning "There are no new nodes".

I've traced this down to the hash check, in FeedsProcessor::process(). The stored hash is loaded as it should, but the hash for the current item is being calculated differently. It seems as this is due to a static variable in FeedsProcessor::hash(), which stores the serialized mappings. I think that the static variable is getting it's initial value based on the importer that is triggered at first. The rest of the importers, will then use that static variable.

I haven't done any thorough research, but I've removed the static variable to test my thoughts, and everything is indeed working as it should.

Comment	File	Size	Author
#4	feeds-serialized-mappings-1565890-4.patch	1.07 KB	twistor
#4
#1	1848726-1-feeds-hash_check_cron.patch	658 bytes	olofbokedal
#1

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

olofbokedal CreditAttribution: olofbokedal commented 23 November 2012 at 15:36

Status:

Active

» Needs review

File	Size
1848726-1-feeds-hash_check_cron.patch	658 bytes

This patch simply removes the static variable, causing the mappings to be serialized every time.

Comment #2

olli CreditAttribution: olli commented 21 December 2012 at 09:50

Marked #1565890: Nodes updated sometimes when feed hasn't changed as a duplicate.

Comment #3

olli CreditAttribution: olli commented 21 December 2012 at 10:10

Version:	7.x-2.0-alpha5	» 7.x-2.x-dev
Status:	Needs review	» Reviewed & tested by the community

#1 solves this.

Comment #4

twistor CreditAttribution: twistor commented 17 January 2013 at 23:54

Status:

Reviewed & tested by the community

» Needs review

File	Size
feeds-serialized-mappings-1565890-4.patch	1.07 KB

We really shouldn't get rid of the cache entirely.

Comment #5

olli CreditAttribution: olli commented 18 January 2013 at 09:43

Thanks for looking at this.

#4 makes sense. Would #1 brake something or just be a little slower?

Comment #6

olli CreditAttribution: olli commented 18 January 2013 at 09:47

+++ b/plugins/FeedsProcessor.inc
@@ -763,11 +768,10 @@ abstract class FeedsProcessor extends FeedsPlugin {
    *  Empty/NULL/FALSE strings return d41d8cd98f00b204e9800998ecf8427e

I guess that has not been true for a while...

Comment #7

olli CreditAttribution: olli commented 18 January 2013 at 11:11

Hm. Do we need an update function for this?

Additionally, would it make sense to cache the hash of serialized mappings?

Comment #8

olofbokedal CreditAttribution: olofbokedal commented 18 January 2013 at 16:42

Without any actual testing, I believe that #4 would work, but I can't see that caching would mean any significant performance improvements, since it's a simple matter of serializing. #1 is simpler to understand and maintain, but #4 would mean that the serializing is only done once.

However, this is a small change. Both patches would work, the choice is up to the maintainer. This won't need an update function.