[Bugfix] Use the specified capture_generator even for nested captures

[jirutka]

We may get a capture nested inside another capture. For example Slim generates such Sexp for:

.foo class=@class

In this case three generators are initialized: the main generator G0, the capture_generator G1 created by G0 in #on_capture, the capture_generator G2 created by G1 in #on_capture.

The problem is that Generator#on_capture does not pass option :capture_generator into the generator being created. Thus G1 always uses the default capture_generator (StringBuffer), not the one originally specified by the option :capture_generator.

[jirutka]

@judofyr, it’s almost 3 years since I opened this PR and still no response…

[jirutka]

The build failed only on Ruby 1.9.3 which is after its end of life for a very long time.

[timriley]

👋🏼 Hi folks (and cc @minad, as the author of the commit that rolled this PR back) —

I've just hit this issue myself. I'm working on changes in hanami-view that mark captured template content (made via Temple's :capture sexps) as html_safe.

I've done so via creating a custom capture generator:

class HTMLSafeStringBuffer < Temple::Generators::StringBuffer
  def return_buffer
    byebug
    "#{buffer}.html_safe"
  end
end

And then specifying it as the default value for the :capture_generator option on my engine:

module Hanami
  class View
    module ERB
      require_relative "parser"
      require_relative "filters/block"
      require_relative "filters/trimming"

      # Hanami::View ERB engine.
      #
      # @api private
      # @since 2.0.0
      class Engine < Temple::Engine
        define_options capture_generator: Hanami::View::HTMLSafeStringBuffer

However, when I create nested captures, e.g.

<%= some_capture_method do %>
  <div>Outer content</div>
  <%= some_capture_method do %>
    <div>Inner content</div>
  <% end %>
<% end %>

Then in this case, the HTMLSafeStringBuffer is used as the capture generator only on the outer capture, not the inner capture.

This seems to be an identical scenario to what @jirutka described in the top-level description for this PR.

Is there a way we can solve this in Temple while still satisfying whatever requirements led to your rollback commit, @minad?

And in the meantime, @jirutka, I'm wondering if you might have something you can share about how you possibly worked around this issue in Slim or elsewhere?

Thank you!

[timriley]

To make my use case clearer, I've extracted it to a dedicated PR: hanami/view#230

This PR has three commits:

1. ✅ The first commit introduces a custom :capture_generator to serve our specific needs for hanami-view. It includes tests to cover one level of depth for block capturing in templates. Test passes.
2. 🔴 The second commit adds another test to cover deeply nested block capturing in templates. The inner block is not using the custom :capture_generator that we've specified on our Temple engine. Test fails.
3. ✅ The final commit locally patches Temple to match the change in this PR. This ensures the configured :capture_generator is used for all levels of nested block capturing. Test passes again!

So of the three changes that @minad rolled back in 181ea35, the only one that I'd love to see reintroduced (at least to cover our needs in hanami-view!) would be the change from this PR.

@minad — in your commit message you seem primarily concerned in making sure the main generator is separated from the capture generator. I can definitely understand how mixing the two could introduce regressions, but the specific change from this PR doesn't seem to mix the generators at all, instead just ensuring that options are appropriately propagated to newly created capture generators. So my thinking is that this PR is still safe to apply, and it possibly didn't need to be included in the reverts you made before?

I'd be happy to open another PR with the same diff as this one, if that would help! Thanks for checking this out ❤️

[minad]

@timriley The rolled back commit introduced a regression in Slim. It wasn't only a concern. See slim-template/slim#898.

[timriley]

Thanks for following this up, @minad!

The reason I'm following up here is because you rolled back three commits:

• Fix the default value of :capture_generator (8e5c7a2)
• Use the specified capture_generator even for nested captures ([Bugfix] Use the specified capture_generator even for nested captures #112, a98bc35)
• Change default :capture_generator to self (b0f1f6b)

The first commit and the last commit there are about setting the value of capture_generator, while the middle commit (which is what I'm discussion here) is simply about propagating options when capture_generator is used.

So the middle commit is capture-related, yes, but it seems like it might not have been the particular source of the bug in slim, since it doesn't change the value of the capture_generator at all.

So I think it makes complete sense to keep those other 2 commits reverted, I'm just following up about this middle one. It feels safe and IMHO it does fix a legitimate bug (see e.g. my PR). Does that clarify my position a little better?

I'll see if I can manually test this change against slim, but I thought it'd be worth trying to follow up here in writing first 😄

[minad]

@timriley I have to look in detail at this. The problem were nested captures in Rails, so it matters if something is propagated. I would need a small regression test which demonstrates slim-template/slim#898 and then check these commits again for their exact effect. Capturing is the most problematic part of Temple because of the different handling in Rails/non-Rails.

[timriley]

@minad No worries at all 😄 Let me help by sharing a reproduction!

Check out this Rails app: timriley/rails_slim_temple_nested_blocks_demo.

Brief instructions for replicating the issue and confirming it is not reintroduced by this PR are in the README, but let me run you through things step by step here too.

Firstly, I started by replicating the conditions of slim-template/slim#898 as closely as possible: Rails 7.0.4, plus temple 0.9.1 and slim-template/slim's main branch at ref 45a8087. Together, these were all current as of the time of that issue being posted.

When we use these temple and slim versions, we can witness the same bug as reported:

❯ curl -s http://localhost:3000/posts| grep microcopy
   <divtestyes<divtestyes <divtestyes class="<divtestyes<divtestyes <divtestyes class="">awesome microcopy</div>

From here, we can switch to today’s latest versions of temple (0.10.0) and slim (5.1.0) and confirm the bug is indeed gone:

❯ curl -s http://localhost:3000/posts| grep microcopy
    <div class="test yes">awesome microcopy</div>

And then we can use my propagate-options-to-capture-generator branch on top of the latest temple (which is the very same code change as this PR here) and see that the bug is still absent:

❯ curl -s http://localhost:3000/posts| grep microcopy
    <div class="test yes">awesome microcopy</div>

---

To look into what exactly was going on, I went back to the versions of slim/temple that exhibited the bug, and then adjusted Temple::Generator#on_multi to print out the Ruby code it was generating.

Just as a reminder, here's the entire slim template:

.test class="#{class_names(yes: true)}" = "awesome microcopy"

And here's the generated code that exhibits the bug:

_temple_html_attributemerger1 = []; _temple_html_attributemerger1[0] = output_buffer || ActionView::OutputBuffer.new; _temple_html_attributemerger1[0].safe_concat(("test".freeze)); _temple_html_attributemerger1[0]; _temple_html_attributemerger1[1] = output_buffer || ActionView::OutputBuffer.new; _temple_html_attributemerger1[1].safe_concat(((::Temple::Utils.escape_html_safe((class_names(yes: true)))).to_s)); _temple_html_attributemerger1[1]; _temple_html_attributeremover1.safe_concat(((_temple_html_attributemerger1.reject(&:empty?).join(" ")).to_s))
@output_buffer.safe_concat(("<div".freeze)); _temple_html_attributeremover1 = output_buffer || ActionView::OutputBuffer.new; _temple_html_attributemerger1 = []; _temple_html_attributemerger1[0] = output_buffer || ActionView::OutputBuffer.new; _temple_html_attributemerger1[0].safe_concat(("test".freeze)); _temple_html_attributemerger1[0]; _temple_html_attributemerger1[1] = output_buffer || ActionView::OutputBuffer.new; _temple_html_attributemerger1[1].safe_concat(((::Temple::Utils.escape_html_safe((class_names(yes: true)))).to_s)); _temple_html_attributemerger1[1]; _temple_html_attributeremover1.safe_concat(((_temple_html_attributemerger1.reject(&:empty?).join(" ")).to_s)); _temple_html_attributeremover1; if !_temple_html_attributeremover1.empty?; @output_buffer.safe_concat((" class=\"".freeze)); @output_buffer.safe_concat(((_temple_html_attributeremover1).to_s)); @output_buffer.safe_concat(("\"".freeze)); end; @output_buffer.safe_concat((">".freeze)); @output_buffer.safe_concat(((::Temple::Utils.escape_html_safe(("awesome microcopy"))).to_s));
; @output_buffer.safe_concat(("</div>".freeze))

Then I want to use my branch of temple and the latest slim again, and made the same #on_multi adjustment, and got this:

_temple_html_attributemerger1 = []; _temple_html_attributemerger1[0] = ActionView::OutputBuffer.new; _temple_html_attributemerger1[0].safe_concat(("test".freeze)); _temple_html_attributemerger1[0]; _temple_html_attributemerger1[1] = ActionView::OutputBuffer.new; _temple_html_attributemerger1[1].safe_concat(((::Temple::Utils.escape_html_safe((class_names(yes: true)))).to_s)); _temple_html_attributemerger1[1]; _temple_html_attributeremover1.safe_concat(((_temple_html_attributemerger1.reject(&:empty?).join(" ")).to_s))
@output_buffer.safe_concat(("<div".freeze)); _temple_html_attributeremover1 = ActionView::OutputBuffer.new; _temple_html_attributemerger1 = []; _temple_html_attributemerger1[0] = ActionView::OutputBuffer.new; _temple_html_attributemerger1[0].safe_concat(("test".freeze)); _temple_html_attributemerger1[0]; _temple_html_attributemerger1[1] = ActionView::OutputBuffer.new; _temple_html_attributemerger1[1].safe_concat(((::Temple::Utils.escape_html_safe((class_names(yes: true)))).to_s)); _temple_html_attributemerger1[1]; _temple_html_attributeremover1.safe_concat(((_temple_html_attributemerger1.reject(&:empty?).join(" ")).to_s)); _temple_html_attributeremover1; if !_temple_html_attributeremover1.empty?; @output_buffer.safe_concat((" class=\"".freeze)); @output_buffer.safe_concat(((_temple_html_attributeremover1).to_s)); @output_buffer.safe_concat(("\"".freeze)); end; @output_buffer.safe_concat((">".freeze)); @output_buffer.safe_concat(((::Temple::Utils.escape_html_safe(("awesome microcopy"))).to_s));
; @output_buffer.safe_concat(("</div>".freeze))

The cause of the bug is evident even towards the beginning of the first line:

_temple_html_attributemerger1 = []; _temple_html_attributemerger1[0] = output_buffer || ActionView::OutputBuffer.new;

See here how it's trying to use output_buffer || ActionView::OutputBuffer.new for the captured content? This is because of the other two changes that you rolled back, which change the default capture_generator to be self (b0f1f6b and 8e5c7a2). In the case of slim being used within Rails, self here would be Temple::Generators::RailsOutputBuffer. When this is used for every capture, what it'll mean is that the captured content is pushed onto the already existing output buffer, not a new buffer just for that capture, which is why we're seeing all the repeated content.

Now when we compare this to the equivalent part from the template code generated using latest slim and my branch of temple, we see a much more sensible arrangement:

_temple_html_attributemerger1 = []; _temple_html_attributemerger1[0] = ActionView::OutputBuffer.new;

Here we're creating a dedicated new buffer just for the purposes of this particular capture. Hence no repeated content.

Why are we dealing with captures for such a simple slim template, you might ask? It's because slim uses captures for generating HTML tag attributes from Ruby code. To confirm this, I hooked into the Slim::CodeAttributes#on_html_attr and printed out the sexp it was compiling:

[[:html,
  :attr,
  "class",
  [:multi,
   [:code, "_temple_html_attributemerger1 = []"],
   [:capture, "_temple_html_attributemerger1[0]", [:static, "test"]],
   [:capture, "_temple_html_attributemerger1[1]", [:escape, true, [:multi, [:multi, [:escape, true, [:dynamic, "class_names(yes: true)"]], [:multi]]]]],
   [:dynamic, "_temple_html_attributemerger1.reject(&:empty?).join(\" \")"]]]]

So it's using captures for both static- and dynamically-assigned attributes.

---

Ultimately, I hope this exploration here clearly demonstrates that the bug was specifically due to b0f1f6b and 8e5c7a2, which were focused on assigning the default capture_generator to be the same as the top-level generator. Now that we've seen the consequences of that, I think it's clear that this was an ill-advised change from the beginning 😅

On the plus side, I hope this exploration also shows that this changes from PR are safe to reintroduce, because it introduces no such bug within Rails/Slim, and fixes what is IMHO a legitimate bug within Temple. Without this change, configuring a capture_generator is only good for a single capture only, with any nested captures not being appropriately configured. Fixing this bug reinforces the core idea of temple, which is that you can construct your sexps as arbitrarily deeply as you need for your use case 😄

I know you asked for a regression test here, @minad, but I'm not sure if there's much value in adding one that covers the particular combination of issues across Temple/Slim/Rails that we've explored above.

At very least, I've confirmed that the test introduced in this PR does indeed fail without the corresponding code change, so at least this test confirms the "happy path" in a succinct way that is also fitting with the range of existing tests here in Temple.

---

I hope this investigation and reproduction help with your decision making here! To make it easy to move forward, I'm going to open another PR with these same changes, just for the ease of you clicking the "merge" button if you so desire 😇

[minad]

Thanks for your investigation and putting this together. This is greatly appreciated. Still, I have to insist on a regression test or additional tests, which reduce the risk that problematic changes regarding capturing make it into Temple or Slim in the future. The capturing code is clearly not tested well enough. I think we need at least a small regression test for the special scenario from slim-template/slim#898 in Slim (it should pass before and after adding the option propagation) and then additional tests which check the modified behavior of the capture generator in Temple (it should fail before the option propagation, and pass afterwards).

[timriley]

And here's the PR: #144

[timriley]

Thanks for the update, @minad. Glad to hear I'm at least on the right path.

Adding the regression test into Slim itself makes a lot of sense. I can look into doing that.

As for the additional tests within Temple, the test in #144 already satisfies what you suggested (fails before the change, passes after):

  it 'should compile nested capture with the same capture_generator' do
    gen = SimpleGenerator.new(buffer: "VAR", capture_generator: SimpleGenerator)
    expect(gen.call([:capture, "foo", [:multi,
      [:capture, "bar", [:multi,
        [:static, "a"],
        [:static, "b"]]]]
    ])).to eq "VAR = BUFFER; foo = BUFFER; bar = BUFFER; bar << (S:a); bar << (S:b); bar; foo; VAR"
  end

The way this test fails without the corresponding change is like so:

       expected: "VAR = BUFFER; foo = BUFFER; bar = BUFFER; bar << (S:a); bar << (S:b); bar; foo; VAR"
            got: "VAR = BUFFER; foo = BUFFER; bar = ''; bar << (\"a\".freeze); bar << (\"b\".freeze); bar; foo; VAR"

Given you mention "additional tests", what else would you like to see?

I could possibly update this one test to exercise three levels of capture depth, just to reinforce the idea that this works to arbitrary depth. I could also possibly make it use a dedicated SimpleCaptureGenerator test class which prints CAPTURE_BUFFER instead of BUFFER, just to make it clear that it's different from the top-level generator.

If you could share a little bit of guidance on this front, I'd really appreciate it, just to make sure that I'm working in a way that you'll find satisfactory.

[minad]

Adding the regression test into Slim itself makes a lot of sense. I can look into doing that.

Okay, great.

Given you mention "additional tests", what else would you like to see? I could potentially update this one to exercise three levels of capture depth, just to reinforce the idea that this works to arbitrary depth. I would possibly also make it use a dedicated SimpleCaptureGenerator test class which prints CAPTURE_BUFFER instead of BUFFER, just to make it clear that it's different from the top-level generator.

Sounds good. 👍

[timriley]

@minad I've just updated #144 with the changes to the test we discussed, and opened slim-template/slim#917 with the regression test for Slim.

Let me know if there's anything more you'd like to see!