I wanted to add some taxonomy terms but this wouldn't succeed because no $node->vid was set. #623424: Mapper for Taxonomy

I believe more modules might need the vid so I think its a good Idea to add this to the node processor modules

// Avoid full node_load() and only fetch vid.
$node->vid = db_result(db_query(" SELECT max( vid ) AS current_vid FROM node_revisions WHERE nid = %d", $nid));

So we just load the vid and avoid a full node_load().
Thanks to stborchert for the code!

CommentFileSizeAuthor
#5 634886-5_vid.patch1.71 KBalex_b
#4 634886_feeds_add_vid-d6-v02.patch1.29 KBkars-t
feeds_add_vid-d6-v01.patch1.48 KBkars-t

Comments

eugenmayer’s picture

eugenmayer commented
Category: feature » bug
Status: Needs review » Reviewed & tested by the community

Its all about updating existing feed-items, so when you refetch an existing feed and want to update the feed-node-items. Its very important to use the correct vid here, as its a primary key and it will not be handeled in node_save.

Switched over to bug, as this will produce bugs for every feed update.

alex_b’s picture

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

Good catch. Can you describe the bug better that you are experiencing?

Some cleanup issues:

1. Look up the current vid from node rather than max(vid) from node_revisions
2. No need for AS current_vid
3. Remove unnecessary spaces from the query
4. As a more general rule - could you roll a patch from the feeds/ directory?

Thank you

eugenmayer’s picture

eugenmayer commented

well its still a question whether we need db_rewrite() here. As a user might "update" feeds he aint able to read...normaly feeds are public anyway..but there also could be compony-intern-feeds.

Its quite a special-special-case but still present

kars-t’s picture

kars-t commented
StatusFileSize
new 634886_feeds_add_vid-d6-v02.patch1.29 KB

Can you describe the bug better that you are experiencing?

Please take a look at node.module from line 909.

If we use node_save and update an existing node without revisioning acitve we end up in line 922

_node_save_revision($node, $user->uid, 'vid');

In _node_save_revision we end up with 

  if (isset($update)) {
    drupal_write_record('node_revisions', $node, $update);
  }

drupal_write_record would set a newly inserted id to the given object. But in this case its an update and $node->vid will always be "0". So if we update a node by node_save we need a vid or any changes will end up will a faulty primary key in the revisions table.

Keeping this in mind and looking at FeedsFeedNodeProcessor.inc from line 27 onto node_save in line 38. If no vid is added we would end up doing an update to revision 0.

So lets reroll the patch with your suggestions.

Thanks to Eugen I did replace the SQL and hopefully the path is better now.

[edit]

well its still a question whether we need db_rewrite() here. As a user might "update" feeds he aint able to read...normaly feeds are public anyway..but there also could be compony-intern-feeds.

I think there are two possibilities.
1. A feed can only be updated by a adminisitrative user. So we don't need this and everything is fine.
2. A feed that is updated could contain nodes that are for any reasons beyonf the reach of the user. Maybe an admin revoked or changed permissions or something. So yes it should be added. But maybe its a better fit inside of the existingItemId() method.

If this really is a problem maybe we should open a new issue for it ;)

alex_b’s picture

alex_b commented
Status: Needs work » Needs review
StatusFileSize
new 634886-5_vid.patch1.71 KB

#4: thanks for elaborating and cleaning up the query. I see that you're also using apostrophes, Pgsql users rejoice :)

I couldn't apply the patch:

ab$ patch -p1 < 634886_feeds_add_vid-d6-v02.patch 
patching file plugins/FeedsFeedNodeProcessor.inc
patch: **** malformed patch at line 15: \ No newline at end of file

(Minor, not related: it's still not rolled from the feeds directory).

Applied with copy/paste.

db_rewrite_sql() is not needed here as a) this is not a listing query and b) Feeds does not apply access restrictions to CRUD operations on feed item nodes.

By assigning a feed item node to an importer, any user with import permissions and automatic import on cron have indirect access to creating, updating and deleting feed item nodes. The settings form states this - I hope to a sufficient extent.'

That said, there may be corner cases where this assumption breaks. I agree with Kars-T in #4 though that this is a matter for a separate issue.

Cleaned up comments, this should be good to go now.

If you confirm that this patch fixes the issue, I will commit.

eugenmayer’s picture

eugenmayer commented

@patch: looks fine for me

@db_rewrite: Well i dont take the "its not a listing" argument for a valid one, as "updating" is more then even listing. By normal logic you need view + update rights to be able to update a node, so actually updating is always kind of listinging.

In the case, that this is not a "live" query, just a cron job run every hour or what, i expect we should use db_rewrite as it just a good coding style to let other limit the result which can be updated.

On the other hand, if fetching nodes is a "cron" task, its superuser anyway, any accesschecks are useless anyway then. So until "fetch and update" is not a user action, iam ok with not using db_rewrite.

kars-t’s picture

kars-t commented
Status: Needs review » Reviewed & tested by the community

So thats the path you want ;)

Applied the patch, run import and nothing strange did happen.

Added common.inc Line 3464

    print_r($values);
    echo $query;

Without the patch:

Array
(
    [0] => 81
    ...
    [8] => 
)
UPDATE {node_revisions} SET nid = %d, uid = %d, title = '%s', body = '%s', teaser = '%s', log = '%s', timestamp = %d, format = %d WHERE vid = %d

With the patch:

Array
(
    [0] => 81
    ...
    [8] => 81
)
UPDATE {node_revisions} SET nid = %d, uid = %d, title = '%s', body = '%s', teaser = '%s', log = '%s', timestamp = %d, format = %d WHERE vid = %d

So its fine now! :D

+++ plugins/FeedsNodeProcessor.inc	18 Nov 2009 15:19:43 -0000
@@ -31,8 +31,11 @@ class FeedsNodeProcessor extends FeedsPr
+        // If updating populate nid and vid avoiding an expensive node_load().

I like the comment style! More straight und smaller this way.

RTBC I'd say!

I'm on crack. Are you, too?

alex_b’s picture

alex_b commented
Status: Reviewed & tested by the community » Fixed

Committed, thank you for the contribution and the thorough review.

#6:

Rewrites node, taxonomy and comment queries. Use it for listing queries. Do not use FROM table1, table2 syntax, use JOIN instead.

From http://api.drupal.org/api/function/db_rewrite_sql

alex_b’s picture

alex_b commented

FYI, this patch actually "broke" tests by fixing the node updates.

http://drupal.org/cvs?commit=290902

Status: Fixed » Closed (fixed)

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