#672732: Wrong 'delta' count are in part a side effect of direct handling of data transactions with CCK-managed fields. I found myself wondering if we could instead use CCK's native handling. Doing so could significantly reduce the potential for issues with the code and thus make it both more stable and easier to maintain and upgrade.

Attached is a proposed refactoring. The basic approach is: after insert, update, and delete operations, we determine what has changed, make the adjustments to referenced nodes, and call content_update() to register the changes. Doing so avoids all direct read/write operations on CCK fields.

Please have a look when you get a chance.

CommentFileSizeAuthor
#10 cnr-730986-10.patch12.96 KBnedjo
#7 cnr_730986.patch13.49 KBdrewish
#6 cnr_730986.patch6.8 KBdrewish
#5 730986-5-cnr-content-update.patch13.67 KBnedjo
cnr_content_update.patch11.96 KBnedjo

Comments

domidc’s picture

domidc commented

Wow! I just tested quickly and all seems to work well. Great refactoring. This solves indeed the delta count problem.
It needs some more testing to make sure it works in all possible cases, especially when using multiple content types referencing to each other and same node types referencing to each other.
I m really short of time as I m approshing a deadline of major project at the end of the month. So I wont be able to do much work on the module.
If you want I ll make you co maintainer of the project?

domidc’s picture

domidc commented
Status: Active » Needs review
ManyNancy’s picture

ManyNancy commented
Status: Needs review » Needs work

I tested the patch.

My use case has the same field on the same node type referencing itself.

Whenever I add a reference, I get 2 references back on the opposing node, rather than the expected one.

But this does fix the delta count problem.

Thanks.

gleroux02’s picture

gleroux02 commented

I tested the patch as well and so far it seems to be working.

In my setup i have a nodereference field on content type 1 that references content type 2.

With a different nodereference field on content type 2 that references content type 1.

There is also the same setup for a content type 3 that references content type 2.

My delta's are not changing and I am not getting any other irregularities. Thanks for the work.

nedjo’s picture

nedjo
he/him/his
Primary language English
commented
Status: Needs work » Needs review
StatusFileSize
new 730986-5-cnr-content-update.patch13.67 KB

Here's a revised patch (not tested) that attempts to fix the but ManyNancy reported. Approach: to avoid duplicates, test to ensure a nodereference doesn't already exist before adding it.

I've also moved the new code to an include file since it's seldom needed--only on node save, update, and delete.

drewish’s picture

drewish commented
StatusFileSize
new cnr_730986.patch6.8 KB

Once I fixed:

+      module_load_include(module_load_include('inc', 'corresponding_node_references', 'corresponding_node_references.crud');

it seemed to work pretty well.

drewish’s picture

drewish commented
StatusFileSize
new cnr_730986.patch13.49 KB

forgot to add the .inc file to my patch.

gleroux02’s picture

gleroux02 commented

I think this patch still has some issues un-related to the delta's changing. I have applied the latest patch to cnr version 6.x-3.2 and have turned on cnr for content type A with nodereference 1 that references content type B with nodereference 2 referencing content type A on content type B. My deltas are not changing but any image that I have uploaded to content type A or content type B is being removed both nodes. This only happens when content type A or content type B either adds a new reference or removes a reference. If the reference is already established and either node is edited this does not happen. I should also note that after I turn off cnr or un-patch the module that this problem goes away.

gleroux02’s picture

gleroux02 commented

I should update this comment, I have just applied the original patch and am noticing this issue as well.

nedjo’s picture

nedjo
he/him/his
Primary language English
commented
StatusFileSize
new cnr-730986-10.patch12.96 KB

@gleroux02: thanks for testing and for the clear bug description.

The bug I believe came from my stripping down the node before passing it to content_update(). I'd done that so that we would be sending only the data needed to update the individual field. However, all fields are updated in content_update(), so if we leave out fields, their existing data is deleted.

I've updated the patch to remove this stripping down and instead pass the complete loaded node, as would regularly be done. I assume this should fix the bug, though I have not tested.

goofrider’s picture

goofrider commented

This patch also seems to fix #704020: Multiple node reference weight changes.

EDIT: It works wonderfully. Please merge this patch into CNR's mainline.

domidc’s picture

domidc commented
Status: Needs review » Fixed

The patch has been tested and reviewed by the community and is included in the 3.3 release

Status: Fixed » Closed (fixed)

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