Rails Ruby on Rails

Comments and changes to this ticket

• You flagged this item as spam.
  

  cake_joe November 1st, 2010 @ 12:00 AM

  • Tag set to “csrf, form_helper, rails3, semantic”

  Afaik a "wrapper" element is required for html validation.

  The code being generated currently is not good though:

  Reasons:

  • Some people do not like tag-soup
  • Inline CSS is bad
  • Its hard to use display: none instead of display: inline

  Quick & Easy fix:

  and in scaffold.css .protect_from_forgery { display: inline; margin: 0; padding: 0; } The css class name could be different as it includes the utf-8 flag as well and thus is not "exactly" matching what it contains. Better fix: 1. Add class="protect_from_forgery" or something similar to all 3 hidden input fields for browser backwards compatibility (IE, other browsers can access the hidden elements style by going by css selector [type="hidden"]. 2. Prepend the the 3 hidden fields to - either: first generation of a rails3 input/textarea/select - or: the first generation of a rails3 submit/button

• You flagged this item as spam.
  

  cake_joe November 1st, 2010 @ 12:07 AM

  Sorry for 2 comments. Messed up the formatting.

  Afaik a "wrapper" element is required for html validation.

  The code being generated currently is not good though:

  <div style="margin:0;padding:0;display:inline"><input name="utf8" type="hidden" value="&#x2713;" /><input name="authenticity_token" type="hidden" value=".............." /></div>

  Reasons:

  • Some people do not like tag-soup.
  • Inline CSS is bad.
  • Its hard to use display: none instead of display: inline.

  ---

  Quick & Easy fix:

  Generate: <div class="protect_from_forgery"><input name="utf8" type="hidden" value="&#x2713;" /><input name="authenticity_token" type="hidden" value=".............." /></div>

  ...and in scaffold.css

  .protect_from_forgery { display: inline; margin: 0; padding: 0; }

  The css class name could be different as it includes the utf-8 flag as well and thus is not "exactly" matching what it contains.

  ---

  Better fix:

  1. Add class="protect_from_forgery" or something similar to all 3 hidden input fields for browser backwards compatibility (IE, other browsers can access the hidden elements style by going by css selector [type="hidden"].
     
  2. Prepend the the 3 hidden fields to
     
     • either: first generation of a rails3 input/textarea/select
     • or: the first generation of a rails3 submit/button

• You flagged this item as spam.
  

  Bruno Michel November 11th, 2010 @ 07:10 PM

  I've made a patch with cake_joe's suggestion. I hope it can be integrated for Rails 3.0.2.

  • authenticity_token_wrapper.diff 5.6 KB

• You flagged this item as spam.
  

  Bruno Michel November 11th, 2010 @ 07:10 PM

  • Tag changed from “csrf, form_helper, rails3, semantic” to “csrf, form_helper, patch, rails3, semantic”

• You flagged this item as spam.
  

  cake_joe November 13th, 2010 @ 02:44 AM

  First of all, thanks.

  Looking through your patch I notices this line:
  tags = snowman_tag << method_tag

  Snowman is not exactly the same like authenticity token => http://railssnowman.info/
  So the class name is not perfect or rather in some cases its missleading. Thus CSS classname 'rails_snowman_and_authenticity_wrapper' or something /better/ would fit better ;-)

• You flagged this item as spam.
  

  Lars Smit February 10th, 2011 @ 12:57 PM

  I changed the CSS classname as suggested by cake_joe to "snowman_and_authenticity_wrapper", which I think conveys the message. See the patch for suggestions and comments.

  • ticket5901_patch 5.8 KB

• You flagged this item as spam.
  

  Lars Smit February 16th, 2011 @ 08:45 AM

  Added .diff file for compatibility reasons.

  • ticket5901_patch.diff 5.8 KB
• 

  Jeff Kreeftmeijer February 27th, 2011 @ 05:53 PM

  • State changed from “new” to “open”
  • Importance changed from “” to “Low”

  The patch looks good and applies cleanly to master, it only gave me some whitespace errors. I've updated Lars' patch to remove those, retaining his authorship.

  @Lars: Maybe this is a good tip for next time: You can make sure your patches don't add any whitespace by applying it yourself using the --whitespace=error-all option. :)

  • ticket5901_patch.diff 5.8 KB

• You flagged this item as spam.
  

  Lars Smit February 28th, 2011 @ 11:21 AM

  Thanks Jeff for reviewing the patch! I added your tip about checking for whitespace to the Rails Guides, so other developers can benefit from it.

• 

  Jeff Kreeftmeijer March 1st, 2011 @ 11:29 AM

  • State changed from “open” to “verified”
  • Assigned user set to “Santiago Pastorino”

  Verified. The patch I posted was Lars' patch in which I removed some whitespace, I'm not verifying my own. ;)

  The patch applies cleanly to master and nicely removes some nasty inline styles. Bedankt Lars! :)

