Multiple "content_for" calls should concatenate

[Fjan]

Currently, when you call content_for twice on a page the second call will overwrite the content captured for the first one. It would be more useful if they concatenated, just like the behaviour of content_for in Rails. Consider this example that would be made possible:

<% content_for :head do %>
   <link ref="...">
<% end %>

And somewhere else you could do:

<% content_for :head do %>
   <script src="...">
<% end %>

And then put a single <%= content_for :head %> in the layout.

It would actually be a very simple change in nanoc/helpers/capturing.rb. Just replace line 45 with this:

if @store[item.identifier][name] then
  @store[item.identifier][name] << content
else 
  @store[item.identifier][name] = content
end

[denisdefreyne]

This wouldn’t work out of the box without breaking backwards compatibility, but it could work with a param like :append:

<% content_for :head, append: true do %>
   <script src="...">
<% end %>

vs

<% content_for :head, append: false do %>
   <script src="...">
<% end %>

vs

<% content_for :head do %>
   <script src="...">
<% end %>

(default: replace rather than append)

[Fjan]

Do you think anybody uses the current behaviour of calling content_for twice with the same symbol, which would erase the contents of first call? I find it hard to imagine a use case for that. I think most people simply call it once so the issue never arises, so a warning of the changed behaviour in the upgrade notes would suffice. Of course, if I'm overlooking a way in which the current behaviour might be useful, then adding it as an option would be better.

[denisdefreyne]

It’s not possible to change the current behavior without breaking backwards compatibility, and since Nanoc 4 is so close to a final release, this behavior is not going to change before Nanoc 5.0.

[denisdefreyne]

FYI, I’d certainly welcome a PR that adds the new functionality (in a backwards-compatible way); it’ll end up in Nanoc 4.1 if you do so!

[Fjan]

Ok, I don't mind making a PR, but I really don't like adding a parameter for an option for which no one can think of a use case. I'm fine with using the 1-line monkey patch until the behaviour can change in 5.0

[denisdefreyne]

I’m OK with a change in behavior in 5.0!

[Fjan]

I would say the current default behaviour is broken so we should remove it. Overwriting content is unexpected and especially confusing because the Rails call of the same name behaves differently. But if there is anyone who can think of a use case for the current behaviour I'm interested to hear it.

[gpakosz]

I don't know Rails but well thinking about it I would say I doubt people use the current content_for helper twice per page :)

Since it's a helper and not a core functionality I would say it could be viewed as a bug that should be fixed without waiting for 5.0?

[denisdefreyne]

This behaviour has been around since Nanoc 2.x, and it’s intentional, so calling it a bug and changing the behaviour won’t work.

What could be considered a bug is that the current behaviour silently overwrites previous content. I’m OK with a change that makes it raise an exception rather than overwrite content.

[Fjan]

Do you mean you want to change silent overwriting to raise an exception now, and then change it in a future version to concatenate?

[denisdefreyne]

Yes!

[barraq]

👍

[gpakosz]

So until Nanoc 5.0, we're heading towards an :append additional parameter?

[Fjan]

For now, I've just added addressed the silent overwriting behaviour in the pull request. Adding an additional parameter temporarily did not seem worth the trouble.

[denisdefreyne]

See #744 for an implementation of #content_for where the behavior is customisable (error, append, overwrite).