FeedsNodeProcessor: Update when changed

Trying to gather some initial ideas for the additions that need to be made:

Added to feeds.install $schema['feeds_node_item']:

//Snip $schema['feeds_node_item']
  'hash' => array(
        'type' => 'varchar',
        'length' => 32, //The length of the output of md5()
        'not null' => TRUE,
        'default' => '',
        'description' => t('The hash of the item.'),
      ),
//End snip
//An update function?
function feeds_update_600X() {
  $ret = array();

  $spec = array(
      'type' => 'varchar',
      'length' => 32, //The length of the output of md5()
      'not null' => TRUE,
      'default' => '',
      'description' => t('The hash of the item.'),
    );
  db_add_field($ret, 'feeds_node_item', 'hash', $spec);
  return $ret;
}

Hash item function:

protected FeedsNodeProcessor::hash($item) {
  $to_hash = '';
  if(is_array($item)) {
    foreach($item as $val) {
      $to_hash .= $val;
    }
  }
  else {
    return FALSE;
  }
  return hash('md5', $to_hash);
}

getHash function:

protected function FeedsNodeProcessor::getHash($nid) {
  $hash = db_result(db_query('SELECT hash FROM {feeds_node_item} WHERE nid = %d', $nid));
  if ($hash) {
    // Return with the hash
    return $hash;
  }
  return 0; //Or FALSE?
}

With regards to the hash function and database definition - it may be better/faster if we use the second argument of md5() to return the raw output (see: http://us2.php.net/manual/en/function.md5.php ) and treat it as an int.

Thoughts, additions, corrections, requests?

Edit: corrected syntax of update function and made protected func syntax match.
Edit2: reading up on that PHP page suggests that hash('md5', $to_hash) is faster than md5(), adjusting.

[edit: most up-to-date below]
Patch, did someone say patch?

I modified FeedsNodeProcessor::map() as you suggested so that it can modify the $node as a parameter - however it still returns - good functionality?

Edit: I also left node_object_prepare in map() as the include statement for the node module is there and I didn't want to break it out of map.

I also included an update function in install as 6007.

I tested by running update.php then first deleting nodes and then importing again; and then trying to import again, I got a happy "There is no new content." I did NOT test importing over nodes that did not yet have a hash set - but I would assume that they would get one final 'fake' revision.

[edit: most up-to-date below]
New patch- the last node_processor.patch was overwriting the $node even if it already existed because $node = new stdClass(); came after $node->nid = $nid when a $nid was returned by existingItemId().

This new one moves the creation of the $node up to where before it's really needed, but is cleaner.

@alex_b:

I have addressed most of the stuff in my working copy of the code - I can post that as a patch if you'd like, but I'd prefer to get some feedback re: my questions in 3 & 5.

1. D'oh! Was patching to alpha7. I can grab stuff from CVS, but it's a huge pain on this Windows box - any chance the -dev/HEAD portion could be tarballed for the front page of Feeds?
2. Will do in future patches, my mistake.
3. Done - all that's left is self::loadMappers() and the parent::map() call - do you want the static $included stuff to live on, or just do the module_load_include() call each time?
4. My bad again, it was in there because I had borked a previous attempt at the patch and didn't want to do it again - I'll take it out before rolling further patches.
5. I'm in the habit of type-checking - and it provided an easy way to return on bad input. If that's not a concern (second half of point 6?), do we even need the overhead of another function call vice:
$hash = hash('md5', serialize($item)); in process()? *
6. Updated getHash() in my working copy to return an empty string when no hash is found. **
7. Done.
8. Definitely! Unfortunately I'm not really great with Drupal's test framework - I'll poke around in other tests and see if I can come up with something, but I expect someone else may be able to do a better job.

* - I can't get hash('md5') nor md5() to ever return NULL (I tried passing, NULL, FALSE, empty strings to each) - only to get the d41...27e hash every time. Beyond that I'm not committed to any particular hash creation method.

** - Forgive my ignorance, but if the query doesn't find anything, the db_result/db_query returns FALSE, not the schema default, right? So we'd still have to catch the FALSE and return ''?

[edit: most up-to-date below]
Alright, based on the past couple of comments, I've got a new version of the patch (against dev/1.15). The last hunk looks strange (removes and adds the final '}', notes 'No newline at end of file') as I didn't touch that function - but I've left it in for the time being. I've also added to the function header comments to clarify behavior (protected hash always returns a hash, getHash returns hash or empty string).

