I really like the concept and ease-of-use of this module. However, I have found two major bugs during my testing. Here's my setup. I have a node type Team, and Teams are parts of a node type called Group. So, a Team can have one Group, and a Group has many Teams. Two node reference fields are setup to accomplish this. Here are my bugs:

1) Duplicates

When saving my Team form the first time, this module correctly sets up the back reference in the Group. However, if I edit and save the Team again with the same Group value, a duplicate back reference is created in the Group node. An additional duplicate is created with each edit. The expected behavior is that there would be no duplicate.

(This is similar to the behavior outlined in #589094: duplicates in node view, however, no change of behavior was noticed from simply clearing the caches.)

2) Dangerously mysterious deletion of all CCK data!

I then opened up my Group node and removed all references to my Team, and saved. Much to my surprise, when I visited my Team node, all of my CCK content was completely missing! The node was still there, but all of the CCK fields were empty (even those completely unrelated to the node reference field). Looking in the database, the Team record exists in the nodes and node_revisions tables, but is completely missing in the content_type_team table.

Note: this only happens when I delete all references to that Team from the Group node, even if there is only one (no duplicates). If I have duplicate Team references in my Group node, and I remove only the duplicates, this doesn't happen. And thankfully, when I assign my Team to another Group, nothing gets deleted from the content_type_group table.

How can we get these two bugs fixed immediately? This bug is especially troublesome because the CCK data gets deleted in a very quiet way, and users may be experiencing this right now without even noticing it.

CommentFileSizeAuthor
#14 backreference.module.patch6.77 KBjoelstein
#13 backreference.module.patch6.73 KBjoelstein
#3 backreference.module.patch4.81 KBjoelstein
#2 backreference.module.patch2.94 KBjoelstein
#1 backreference.module.patch1.1 KBjoelstein

Comments

joelstein’s picture

joelstein commented
StatusFileSize
new backreference.module.patch1.1 KB

I am submitting a patch which fixes the duplicate error. I don't know how to run simple tests, so please let me know if this passes.

For the deletion error, I have discovered what is causing this bug (and it is more harmful that I originally thought). It seems the function _backreference_old_values_remove() assumes that the right field is a multiple field, instead of handling single-instance fields differently than multiple fields. Accordingly, it also assumes that the right table from which the deletions will occur is a "content_field_" joins table. However, in my case the right field is a single-instance field, which means that the right table is the actual content_type_teams table, and ALL records from the content_type_teams table who are in that Group get deleted. Major (stealthy) problem!

So, the best approach is to determine if we are removing old values from a single-instance field or a multiple field. For multiple fields, which are stored in "content_field_" joins tables, we delete the records. For single-instance fields, which are stored in the "content_type_" tables, we instead update the field to NULL.

I'll take a stab at a patch for this soon.

joelstein’s picture

joelstein commented
Status: Active » Needs review
StatusFileSize
new backreference.module.patch2.94 KB

Here is a patch which solves both issues.

joelstein’s picture

joelstein commented
StatusFileSize
new backreference.module.patch4.81 KB

Here is an updated patch, which handles the duplicate checking more intelligently. If the left field is single-instance, it will check if a reference already exists in the right table. If so, it will return without adding the (duplicate) reference.

bohz’s picture

bohz commented

I can confirm that both errors happen (I am glad I were in a testing environment!) and that they were, so far, solved by the patch in #3.
Beside this, the patched module seems to work seamlessly.
I have tested this with
TYPE-A (single value ref) <-> TYPE B (multiple value ref)
TYPE-A (single value ref) <-> TYPE B (single value ref)
TYPE-A (multiple value ref) <-> TYPE B (multiple value ref)
both ways (entering data from A and from B).

This patch seems to solve also #694544: Invalid argument supplied for foreach() that appeared when adding single value refs from the multiple values counterpart of the referred type or, less consistently, when removing a duplicate from a multi value reference.

Two minor annoyances still remain:
A) in multiple fields, it is still possible to (purposely or mistakenly) add the same reference multiple times.
But I guess this is more an issue of the nodereference dropdown and autocomplete widgets, which do not check if any of the referenceable values has been already entered.
B)when deleting the above cited duplicates, the duplicate links in the referred node persist until the latter is re-saved.

