FIX: phpbb import - attachments not embedded in posts (PR #14570)

Problem

The phpBB import script successfully recognizes and imports the attachments to Discourse but fails to embed them correctly in the posts’ Markdown.

The issue is in TextProcessor::process_attachments which is responsible for embedding the attachment Markdown code to the post text.

# This replaces existing [attachment] BBCodes with the corresponding HTML tags for Discourse.
# All attachments that haven't been referenced in the text are appended to the end of the text.
def process_attachments(text, attachments)
  attachment_regexp = /\[attachment=([\d])+\]<!-- [\w]+ -->([^<]+)<!-- [\w]+ -->\[\/attachment\]?/i
  unreferenced_attachments = attachments.dup

  text = text.gsub(attachment_regexp) do
    index = $1.to_i
    real_filename = $2
    unreferenced_attachments[index] = nil
    attachments.fetch(index, real_filename)
  end

  add_unreferenced_attachments(text, unreferenced_attachments)
end

The faulty line is text = text.gsub(attachment_regexp) do since it causes the text reference to change only inside process_attachments (see How arguments are passed? in Ruby docs) and the return value even though it is correct, is not used by the caller.

Fix

text = text.gsub(...) is changed to text.gsub!(...)

Notes

I wasn’t sure where to add tests. Any suggestions would be welcome. :slight_smile:

GitHub

This pull request has been mentioned on Discourse Meta. There might be relevant details there:

There’s no need for gsub! to fix this issue. The correct solution is to change line 61 to use the return value. Otherwise the changes in add_unreferenced_attachments would be ignored.

text = process_attachments(text, attachments) if attachments.present?

Do you want to update your PR?

I wasn’t sure where to add tests. Any suggestions would be welcome. :slight_smile:

There aren’t any tests for it, so that’s fine. :wink:

Otherwise the changes in add_unreferenced_attachments would be ignored.

Actually, it happens that add_unreferenced_attachments works properly since << modifies text in-place.

I chose gsub! for consistency with the other methods (clean_bbcodes, process_smilies, etc), otherwise maybe they should all be refactored to use return values?

Let me know and I’ll update the PR :smile:

Ah, right! I should have looked more closely. No need to refactor everything ATM. There are changes coming…