HTML::Document fails to infer closing tag when final node has no children #7896

Closed
betesh opened this Issue Oct 10, 2012 · 8 comments

Projects

None yet

4 participants

Contributor
betesh commented Oct 10, 2012
HTML::Document.new("<body><br />").root.to_s =>  "<body><br /></body>"

Note that in the above example, the closing tag is correctly inferred. To be consistent, HTML::Document should behave with:

HTML::Document.new("<body>").root.to_s =>  "<body></body>"

However, the actual output is:

<body>

This is the root cause of the fact that testing failed to catch #7894.

Member
arunagw commented Oct 10, 2012

what's this about? Can you add description to this so people can understand what you are talking about?

Contributor
ayrton commented Oct 10, 2012

After investigating the issue included with this PR, I saw that <%= form_tag "" %> returns invalid html because it does not close the form-tag with </form>.

<%= form_tag '/posts' %> for instance does this correct.

I can't see why anyone would want to use the form_tag with an empty string though, wouldn't it be better to enforce people passing a block when using this helper?

Contributor
betesh commented Oct 10, 2012

Ayrton,

  1. I came across the bug in #7894 because I was trying to use form_tag without a block, which is not the same as without an action (i.e. '/posts').
  2. It's not for you to judge "why anyone would want" to do this--there are legitimate uses, and enforcing that people should only be able to use form_tag in the ways that an individual programmer was able to think up is not a very open-source attitude. Why should I be limited to generating the form element's children on the server when sometimes it's more practical to use Javascript?
  3. Your comments are not pertinent to this issue and should have been posted on #7894. If you'd like to begin a discussion there, I'd be happy to explain to you why form_tag should not require a block or an action.
Member

Closing this in favor of #7894

@vijaydev vijaydev closed this Oct 10, 2012
Contributor
betesh commented Oct 10, 2012

vijaydev, could you clarify what "closing in favor of" means? It sounds to me like that means you're not going to pull this fix. #7894 is a distinct issue. Fixing it doesn't fix this issue, though fixing this issue makes it possible to test #7894.

Member

I mean, let's have one issue/PR where the whole thing is sorted out. You've anyway made a single PR which apparently fixes both issues.

Contributor
betesh commented Oct 10, 2012

Yes, since #7896 blocked testing of #7894 but could be fixed in isolation,
I thought it made sense to wrap them in a single PR but two separate
commits. Was that the right move?

Member

that's fine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment