I have imported several items through the feeds importer (+XPath parser). After initial import I edited some of the other fields (which are not imported), created menu links, and put the node processor settings to the "update" setting.

When I import the feed again the relevant fields are updated, the edited fields keep their content but... the menu links are deleted!

Is this a bug or a feature? Seems a bug to me...

CommentFileSizeAuthor
#2 1245094-1.patch792 byteschrisdejager

Comments

chrisdejager’s picture

chrisdejager commented
Status: Patch (to be ported) » Closed (won't fix)

Earlier this day I thought the problem was caused by a core 7.7 bug in the menu module, see: http://drupal.org/node/1247506

But the real problem is within the Feeds Node Processor plugin. If nodes from a feed are going to get updated, not only node_load is called but also node_object_prepare (plugins/FeedsNodeProcessor.php:50):

protected function entityLoad(FeedsSource $source, $nid) {
    if ($this->config['update_existing'] == FEEDS_UPDATE_EXISTING) {
      $node = node_load($nid, NULL, TRUE);
    }
    else {
      // We're replacing the existing node. Only save the absolutely necessary.
      $node = db_query("SELECT created, nid, vid, type FROM {node} WHERE nid = :nid", array(':nid' => $nid))->fetchObject();
      $node->uid = $this->config['author'];
    }
    node_object_prepare($node);
    // Populate properties that are set by node_object_prepare().
    if ($this->config['update_existing'] == FEEDS_UPDATE_EXISTING) {
      $node->log = 'Updated by FeedsNodeProcessor';
    }
    else {
      $node->log = 'Replaced by FeedsNodeProcessor';
    }
    return $node;
  }

If a node is going to get replaced it is (arguably) logical to call node_object_prepare when you are doing a custom node_load hack. In this way missing fields are filled in with the defaults.

But if you are doing a update this call is not needed. In the case of Drupal core 7.7 (and possibly below, I did not check) the problem is that an existing menu link to the node will be deleted. This is explained in the issue I posted on the menu module for Drupal 7.7 core: http://drupal.org/node/1247506

To resolve the problem I propose that the node_object_prepare is moved 2 lines up into the else block executed only in the case of a node replace:

    if ($this->config['update_existing'] == FEEDS_UPDATE_EXISTING) {
      $node = node_load($nid, NULL, TRUE);
    }
    else {
      // We're replacing the existing node. Only save the absolutely necessary.
      $node = db_query("SELECT created, nid, vid, type FROM {node} WHERE nid = :nid", array(':nid' => $nid))->fetchObject();
      $node->uid = $this->config['author'];
      node_object_prepare($node);
      // Populate properties that are set by node_object_prepare().
    }

Proposed patch is attached.

chrisdejager’s picture

chrisdejager commented
Status: Active » Patch (to be ported)
StatusFileSize
new 1245094-1.patch792 bytes
chrisdejager’s picture

chrisdejager commented
Status: Closed (won't fix) » Needs review
twistor’s picture

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

The comment shouldn't move, but other than that, she be golden!

febbraro’s picture

febbraro commented
Status: Reviewed & tested by the community » Postponed (maintainer needs more info)

I can't reproduce this to see if the patch actually fixes anything. Can you provide a detailed set of steps to produce the error, or a unit test in the patch?

emilcarpenter’s picture

emilcarpenter commented

Thanks chrisdejager, that helped me out!

sr631’s picture

sr631 commented

+1 I am having this same exact problem. I would love to see this fixed in a real release rather than a patch, so per febbraro's request #5 above, I'll provide my steps to hopefully help.

1. Create a content type with any number of fields. (ex: product)
2. Setup a feed to run as a stand-alone which create and/or update exiting nodes
3. Map the feed to the fields
4. Run the feed manually from the stand alone form.
5. Confirm that node have been created with feed data
6. Edit any node that was created by the feed - check 'provide a menu link' and add it to the menu (in my case, main menu under 'our products')
7. Edit the feed data - so it will trigger an update on next run (ex: new price)
8. Run the feed manually from the stand alone form
9. Confirm that the edited node has the new data (ex: new price), but is no longer in the menu.

Thanks!

sr631’s picture

sr631 commented
Status: Postponed (maintainer needs more info) » Reviewed & tested by the community

I'm not sure if I was supposed to change the status so the maintainer knew more info has been provided in my last post. But this patch also worked for me too and it would be great if it could be in a real release.

My apologies if I'm not following the right process. Please let me know for next time.

dman’s picture

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

I'm hitting this also #1534356: Data loss - menu items unintentionally delete themselves if a node is updated via code (not via form UI) . Test case attached.

This patch just removes the call to node_object_prepare() - though there are reasons that's there. node_object_prepare() usefully fills in any missing default data that is needed to perform a safe save. However, due to an inconsistency with menu.module, it also discards the menu item unexpectedly.

(I think) The/A work-around to deal with this - just for menu items, because it's only menu.module that deletes data like this - is to set the flag that menu_node_save expects to see, but that it forgot to set itself.

    node_object_prepare($node);
    if (!empty($node->menu)) {
      // If the node has a menu item(with a valid mlid) it must be flagged 'enabled'
      &$node->menu['link']['enabled'] = (int) (bool) $node->menu['link']['mlid'];
    }
billstennett’s picture

billstennett commented

We use feeds and xpath parser and I can reproduce this using the exact same steps as in #7 above

I've got little experience of using patches though so not sure which of the two suggestions above is the best course of action. Is this a 'fix' that will be incorporated into Feeds at some point or is it a menu.module issue?

It would be good to understand the best approach to take both to fix the immediate issue and for the longer term.

Thanks

dman’s picture

dman commented

I believe (#1534356: Data loss - menu items unintentionally delete themselves if a node is updated via code (not via form UI) . Test case attached.) that the full fix would be best in menu.module : which should not arbitrarily be throwing away user data just because another module didn't use the one form provided by menu.module for editing.
I believe we should be able to safely update nodes via code without this happening. The cause (creating an unstable setting in $node->menu) and the error result (deleting entires found in $node->menu when they are unstable) are both in menu.module.
Feeds just happens to be a utility that triggers both A and B in succession.

HOWEVER, it'll take somewhere close to never for the one-line repair to get into core #1247506: menu link deleted on programmatically updated nodes even though I wrote a test case for it.

INSTEAD I guess the only thing to do is work-around it in feeds. It's only three lines more here to deal with the issue.

FTR, token.module also encountered this bug a while ago and has a documented work-around in place also.

twistor’s picture

twistor commented

@dman,
I followed your issue trail yesterday, good on ya.
In the supplied patch above, you said that it just removes the node_object_prepare, which is not quite true. It moves the call, so that it's only called when overwriting the node, and not updating. The logic, as I understood it, is to apply defaults for the initial node_save(). Those defaults shouldn't have to be set again.

I'm wondering what the value is with calling node_object_prepare() and specially handling the menu issue. After a quick glance, it looks like menu, book, comment, and translation, are the only things that implement it in core. We don't really support any of those modules(yet) in Feeds. I'm reluctant to add code to support a feature we are not really using.

On the other hand, I'm sure there's all sorts of special stuff in contrib that uses hook_node_prepare().

hrm

twistor’s picture

twistor commented

In the name of increasing stability, I'm thinking this should get committed as per #9. It changes the existing behavior the least.
We can re-evaluate if support for mapping to menus gets added. It's one of those weird cases, similar to #1001590: Path alias mapping target, where support should really be entity-based, but nodes are the only ones we can support in a straight-forward manner.

I guess what we should really be doing is a form submission to save nodes, sigh.

twistor’s picture

twistor commented
Status: Needs work » Fixed

Committed.

http://drupalcode.org/project/feeds.git/commit/8b1900b

agoradesign’s picture

agoradesign commented
Status: Fixed » Needs work

Hi,
sorry I have to switch the status back to "needs work" because if the node you're updating doesn't have a menu link, you get lots of error messages in your log. You should rather check for empty($node->menu['link']) than for only the menu. It seems that $node->menu is always set, but the "link" key is of course only available, when there's a link.

Sorry that I can't supply a patch here in my post, I'm very busy working at the moment... Just wanted to inform you

twistor’s picture

twistor commented
Status: Needs work » Fixed

Thanks, turns out things are stored on $node->menu directly, not in $node->menu['link'].

I committed a new fix http://drupalcode.org/project/feeds.git/commit/5fbf8ae.

Let me know if there are anymore issues.

agoradesign’s picture

agoradesign commented

Good morning,
I've tried your fix with my imports -> no more problems :)

dman’s picture

dman commented

Sorry, my mistake on the proposed fix.
The code above was written freehand, not a tested patch :-/

Status: Fixed » Closed (fixed)

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