This project is archived and is in readonly mode.
[PATCH] [BENCHMARK] [RAILS3] image_tag shouldn't bomb on creating an alt attribute when split returns nil or ignore other parts of a filename if it has more than one period
Reported by Rael Dornfest | June 25th, 2009 @ 08:52 PM | in 2.x
Having an image filename starting with a . (odd, admittedly) causes image_tag to throw an error on attempting to auto-create an alt attribute since '.'.split('.').first is nil.
/actionpack/lib/action_view/helpers/asset_tag_helper.rb, line 493:
def image_tag(source, options = {})
options.symbolize_keys!
options[:src] = path_to_image(source)
options[:alt] ||= File.basename(options[:src], '.*').split('.').first.to_s.capitalize
Comments and changes to this ticket
-
Yehuda Katz (wycats) July 2nd, 2009 @ 07:32 PM
- State changed from “new” to “incomplete”
There is already a test for a case starting with period. This could only happen with image_tag("."). Is there a reason someone would want to do that? Or is there a case starting with a period that still fails?
-
Eadz February 24th, 2010 @ 10:41 AM
- Assigned user set to “Yehuda Katz (wycats)”
- Tag changed from “:alt, image_tag, nil” to “:alt, benchmark, doc, image_tag, nil, rails3”
- Title changed from “image_tag shouldn't bomb on creating an alt attribute when split returns nil” to “[PATCH] [BENCHMARK] [RAILS3] image_tag shouldn't bomb on creating an alt attribute when split returns nil or ignore other parts of a filename if it has more than one period”
Hi there,
I have had a different issue with image_tag. It's bugged me for a while. I've attached a patch that fixes it. I've also included tests and benchmarks, and documentation typo fix. This makes the part of image_tag that deals with alt text twice as quick too.
The changes are minor, and only effect filenames with more than one period.
A filename of 'google.com.png' will now have an alt of 'Google.com' rather than 'Google'
A filename of '..jpg' will now have an alt of '.' rather than ''
A filename of '.pdf.png' will now have an alt of '.pdf' rather than ''
A filename of 'slash..png' will now have an alt of 'Slash.' rather than 'slash'
The nil issue shouldn't happen anymore as I have removed the unnecessary(?) call to split and first. File.basename(filename,'.*') does the job of removing the filename suffix and I don't know why split was there in the first place. All actionpack tests pass as of current rails edge.
Benchmarks: ( this is a very simple benchmark. As the new version simply removes two calls, without adding any, I don't know how it could get slower anyway ;) )
Old Version
user system total real1.510000 0.000000 1.510000 ( 1.526783) New Version
user system total real0.820000 0.000000 0.820000 ( 0.816405) Old Version [2nd test]
user system total real0.310000 0.000000 0.310000 ( 0.313089) New Version [2nd test]
user system total real0.150000 0.000000 0.150000 ( 0.153177)
-
Eadz February 24th, 2010 @ 10:54 AM
- Tag changed from “:alt, benchmark, doc, image_tag, nil, rails3” to “:alt, benchmark, doc, image_tag, nil, rails3, tiny”
sorry, to_s is redundant too .
Updated patch is attched.
The method changes from
File.basename(src, '.*').split('.').first.to_s.capitalize
to
File.basename(src, '.*').capitalize
-
-
Repository March 12th, 2010 @ 01:33 AM
- State changed from “incomplete” to “committed”
(from [b27376773e8f51b03bd4cb2678764cd392455870]) simplify alt tag generation for images
[#2837 state:committed]
Signed-off-by: Jeremy Kemper jeremy@bitsweat.net
http://github.com/rails/rails/commit/b27376773e8f51b03bd4cb2678764c...
Create your profile
Help contribute to this project by taking a few moments to create your personal profile. Create your profile »
<h2 style="font-size: 14px">Tickets have moved to Github</h2>
The new ticket tracker is available at <a href="https://github.com/rails/rails/issues">https://github.com/rails/rails/issues</a>
Eadz
Rael Dornfest
Yehuda Katz (wycats)
