fragment_for assumes ActionView::OutputBuffer.

[cmeiklejohn]

Handle for cases where the buffer is a SafeBuffer, or a particular subclass of SafeBuffer.

If this looks good, I'll make a patch for the other release branches.

Thanks!

[josevalim]

I believe removing the if branch completely leaving just the else branch would work fine.

[cmeiklejohn]

That doesn't work, as it attempts to concat a safe and a non-safe buffer together.

[josevalim]

Could you please provide a test with such changes? I think since the beginning it had no tests. So unless we are testing it, we will be shooting in the dark. Tks!

[cmeiklejohn]

Jose,

I just made the syntax a bit more concise. The original changes were implemented to address failures in ./actionpack/test/controller/caching_test.rb. I think the coverage there is already comprehensive and this change serves just to clean up the syntax.

Let me know if there is some specific test case you'd like to see tested.

[josevalim]

Ah ok, thanks for the explanation. But there is still an issue with your code. Being a SafeBuffer no longer means the string is html safe. You should probably check it using html_safe? .

[cmeiklejohn]

The code in place now currents assumes the data is safe, so I don't believe that is a new change.

Is it safe to assume the data is HTML safe? It's slicing out the content-to-cache from the output_buffer, which is resulting in the output_buffer being marked as dirty, and then it's being concat'd back with the fragment, so would it be safe to assume that the only way non-safe content would be rendered would be if it was read in from the cache?

[josevalim]

Exactly, the current code assumes the output buffer is always safe, which is true for 90%, but for the other 10%, we may potentially mark some dirty content as safe. We should probably check for html_safe? before slicing the buffer. And when it comes to the fragment, the dirty state is preserved on cached, so I assume we don't really need to change anything.

[cmeiklejohn]

Good point, latest commit stores the safe status of the buffer, and only marks safe if it was safe to begin with.

[josevalim]

Tks! The if clause could simply be if output_safe, no?

[cmeiklejohn]

Good call.

Should I cherry pick this to 3-0-stable and 3-1-stable?

[josevalim]

cherry picking + squashing would be great.

[cmeiklejohn]

Created pulls #1791 and #1792.