Remove noop `context.merge(post_process: true)` call

Asherah Connorrequested to merge
push-kqnwkqnlxwvq into master

What does this MR do and why?

Remove noop context.merge(post_process: true) call.

This was inadvertently taken out of the execution path in e817748d when the call to AssetProxyFilter.transform_context was added after it. If this context change was ever load bearing -- if it was really important that we set post_process: true in the context here -- we would've been in trouble for the last nine months.

But it isn't! It was introduced in its current form in abbca615 (11 years ago), and even at that stage it didn't actually do anything:

$ git grep post_process abbca615^{tree}
abbca615^{tree}:app/helpers/gitlab_markdown_helper.rb:    Gitlab::Markdown.post_process(html, context)
abbca615^{tree}:lib/gitlab/markdown.rb:    # through `post_process`.
abbca615^{tree}:lib/gitlab/markdown.rb:    # requiring XHTML, such as Atom feeds, need to call `post_process` on the
abbca615^{tree}:lib/gitlab/markdown.rb:    def self.post_process(html, context)
abbca615^{tree}:lib/gitlab/markdown.rb:      pipeline = pipeline_by_type(:post_process)
abbca615^{tree}:lib/gitlab/markdown.rb:    autoload :PostProcessPipeline,          'gitlab/markdown/post_process_pipeline'
abbca615^{tree}:lib/gitlab/markdown/post_process_pipeline.rb:          post_process: true
$

At this point we use post_process in the input context to decide whether to call the post-processing pipeline or not: if we don't, we'll never call PostProcessPipeline.transform_context anyway.

We actually explicitly remove post_process from the context when making this decision (app/helpers/markup_helper.rb:217-219):

def render_markdown_field(object, field, context = {})
  post_process = context.delete(:post_process)
  post_process = true if post_process.nil?

We do this because it's not used by any filter, which is the destination for this context object after this point:

$ rg post_process lib/banzai/filter
$

So the old behaviour was just putting it back, for no real reason other than we were doing it in 2015.

References

I was researching CacheMarkdownField.banzai_render_context forces... (#589496) when I noticed the no-op context.merge, so I dived much deeper to see (a) if we had a security issue that had gone unreported for almost a year (nope!), and then (b) what the point of that was ever meant to be.

How to set up and validate locally

I'm removing one line of code which has no effect on execution whatsoever, so there is quite literally no way to tell the difference between the codebase without this MR and the one with it. There is no change whatsoever, except we don't make one extra Hash object and throw it away.

MR acceptance checklist

Evaluate this MR against the MR acceptance checklist. It helps you analyze changes to reduce risks in quality, performance, reliability, security, and maintainability.

Merge request reports