Currently the parse_message::get_submitted_attachment_data function calls trigger_error('NO_ACCESS_ATTACHMENT', E_USER_ERROR) if any attachment from the POST data is not found in the database. This approach makes it very difficult for users to recover their post content if an attachment happens to be deleted before the final form submission. There appear to be multiple past reports about this problem (PHPBB3-4238, PHPBB3-9048, PHPBB3-9789, PHPBB3-11944, PHPBB3-12914), but they’re all closed, even though the bug still exists.

This bug can be triggered in multiple ways:

1. The board is set up to purge orphaned attachments in some way;
2. A post is being edited simultaneously in two windows, and one of them deletes an attachment;
3. With plupload enabled, an attachment is deleted by XHR and then the form is submitted before the row containing the form fields for that attachment is actually removed from the DOM.

There is no reason for get_submitted_attachment_data to trigger an error, since all the call sites have access to error arrays for sending errors to the user in the normal way. Instead of calling trigger_error, this function should just process all the attachments at once, and then at the end of the function return whether or not all incoming attach_ids were successfully handled. The caller can then add errors which will prevent form submission while giving the user an opportunity to make corrections in the normal way.

Reproduction:

1. Go to posting form
2. Upload an attachment
3. Remove the record for that attachment from the database
4. Submit the form

Expected: A normal inline error that some attachments are not available any more. User can re-upload files or save the rest of their post content without attachments.

Actual: “General Error: You are not allowed to access this file”. Post content is probably lost since the user can’t get back to the form.

Attached is one possible patch. I have no idea why the original code was iterating over non-orphans (which may have included orphans since the query did not actually filter out orphaned files!) separately from orphans since the end result was always the same so this patch also gets rid of that duplicate code.