• 

  Josh Kalderimis March 1st, 2011 @ 12:37 PM

  • Tag changed from “csrf, form_helper, patch, rails3, semantic” to “action_pack, csrf, form_helper”
  • State changed from “verified” to “open”

  Hi,

  I have concerns about the current patch.

  First off, adding the css class definition to scaffold.css is a bad idea. Many people do not use scaffolds or keep the scaffolds.css file around, which would mean they the div would not be hidden properly and they would have a hard time figuring out why.

  Also, there is no current upgrade path taken into account. If I were to upgrade me 3.0.x app to 3.1 the authenticity div would no longer be hidden because i am missing the css class definition.

  And to be honest, I would prefer this were an opt-in change, either through a railtie and/or via the form_tag (eg. standards_wrapper_class => 'im_a_little_teapot'. That way the current inline would continue to work, but if the option was supplied then the class version would be used.

  And finally, snowman_and_authenticity_wrapper is implementation specific, how about standards_and_authenticity_wrapper, or just standards or authenticity wrapper?

  I am moving the ticket back to open for the time being as I think this needs to be fleshed out further.

  Thanks,

  Josh

• You flagged this item as spam.
  

  cake_joe March 1st, 2011 @ 02:24 PM

  @ Josh: +1 for the wrapper name 'standards_and_authenticity_wrapper'. Your idea works well for upgrading without hassles, though adds more extra/"bloat" code. Do you want to keep that forever or just have one deprecation "version" and then drop it. Is the extra code worth that? IMHO: worst if it stays like it currently in 3.1.

• 

  Josh Kalderimis March 1st, 2011 @ 05:45 PM

  @cake_joe : I am not saying that the ticket doesn't have some merit, but there are lots of use cases not taken into account before this could be a cohesive change.

  I think the first step is to add the form_tag variation so you can pass :standards_tag_class => 'auth_class' and have the inline properties replaced by a class.

  Then a Railtie addition can be set up so you can define it for all your forms.

  As for a default for new apps, I am not sure about this as it requires people to either use scaffold.css or application.css, if they do not use these asset files then they won't know about the css class.

  Thanks,

  Josh

• You flagged this item as spam.
  

  Lars Smit March 1st, 2011 @ 09:05 PM

  @Josh: The naming of the wrapper class is implementation specific. It shouldn't, so renaming it to "standards_and_authenticity_wrapper" is better. I furthermore look into a solution for a more flexibel class assigning.

• 

  Michael Koziarski March 2nd, 2011 @ 09:55 PM

  • State changed from “open” to “wontfix”

  I don't think these concerns justify adding yet another option to form_tag and having to support that forever.

  However we could take a patch which refactors it such that a custom form builder can generate whatever markup users want.

• You flagged this item as spam.
  

  Lars Smit March 3rd, 2011 @ 07:54 AM

  Is it a good idea to make it a configuration setting, like this?:

  config.standards_and_authenticity_wrapper.class = <name_of the_class>
  

  So people can assign their own class name and add it to their own style to the stylesheet. If the config class isn't set it will just display the inline style, otherwise the class. What do you think?

• 

  Josh Kalderimis March 4th, 2011 @ 01:29 PM

  @Lars, I would suggest you take Kozs advice on board and instead go down the custom form builder road. Maybe after this has been added we can look into the Railtie config possibility, but right now I do not think it has priority.

  Create a patch, with tests, which allows for custom form builders to generate whatever markup users want, then we can go from there.

  Thanks,

  Josh

• You flagged this item as spam.
  

  Lars Smit March 5th, 2011 @ 08:45 AM

  @Josh and @Michael, I'm going to work on the more generic solution, like Michael suggested; refactoring it to a custom form bulder so users can generate the markup they want. I will let you know when I have finished the patch.