I'll see what I can do to get a test set up for this functionality.

Is the feeds.install patch from comment #4 OK once this patch is up to snuff?

I guess I still need some more coffee!
[edit: see below for bug cause - and fix]
The patch in #9 will update a node, but not create a new revision. I was expecting to get revisions as I had with the patch in #5, but that isn't happening anymore - I think it's because of the $node->vid addition, although I can't pin it down to that.
Sorry I didn't catch this before posting the patch, I had only been checking if the node content was updated, not if the "revisions" link was available.

To get revisions again:

$node->nid = $nid;
$node->vid = db_result(db_result(db_query('SELECT vid FROM {node} WHERE nid = %d', $nid));
$node->revision = TRUE;

Ultimately the question is: What is the "correct" behavior? I think creating a new revision is expected, but that could be just for my use case.

And yes, I realize that this just further highlights the need for tests :) .

The 'bug' in #10 was my own fault - copy and paste coding fails when the variables change; node_object_prepare() is now called properly and the node's revision status is set based on the setting in the content type (as in #11).

I'm still working towards tests.

The .install indentation is fixed and both .install and FeedsNodeProcessor.inc are in this patch.

Some feed format (Atom for instance) provides a field that indicate the last modification time of an item (in Atom, that would be the updated element).

In addition to hashing, wouldn't it be nice to have the option to only update a node only if $node->changed < $item->udpated ?

After a bit of a fight with testing and the core patch, I've got tests written and passing to verify the update-if-changed behavior. The other RSS file I've added (/tests/feeds/developmentseed.rss3) I had to attach separately as my cvs/diff would not incorporate it into the patch.

Please review and let me know if more needs to be done.

@monoglito404 : I believe this patch will take care of the $node->changed < $item->updated case by virtue of hashing. If there's a specific case where hashing wouldn't take care of that use case (or another similar situation) please post back.

I'm having some trouble getting (any) tests to work in my environment right now so I can't really put your patch (17) through its paces. I did however just do some import tests by hand and it seems as though $node->log is cleared by node_object_prepare() - moving the $node->log = "Created/..." below the n_o_p call fixed it.

I will post back once I've got my test environment up and running again and an updated patch.

Edit: I forgot to mention: everything else you did looks great and other than the above, I think process() is good - it had been arranged the same as before with the map() code in place of the map call for consistency, so no 'real' reason. Both edit and createFeedNode could use some cleanup/unification, but I didn't know where to start so I just mimicked the old code. Also, I can't believe I missed setSettings!

Thank you for the offer of help - it appears as though my issue is #523878: Fatal error: Maximum execution time of 180 seconds exceeded which hasn't yet been backported as it hit HEAD after SimpleTest 2.9 was made. I'll get the patch into my SimpleTest and hope that clears things up. Posting here so others with a similar issue may find the fix - if it still doesn't fix the problem, I will open a new issue.

This is the referenced patch:

+++ modules/simpletest/drupal_web_test_case.php	16 Aug 2009 20:26:48 -0000
@@ -31,7 +31,7 @@
   /**
    * Time limit for the test.
    */
-  protected $timeLimit = 180;
+  protected $timeLimit = 500;

My testing platform is working again, yay! After moving the $node->log as in 18 the RSS import to nodes test was working fine (as before) and no more log error on manual import.

I found errors in Defaults: Node import test and ran the import manually - it was creating 9 Story nodes (incorrect) and also complaining that 'url' didn't have a default value. This happened regardless of location of $node->log. If I added a mapping for (CSV) guid => (DB) url, it imported correctly. I assume the correct behavior is to not have to map anything to 'url', but then Drupal complains about the INSERT queries. I'm leaving NW because of this, but it may not be related.

I also made minor doc updates in feeds_default.test. The updated patch has new files included (thanks for the cvsdo tip!). While the RSS to node test passes, I had been seeing errors on others*.

*:I was getting #587304: Call to undefined function drupal_realpath() on the CVS import to users, Defaults: Node import, and other(?) tests. I made the adjustment as noted in that issue and now everything not already mentioned that I can test is passing (i.e. those tests requiring SimplePie or Data were not run).

Maybe this is an edge case, but this doesn't work if your feed items don't have timestamps. The reason is that "Feeds" applies the feed timestamp to all the items if there is no timestamp. Reasonable enough, but the hash in this case is guaranteed to be different from the hash value in the node regardless of whether the content is different.

I'm just adding a timestamp to my feed items now, but this is just an FYI.