Bug #441

Deleting a copied thread deletes the attachments of the original thread too

Added by bosit man over 2 years ago. Updated about 2 years ago.

Status:Closed Start date:08/30/2009
Priority:Normal Due date:
Assignee:Ryan Gordon % Done:

100%
Category:Moderation
Target version:1.4.10
Reproducibility:Always Database Type:MySQLi
Reported In MyBB Version:1.4.8 Database Version:
PHP Version:5.2.6 SQA assignments:
Browser:Firefox 3.5.2

Description

Using Mybb v1.4.8

How to replicate:

1) Make a copy of a thread that contains attachment
2) Delete the copy of the thread you made
3) Attachments on original thread got deleted along with the copy

I realize MyBB has no way of knowing if you are deleting the copy or the original thread but this by no means should be expected behavior. What seems to be an (easy?) solution would be a confirmation message upon deleting a thread, if you also want attachments to be deleted too.
Or a more advanced solution would be creating duplicates of the attachments upon copying a thread, so the duplicates get deleted instead of the original thread's attachments.

Associated revisions

Revision 4531
Added by Ryan Gordon about 2 years ago

Fixes Deleting a copied thread deletes the attachments of the original thread too (fixes:441)

Revision 4558
Added by Ryan Gordon about 2 years ago

Fixes Deleting a copied thread deletes the attachments of the original thread too (fixes:441)

Revision 4560
Added by Ryan Gordon about 2 years ago

Fixes Deleting a copied thread deletes the attachments of the original thread too (fixes:441)

History

#1
Updated by Ryan Gordon about 2 years ago

  • Category set to Moderation
  • Status changed from New to Assigned
  • Assignee set to Ryan Gordon
  • Target version set to 1.4.10

What we should do is when we copy a thread with attachments is make a new row for the attachment in the database, correlating to the same file. Then before deleting the actual file from the filesystem, we check the database for any records that still reference it.

#2
Updated by Ryan Gordon about 2 years ago

Ryan Gordon wrote:

What we should do is when we copy a thread with attachments is make a new row for the attachment in the database, correlating to the same file. Then before deleting the actual file from the filesystem, we check the database for any records that still reference it.

Following up on this, when we copy a thread, we already make a new attachment row in the database. All we have to do is the later part during the deletion process

#3
Updated by Ryan Gordon about 2 years ago

  • Status changed from Assigned to Resolved
  • % Done changed from 0 to 100

Applied in changeset r4531.

#4
Updated by Michael Schlechtinger about 2 years ago

  • Status changed from Resolved to Feedback

This fix is not working. The file in the filesystem still gets deleted when removing the copied thread. The reason is that the function remove_attachments() is used which does not contain the fix.

Do we really need remove_attachment() and remove_attachments()? I don't see a reason for having two functions for the same purpose.

#5
Updated by Ryan Gordon about 2 years ago

I didn't add any functions, I just modified one function.

As for the code, are you sure? Look at the code and read it. It's logic is correct.

#6
Updated by Michael Schlechtinger about 2 years ago

Ryan Gordon wrote:

It's logic is correct.

It is. But there are 2 functions in inc/functions_upload.php: remove_attachment() and remove_attachments(). Your fix is in the function remove_attachment() and the function called when deleting a thread is remove_attachments(). Because of that the fix is not working here.

#7
Updated by Ryan Gordon about 2 years ago

  • Status changed from Feedback to Resolved

Applied in changeset r4558.

#8
Updated by Michael Schlechtinger about 2 years ago

  • Status changed from Resolved to Feedback

This is still not working. I think I found the problem. In both functions remove_attachment() and remove_attachments() the database record is deleted before checking for other references:

$db->delete_query("attachments", "aid='{$attachment['aid']}'");
[...]
// Check if this attachment is referenced in any other posts. If it isn't, then we are safe to delete the actual file.
$query = $db->simple_select("attachments", "COUNT(aid) as numreferences", "attachname='".$db->escape_string($attachment['attachname'])."'");
if($db->fetch_field($query, "numreferences") <= 1)

If you change
if($db->fetch_field($query, "numreferences") <= 1)

to
if($db->fetch_field($query, "numreferences") == 0)

everything seems to be fine.

#9
Updated by Ryan Gordon about 2 years ago

Michael Schlechtinger wrote:

This is still not working. I think I found the problem. In both functions remove_attachment() and remove_attachments() the database record is deleted before checking for other references:
[...]
If you change
[...]
to
[...]
everything seems to be fine.

Woops. That's what I intended.

#10
Updated by Ryan Gordon about 2 years ago

  • Status changed from Feedback to Resolved

Applied in changeset r4560.

#11
Updated by Michael Schlechtinger about 2 years ago

  • Status changed from Resolved to Closed

Also available in: Atom PDF