Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Correct fix for Slim #1431, #1434, #1435 #1437

Merged
merged 1 commit into from Oct 9, 2013
Merged

Correct fix for Slim #1431, #1434, #1435 #1437

merged 1 commit into from Oct 9, 2013

Conversation

minad
Copy link
Contributor

@minad minad commented Oct 8, 2013

This is the correct way how to fix the issues #1431, #1434, #1435. Since you want to use a capture helper you have to disable the internal Slim capturing. This is how it is also done in Rails. I don't recommend to monkey patch the Slim::Template how you did in #1435.

Unfortunately your test cases somehow don't cover the capturing very well and the problems described in #1431, #1434, #1435. You should definitely improve that.

@minad
Copy link
Contributor Author

minad commented Oct 8, 2013

What I don't get is why you replace the output_buffer in the erb and slim handler, but you don't replace the @_out_buf variable in capture_from_template. So why is this working after all?

@DAddYE
Copy link
Member

DAddYE commented Oct 8, 2013

Looks great to me, can't we improve ours tests?

@namusyaka
Copy link
Contributor

It's my fault. Thanks!

ujifgc added a commit that referenced this pull request Oct 9, 2013
@ujifgc ujifgc merged commit 28d436f into padrino:master Oct 9, 2013
@nesquena
Copy link
Member

nesquena commented Oct 9, 2013

Thanks @minad!

@namusyaka namusyaka mentioned this pull request Oct 9, 2013
@minad
Copy link
Contributor Author

minad commented Oct 9, 2013

Could you improve your tests for capturing? See what I wrote before about @_out_buf. I think the same applies for the erb handler.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants