First off, thanks for addressing my question in #631212: Updating content with Feeds?. It looks like Feeds is going to do what I was hoping it could do, and a lot easier than I was expecting.

The attached patch addresses two issues in FeedsNodeProcessor::process():

- The $node->revision flag wasn't being set to TRUE when a node was updated. Setting this allows for tracking revisions to updated nodes - very handy.
- The user was notified that X nodes had been updated only if nodes had been updated, with no new ones being created - in other words, if nodes were both created and updated, the user was told that nodes had been created, but not told how many had been updated. This fixes that, and also uses format_plural() to avoid messages like "Updated 1 story nodes."

Thoughts: It would be nice if it were possible to only update a node if it appears that something about it is going to change; currently, the mere presence of the item in the feed appears to be enough to trigger an expensive node_save(). However, I'm not sure the best way to go about this without using a node_load() - also expensive. Perhaps the parsers would mandatorily return a hash for each item, which could be stored and later compared for changes in that item…

CommentFileSizeAuthor
#8 631340_8_correct_plural_on_delete.patch465 bytesvelosol
fnp-notifications-revisions.patch1.91 KBGarrett Albright

Comments

velosol’s picture

velosol commented

Looks good to me - and I believe my patch in #631248: FeedsNodeProcessor doesn't set 'log' when updating nodes can coexist and extend the revision support.

Edit: Re: updating only when needed: I've had the same thoughts, personally I think the node_load expense would be better if you're going to have to go to the database for the hash. Then again, I'm relatively inexperienced in what the performance costs would be with each method.

Garrett Albright’s picture

Garrett Albright commented

personally I think the node_load expense would be better if you're going to have to go to the database for the hash. Then again, I'm relatively inexperienced in what the performance costs would be with each method.

Hashes: One simple (no joins) DB call per node; or, if we can build a list of nids for all the nodes beforehand, one DB call to get all hashes.

node_load(): DB calls to get the basic data for the node; calling the hook_load() for that node type (possibly more DB calls); calling every implementation of hook_nodeapi() with $op = 'load' (possibly more DB calls). At least some of these DB calls will require joins, especially if CCK is in play.

It's no contest, man…

alex_b’s picture

alex_b commented
Status: Active » Needs work

This patch breaks tests. I have not dug up why. If you run tests, make sure all of them pass on a vanilla set up. Not many people are using the tests yet.

Defaults: Node import: 69 passes, 8 fails, and 1 exception

Right, update-if-changed could be implemented pretty cleanly with a hash that is computed and stored every time a feed item node is created. I would avoid node_load() at any cost as it bloats memory by caching every node loaded.

velosol’s picture

velosol commented
Status: Needs work » Active

Thank you for the clarification on the 'hidden' costs of node_load.

A possible direction:

  1. Grab the unique_target from $item passed to FeedsNodeProcessor
  2. Do a lookup to a table whose fields are unique target & hash (or an updated feeds_node_item)
  3. If there's a match for $item->unique_target, compare the hash of $item with the retrieved hash
  • If they're different update the node and the hash
  • If not, break and skip the expense of the node_save.
  • If there's no match for the unique_target, it's a new node and a hash needs to be stored before/after the node is saved.

    • This wouldn't slow down the parser and it keeps the code in one place without an API change ('just' the added database or database field to feeds_node_item).

    What do you think? Any gaping holes?

    velosol’s picture

    velosol commented
    Status: Active » Needs work

    Didn't mean to change status from 'needs work' - changing back.

    Garrett Albright’s picture

    Garrett Albright commented

    Tests? My patches need to pass tests? Curse you, quality-oriented programmers!

    I'm probably not going to be able to get able to fiddle with this any more Friday, but I'll try to get to it over the weekend if nobody else beats me to it.

    I would avoid node_load() at any cost as it bloats memory by caching every node loaded.

    node_load()'s third parameter, $reset, lets you reset its cache before it loads its next load, so if that is used, the memory issue isn't worrisome so much. There's no helping the speed, though.

    alex_b’s picture

    alex_b commented

    I've broken out the update if changed issue here #631962: FeedsNodeProcessor: Update when changed.

    #4: on #631962 there is an outline on how the feature could be implemented.

    velosol’s picture

    velosol commented
    StatusFileSize
    new 631340_8_correct_plural_on_delete.patch465 bytes

    While poking around trying to figure out what was causing the tests to fail, I noticed that the 'Deleted X nodes' text needed to be pluralized as well. I've included a patch to take care of just the deleted message (i.e. patches would need to be merged before applying). Unfortunately, I don't think it fixes the test failures caused by the first patch :) .

    alex_b’s picture

    alex_b commented
    Version: 6.x-1.0-alpha7 » 6.x-1.x-dev

    I fixed the "broken" tests. #1 like #634886: Add vid to node proccess functions actually fixes node updates, tests assume them (unknowingly) broken. Review: http://drupal.org/cvs?commit=290902

    alex_b’s picture

    alex_b commented
    Status: Needs work » Postponed

    Postponing after prolonged period of inactivity.

    kenorb’s picture

    kenorb commented
    Issue summary: View changes
    Status: Postponed » Closed (outdated)

    Closed because Drupal 6 is no longer supported. If the issue verifiably applies to later versions, please reopen with details and update the version.