Finally (this happened with the unpatched version too), if one of the nodes in a simmetry is deleted, the referencing node holds the [NID] value in the field until caches are cleared; in such conditions it is not possible to delete or update this node without cleaning the noderef field.

I really hope others would test this patch so that it could be commited as soon as possible.

Thank you.

jordanmagnuson’s picture

jordanmagnuson commented

Thanks for the patches Joel!

Flying Drupalist’s picture

Flying Drupalist commented

Thanks for the patch as well.

@bohz I think the duplicate references is a feature not a bug. I for one need this feature.

bohz’s picture

bohz commented

@Flying Drupalist: I see your point. I too noticed dupes may be useful :)

Is that too early to mark this as RTBC?

Cheers

logii’s picture

logii commented

I have a corporate site which has 1000+ webmasters. All of them can edit type A and references to B.
Eventually there will be like 2000-3000 type B, with type A referenced.
The problem arises when a particular node of Type A gets popular, with 500+ back-references due to the duplicates.

Everything works fine. But when we hit the edit button for that particular Type A node(with 500+ back references), i get this error: Fatal error: Maximum execution time of 30 seconds exceeded in /includes/bootstrap.inc on line 777

How the 500+ back-references came about: the number of distinct referenced node is only less then 50, but due to the editing of the referenced node, the number of back-references got bloated to 500+

From this case, I'm pretty sure it's a bug.

logii’s picture

logii commented
Status: Needs review » Active

@joelstein I'm going to give a shot on your patch, but does it solve the above mentioned problem by removing the duplicated references?

mrfelton’s picture

mrfelton commented
Status: Active » Needs work
Flying Drupalist’s picture

Flying Drupalist commented

I have some php errors with this module, can't post them now, I uninstalled the modules and lost the error. But perhaps someone can take a look in their logs and post the errors.

joelstein’s picture

joelstein commented

It's been awhile since I posted this patch, but I'll take a look at it soon and try to answer your questions.

joelstein’s picture

joelstein commented
Status: Needs work » Needs review
StatusFileSize
new backreference.module.patch6.73 KB

@bohz: Thanks for your comments in #4. Node reference allows you to save duplicates manually, and some people need that (as Flying Drupalist pointed out in #5). This patch simply prevents BackReference from creating those duplicates automatically. In #7, that's basically what you said, but I wanted this point to be clear to anyone else reading this thread.

when deleting the above cited duplicates, the duplicate links in the referred node persist until the latter is re-saved.

Finally (this happened with the unpatched version too), if one of the nodes in a simmetry is deleted, the referencing node holds the [NID] value in the field until caches are cleared; in such conditions it is not possible to delete or update this node without cleaning the noderef field.

I believe these are one and the same issue. Looking at the code, it appears that the module developer had a place for dealing with node deletions, but never finished it. I have updated the patch so that when a node is deleted, all the references back to it will be deleted from other nodes (and the caches cleared for those nodes). I've tested it with various setups, but I would appreciate your testing as well.

@logii: The behavior you described sounds like the same problem which opened this ticket. Please apply the patch below and see if duplicates are still being saved. If not, but you are still having issues loading the page, then I recommend you look elsewhere for a solution, since this is outside the scope of this issue.

@Flying Drupalist: If you find the errors, and they are associated with this patch, please post them. I'm not seeing any errors related to the issues contained in this ticket.

I'm marking this as "needs review". Everyone please give it a try, so we can finally get this issue resolved. Thanks!

PS. Where is the maintainer? These are critical bugs, and he hasn't weighed in since the ticket opened almost 5 months ago.

joelstein’s picture

joelstein commented
StatusFileSize
new backreference.module.patch6.77 KB

Updated slightly.

joelstein’s picture

joelstein commented
Assigned: Unassigned » joelstein
Status: Needs review » Fixed

Thanks to John (jcfiala), I've been made a maintainer on the module and issued a beta release which fixes the three issues covered in this ticket. Tests are included. Please try it out and see if it accomplishes these three tasks:

1) No more duplicates automatically added after saving a referenced node.
2) No more mysterious deletion of CCK data under certain field configurations.
3) No more leftover references after a node is deleted.

Status: Fixed » Closed (fixed)

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