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
base: master
from

Conversation

Projects
None yet
3 participants
@ujifgc
Member

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

This comment has been minimized.

Show comment
Hide comment
@ujifgc

ujifgc Mar 25, 2013

Member

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

Member

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

This comment has been minimized.

Show comment
Hide comment
@hooopo

hooopo Mar 25, 2013

Contributor

👍 The same issue with mine.

Contributor

hooopo commented Mar 25, 2013

👍 The same issue with mine.

@ujifgc

This comment has been minimized.

Show comment
Hide comment
@ujifgc

ujifgc Mar 25, 2013

Member

I'm fixing it now. It almost works.

Member

ujifgc commented Mar 25, 2013

I'm fixing it now. It almost works.

@hooopo

This comment has been minimized.

Show comment
Hide comment
@hooopo

hooopo Mar 25, 2013

Contributor

Yeah, thanks a lot! 💯

Contributor

hooopo commented Mar 25, 2013

Yeah, thanks a lot! 💯

@ujifgc

This comment has been minimized.

Show comment
Hide comment
@ujifgc

ujifgc Mar 25, 2013

Member

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.

Member

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

This comment has been minimized.

Show comment
Hide comment
@nesquena

nesquena Mar 26, 2013

Member

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

Member

nesquena commented Mar 26, 2013

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 ujifgc:double-capture branch Mar 26, 2013

WaYdotNET added a commit to WaYdotNET/padrino-framework that referenced this pull request Mar 26, 2013

Merge remote-tracking branch 'upstream/master' into all-custom-error
* upstream/master:
  have plugin generator respect root option
  Fix very strange bug with form_for and capture_html @ujifgc #1177
  [padrino-core/rake] Adds lib as a load_path for rake tasks #1185
  [form_helpers] Use count instead of size (Closes #1184)
  [format_helpers] Mark escaped text as html_safe (Closes #1183)
  Closes #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