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

Fix very strange bug with form_for and capture_html (double render) #1177

Closed
wants to merge 3 commits into from

Conversation

ujifgc
Copy link
Member

@ujifgc ujifgc commented Mar 24, 2013

Here's some code that fails very strangely on padrino edge:

https://github.com/ujifgc/tiny

It's basically a test if a block provided for form_for executed twice:

- form_for :object, '/' do |f|
  - logger << 'CAPTURE'
  = 1

If you visit the app with padrino edge, log shows double CAPTURE.
If you appy my patch, then it's only one CAPTURE.

The bug is very strange and I fail to write any test that would catch it. Also I don't understand how

    form_tag(url, settings) { capture_html(instance, &block) }

differs from

    html = capture_html(instance, &block)
    form_tag(url, settings) { html }

Please help me with test and maybe explanation of the patch effect.

@ujifgc
Copy link
Member Author

ujifgc commented Mar 25, 2013

In the RenderDemo app test the block is rendered 4 times instead of 1. With my patch 2 times.

@hooopo
Copy link
Contributor

hooopo commented Mar 25, 2013

👍 The same issue with mine.

@ujifgc
Copy link
Member Author

ujifgc commented Mar 25, 2013

I'm fixing it now. It almost works.

@hooopo
Copy link
Contributor

hooopo commented Mar 25, 2013

Yeah, thanks a lot! 💯

@ujifgc
Copy link
Member Author

ujifgc commented Mar 25, 2013

Okay, here's the summary:

haml does not suffer from lacking any of these commits (1 capture in tests)

slim suffers from both. It doubles the number of captures in form_for and in capture_html (4 captures)

erb suffers only from form_for problem (2 captures)

I still don't understand why

html = capture_html(instance, &block)
form_tag(url, settings) { html }

saves slim and erb. The only idea I have is some kind of runtime optimization.

@nesquena
Copy link
Member

Merged your changes in a squash commit, thanks for getting this working! I think I can close this now

@nesquena nesquena closed this Mar 26, 2013
hooopo referenced this pull request in robbin/robbin_site Mar 26, 2013
@ujifgc ujifgc deleted the double-capture branch March 26, 2013 06:30
WaYdotNET added a commit to WaYdotNET/padrino-framework that referenced this pull request Mar 26, 2013
* upstream/master:
  have plugin generator respect root option
  Fix very strange bug with form_for and capture_html @ujifgc padrino#1177
  [padrino-core/rake] Adds lib as a load_path for rake tasks padrino#1185
  [form_helpers] Use count instead of size (Closes padrino#1184)
  [format_helpers] Mark escaped text as html_safe (Closes padrino#1183)
  Closes padrino#1179 for accidental appended extensions for js urls
  format activerecord migrate
  formatting activerecord migration test
  Thanks to @kenkeiter for bringing up the fact that http_router [is missing PATCH](https://github.com/joshbuddy/http_router/blob/master/lib/http_router/route.rb#L5).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants