image_tag("") tries to get an image from the asset_pipeline #3080

Closed
rafamvc opened this Issue Sep 19, 2011 · 40 comments

Comments

Projects
None yet
@rafamvc

rafamvc commented Sep 19, 2011

When image_tag receives a blank string, it tries to get a resource from the asset pipeline and returns

"isn't precompiled"

IMHO it should return a blank <img src=""/> or something like it.

@dmathieu

This comment has been minimized.

Show comment
Hide comment
@dmathieu

dmathieu Sep 20, 2011

Contributor

http://dev.w3.org/html5/markup/img.html

src = non-empty URL potentially surrounded by spaces

The HTML5 documentation is pretty clear about it. The src tag shouldn't be empty.
Why would you want to provide an empty string to your image_tag ? I believe you should manage, inside your application, what you want to display as a default image when the one you're trying to display is not available.

Contributor

dmathieu commented Sep 20, 2011

http://dev.w3.org/html5/markup/img.html

src = non-empty URL potentially surrounded by spaces

The HTML5 documentation is pretty clear about it. The src tag shouldn't be empty.
Why would you want to provide an empty string to your image_tag ? I believe you should manage, inside your application, what you want to display as a default image when the one you're trying to display is not available.

@rafamvc

This comment has been minimized.

Show comment
Hide comment
@rafamvc

rafamvc Sep 20, 2011

I did enforce my application already.
If you want to enforce HTML to be compliant your should output a error for the right thing. The asset pipeline should not search for it.

The behavior of a link_to "", "" will render a invalid HTML a as well.

My point is that the error is non descriptive, and rails can do a better job at it, for the next person that stumble on this. :)

rafamvc commented Sep 20, 2011

I did enforce my application already.
If you want to enforce HTML to be compliant your should output a error for the right thing. The asset pipeline should not search for it.

The behavior of a link_to "", "" will render a invalid HTML a as well.

My point is that the error is non descriptive, and rails can do a better job at it, for the next person that stumble on this. :)

@amacneil

This comment has been minimized.

Show comment
Hide comment
@amacneil

amacneil Nov 18, 2011

+1. In general this exception is pretty annoying. On a production site it's not very helpful that if image_tag receives an unexpected string the whole page fails to load.

+1. In general this exception is pretty annoying. On a production site it's not very helpful that if image_tag receives an unexpected string the whole page fails to load.

@freetwix

This comment has been minimized.

Show comment
Hide comment
@freetwix

freetwix Dec 14, 2011

hey,

though html5 requires non-empty src attributes, rails should not throw an "isn't precompiled". its hard to find the root cause (its pointing to the asset pipeline instead of your code) and the former image_tag helper did behave different.

+1 for an empty src instead of throwing none-specific exceptions

hey,

though html5 requires non-empty src attributes, rails should not throw an "isn't precompiled". its hard to find the root cause (its pointing to the asset pipeline instead of your code) and the former image_tag helper did behave different.

+1 for an empty src instead of throwing none-specific exceptions

@lest

This comment has been minimized.

Show comment
Hide comment
@lest

lest Dec 14, 2011

Contributor

👍 it should not throw error

Contributor

lest commented Dec 14, 2011

👍 it should not throw error

@Godisemo

This comment has been minimized.

Show comment
Hide comment
@Godisemo

Godisemo Jan 24, 2012

This seems to cause errors for me to. Should not raise exceptions IMHO.

This seems to cause errors for me to. Should not raise exceptions IMHO.

@stjernstrom

This comment has been minimized.

Show comment
Hide comment
@stjernstrom

stjernstrom Feb 11, 2012

+1 should not throw error

+1 should not throw error

@stjernstrom

This comment has been minimized.

Show comment
Hide comment
@stjernstrom

stjernstrom Feb 11, 2012

Maybe this could work, showing a blank image if string is empty.

module ApplicationHelper

  def image_tag(source, options={})
    source = "blank_image.gif" if source.blank?
    super(source, options)
  end

end

Or, if you do not want the image tag at all

module ApplicationHelper

  def image_tag(source, options={})
    super(source, options) if source.present?
  end

end

Maybe this could work, showing a blank image if string is empty.

module ApplicationHelper

  def image_tag(source, options={})
    source = "blank_image.gif" if source.blank?
    super(source, options)
  end

end

Or, if you do not want the image tag at all

module ApplicationHelper

  def image_tag(source, options={})
    super(source, options) if source.present?
  end

end
@mipearson

This comment has been minimized.

Show comment
Hide comment
@mipearson

mipearson May 2, 2012

Contributor

+1. This just caused us a site outage as a staffer screwed up an image upload for a content item. We've worked around it in our code base, but we really shouldn't have to.

Contributor

mipearson commented May 2, 2012

+1. This just caused us a site outage as a staffer screwed up an image upload for a content item. We've worked around it in our code base, but we really shouldn't have to.

@mipearson

This comment has been minimized.

Show comment
Hide comment
Contributor

mipearson commented May 2, 2012

@amacneil

This comment has been minimized.

Show comment
Hide comment
@amacneil

amacneil May 2, 2012

Would be easier if we could just get the bug fixed..

amacneil commented May 2, 2012

Would be easier if we could just get the bug fixed..

@mipearson

This comment has been minimized.

Show comment
Hide comment
@mipearson

mipearson May 2, 2012

Contributor

Okay, I'm becoming less sure that this should be 'fixed', as it means that image_path becomes inconsistent with asset_path. I'm changing my +1 to a 0.

Other, custom, asset types might also not merit an Exception when missing (eg, fonts?). I believe an alternative solution should be discussed - maybe a config option to specify certain asset types as non-essential? A different helper method for data-driven content? Eh, I don't know.

Contributor

mipearson commented May 2, 2012

Okay, I'm becoming less sure that this should be 'fixed', as it means that image_path becomes inconsistent with asset_path. I'm changing my +1 to a 0.

Other, custom, asset types might also not merit an Exception when missing (eg, fonts?). I believe an alternative solution should be discussed - maybe a config option to specify certain asset types as non-essential? A different helper method for data-driven content? Eh, I don't know.

@KL-7

This comment has been minimized.

Show comment
Hide comment
@KL-7

KL-7 May 2, 2012

Contributor

@mipearson, the problem is that current implementation is inconsistent across different environments. As I described in my PR you get image tag with meaningless src="/assets" in development mode and in production with assets precompilation turned on you get an exception out of the blue.

Contributor

KL-7 commented May 2, 2012

@mipearson, the problem is that current implementation is inconsistent across different environments. As I described in my PR you get image tag with meaningless src="/assets" in development mode and in production with assets precompilation turned on you get an exception out of the blue.

@mipearson

This comment has been minimized.

Show comment
Hide comment
@mipearson

mipearson May 2, 2012

Contributor

@KL-7 I think having a blank asset_path call raising an exception would be a good solution to the above.

Contributor

mipearson commented May 2, 2012

@KL-7 I think having a blank asset_path call raising an exception would be a good solution to the above.

@amacneil

This comment has been minimized.

Show comment
Hide comment
@amacneil

amacneil May 3, 2012

Forget about the asset path for a second. image_tag is primarily an html helper function (e.g. if you are displaying images uploaded using carrierwave or whatever you already know the image url, the asset path isn't involved).

For example.. link_to("example", nil) works fine, it just links to '/'. Whatever, it doesn't really make sense, but the point is it doesn't raise an exception. This is how image_tag() behaved prior to Rails 3.1. Now, since 3.1, the existing behaviour broke (I'm sure this wasn't intentional). The asset path is the cause of the issue, but it doesn't excuse this behaviour.

The same is true of tag("img", :src => nil). Maybe it doesn't generate valid html, but it doesn't raise an exception. Now explain to me why image_tag() behaves differently to tag("img")?

When you call asset_path(), you know you are dealing with the asset path, so an exception is fine. When you call image_tag(), a lot of the time you aren't using the asset path at all (e.g. uploaded images), so I don't see why these two need to be consistent.

amacneil commented May 3, 2012

Forget about the asset path for a second. image_tag is primarily an html helper function (e.g. if you are displaying images uploaded using carrierwave or whatever you already know the image url, the asset path isn't involved).

For example.. link_to("example", nil) works fine, it just links to '/'. Whatever, it doesn't really make sense, but the point is it doesn't raise an exception. This is how image_tag() behaved prior to Rails 3.1. Now, since 3.1, the existing behaviour broke (I'm sure this wasn't intentional). The asset path is the cause of the issue, but it doesn't excuse this behaviour.

The same is true of tag("img", :src => nil). Maybe it doesn't generate valid html, but it doesn't raise an exception. Now explain to me why image_tag() behaves differently to tag("img")?

When you call asset_path(), you know you are dealing with the asset path, so an exception is fine. When you call image_tag(), a lot of the time you aren't using the asset path at all (e.g. uploaded images), so I don't see why these two need to be consistent.

@carlosantoniodasilva

This comment has been minimized.

Show comment
Hide comment
@carlosantoniodasilva

carlosantoniodasilva May 18, 2012

Member

Closing this since #5020 was merged. Thanks!

Closing this since #5020 was merged. Thanks!

@llacroix

This comment has been minimized.

Show comment
Hide comment
@llacroix

llacroix Dec 4, 2012

Got that problem today. In my case, image_tag really has to be empty. We're using javascript to fill the blanks. In other words, when the template is rendered with erb. There is no content or almost none. And some part of the pages are actual templates for KnockOutjs. I could probably put a falsy url but it's not as simple.

llacroix commented Dec 4, 2012

Got that problem today. In my case, image_tag really has to be empty. We're using javascript to fill the blanks. In other words, when the template is rendered with erb. There is no content or almost none. And some part of the pages are actual templates for KnockOutjs. I could probably put a falsy url but it's not as simple.

@steveklabnik

This comment has been minimized.

Show comment
Hide comment
@steveklabnik

steveklabnik Dec 4, 2012

Member

Why not just '' ?

Member

steveklabnik commented Dec 4, 2012

Why not just '' ?

@llacroix

This comment has been minimized.

Show comment
Hide comment
@llacroix

llacroix Dec 4, 2012

@steveklabnik what's the point of writing a helper someone offer to write <img /> instead of that helper. I fail to understand the logic. image_tag is a bit more than replacing the img tag. It has some parameters like size and mousehover etc...

And yes, I replaced the helper by the img tags everywhere I could. I think it's just quite funny to get offered to not use a helper because the helper isn't really helping. What kind of helper is that? =)

http://api.rubyonrails.org/classes/ActionView/Helpers/AssetTagHelper.html#method-i-image_tag

llacroix commented Dec 4, 2012

@steveklabnik what's the point of writing a helper someone offer to write <img /> instead of that helper. I fail to understand the logic. image_tag is a bit more than replacing the img tag. It has some parameters like size and mousehover etc...

And yes, I replaced the helper by the img tags everywhere I could. I think it's just quite funny to get offered to not use a helper because the helper isn't really helping. What kind of helper is that? =)

http://api.rubyonrails.org/classes/ActionView/Helpers/AssetTagHelper.html#method-i-image_tag

@carlosantoniodasilva

This comment has been minimized.

Show comment
Hide comment
@carlosantoniodasilva

carlosantoniodasilva Dec 4, 2012

Member

I believe this has been fixed in the linked pull request. @llacroix which version are you seeing this? It might be that it wasn't backported to 3-2.

I believe this has been fixed in the linked pull request. @llacroix which version are you seeing this? It might be that it wasn't backported to 3-2.

@steveklabnik

This comment has been minimized.

Show comment
Hide comment
@steveklabnik

steveklabnik Dec 5, 2012

Member

@llacroix because <img /> is not valid HTML. If you want to write invalid HTML, do it by hand.

<img src="" /> is not valid either. At minimum, it must be <img src="#" alt=""/>.

Member

steveklabnik commented Dec 5, 2012

@llacroix because <img /> is not valid HTML. If you want to write invalid HTML, do it by hand.

<img src="" /> is not valid either. At minimum, it must be <img src="#" alt=""/>.

@llacroix

This comment has been minimized.

Show comment
Hide comment
@llacroix

llacroix Dec 5, 2012

@steveklabnik sure it's not valid html, but the html gets processed by javascript to fill up content.

I'm on rails 3.2 as a matter of fact. To sum it up:

image_tag('')
image_tag(nil)
Dev => <img src="/assets/" alt="" />
Production => Precompile error
image_tag(nil, alt: nil)
Dev => <img src="/assets/" />
Production => Precompile error

Uhoh invalid markup in dev without error

image_tag('#')
Dev => <img src="/assets/#' alt='#' />
Production => Precompile error
image_tag('http:///', alt: nil)
Dev: <img src="http:///" />
Production: <img src="http:///" />

Uhoh no need to precompile since it's pointing to http:/// and alt tag is missing... invalid markup works in production. Actually the markup created contains an invalid url and no alt tag.

That's two error in here: http://validator.w3.org/check

But to be honest, my big problem here is that the behavior in production and in development is different. It's not cool that it works very well in dev but once you run the server in production you have lots of "production" errors that only happen in production.

So yes, it's possible to do invalid markup using the image_tag, so I'd be rather happy if the helper only quoted correctly whatever I pass, generated valid html markup but invalid img markup, than trying to validate and img and failing at it a restraining me to process that generated html elsewhere.

I'd be quite happy with something like image_tag(strict: false). But that would probably complicate things since you can't then create a "strict" attribute on the html tag. Having a way to suppress validation and precompilation would be cool and saves lot of time and headache.

llacroix commented Dec 5, 2012

@steveklabnik sure it's not valid html, but the html gets processed by javascript to fill up content.

I'm on rails 3.2 as a matter of fact. To sum it up:

image_tag('')
image_tag(nil)
Dev => <img src="/assets/" alt="" />
Production => Precompile error
image_tag(nil, alt: nil)
Dev => <img src="/assets/" />
Production => Precompile error

Uhoh invalid markup in dev without error

image_tag('#')
Dev => <img src="/assets/#' alt='#' />
Production => Precompile error
image_tag('http:///', alt: nil)
Dev: <img src="http:///" />
Production: <img src="http:///" />

Uhoh no need to precompile since it's pointing to http:/// and alt tag is missing... invalid markup works in production. Actually the markup created contains an invalid url and no alt tag.

That's two error in here: http://validator.w3.org/check

But to be honest, my big problem here is that the behavior in production and in development is different. It's not cool that it works very well in dev but once you run the server in production you have lots of "production" errors that only happen in production.

So yes, it's possible to do invalid markup using the image_tag, so I'd be rather happy if the helper only quoted correctly whatever I pass, generated valid html markup but invalid img markup, than trying to validate and img and failing at it a restraining me to process that generated html elsewhere.

I'd be quite happy with something like image_tag(strict: false). But that would probably complicate things since you can't then create a "strict" attribute on the html tag. Having a way to suppress validation and precompilation would be cool and saves lot of time and headache.

@jackdempsey

This comment has been minimized.

Show comment
Hide comment
@jackdempsey

jackdempsey Dec 29, 2012

Contributor

Just chiming in with a "image_tag and blank string throws and error!?" (on rails 3.2.9). This really surprised me, and the error of "ActionView::Template::Error ( isn't precompiled)" is not helpful.

Any more thoughts on this from core? Definitely feels like current implementation is wrong, and I'm sure plenty of people would love to help out if given guidance.

Contributor

jackdempsey commented Dec 29, 2012

Just chiming in with a "image_tag and blank string throws and error!?" (on rails 3.2.9). This really surprised me, and the error of "ActionView::Template::Error ( isn't precompiled)" is not helpful.

Any more thoughts on this from core? Definitely feels like current implementation is wrong, and I'm sure plenty of people would love to help out if given guidance.

@pixeltrix

This comment has been minimized.

Show comment
Hide comment
@pixeltrix

pixeltrix Dec 29, 2012

Member

One possible solution is to return a url of //:0 like this StackOverflow answer suggests if the path passed to asset_path is nil or blank. That way it passes the HTML validation tests and doesn't cause any server requests. On a related note we currently don't generate a blank alt attribute if one isn't specified and the path is a data uri or blank - should we?

Member

pixeltrix commented Dec 29, 2012

One possible solution is to return a url of //:0 like this StackOverflow answer suggests if the path passed to asset_path is nil or blank. That way it passes the HTML validation tests and doesn't cause any server requests. On a related note we currently don't generate a blank alt attribute if one isn't specified and the path is a data uri or blank - should we?

@llacroix

This comment has been minimized.

Show comment
Hide comment
@llacroix

llacroix Dec 29, 2012

@pixeltrix's suggestion seems good to me. A blank alt should be generated in any case where the alt has a "falsy" value. It can be empty but the alt has to be present to generate a valid "img" tag. And the //:0 seems to be a good alternative. But it has to be documented in some way. Because it might not be that clear of why a "falsy" src might return that url instead of an empty string or nothing.

@pixeltrix's suggestion seems good to me. A blank alt should be generated in any case where the alt has a "falsy" value. It can be empty but the alt has to be present to generate a valid "img" tag. And the //:0 seems to be a good alternative. But it has to be documented in some way. Because it might not be that clear of why a "falsy" src might return that url instead of an empty string or nothing.

@steveklabnik

This comment has been minimized.

Show comment
Hide comment
@steveklabnik

steveklabnik Dec 29, 2012

Member

we currently don't generate a blank alt attribute if one isn't specified and the path is a data uri or blank - should we?

👍

Member

steveklabnik commented Dec 29, 2012

we currently don't generate a blank alt attribute if one isn't specified and the path is a data uri or blank - should we?

👍

@antoineherzog

This comment has been minimized.

Show comment
Hide comment
@antoineherzog

antoineherzog Jan 13, 2013

I had still the error message ..strange .

I had still the error message ..strange .

@steveklabnik

This comment has been minimized.

Show comment
Hide comment
@steveklabnik

steveklabnik Jan 13, 2013

Member

@antoineherzog on which version of Rails?

Member

steveklabnik commented Jan 13, 2013

@antoineherzog on which version of Rails?

@antoineherzog

This comment has been minimized.

Show comment
Hide comment
@antoineherzog

antoineherzog Jan 13, 2013

3.2.8

2013/1/13 Steve Klabnik notifications@github.com

@antoineherzog https://github.com/antoineherzog on which version of
Rails?


Reply to this email directly or view it on GitHubhttps://github.com/rails/rails/issues/3080#issuecomment-12195134.

3.2.8

2013/1/13 Steve Klabnik notifications@github.com

@antoineherzog https://github.com/antoineherzog on which version of
Rails?


Reply to this email directly or view it on GitHubhttps://github.com/rails/rails/issues/3080#issuecomment-12195134.

@steveklabnik

This comment has been minimized.

Show comment
Hide comment
@steveklabnik

steveklabnik Jan 13, 2013

Member

@antoineherzog right. It was fixed on master, so it won't be fixed until 4.0.

Also, PLEASE make sure you've dealt with the recent security issue if you're on 3.2.8: https://groups.google.com/forum/?fromgroups=#!topic/rubyonrails-security/61bkgvnSGTQ

Member

steveklabnik commented Jan 13, 2013

@antoineherzog right. It was fixed on master, so it won't be fixed until 4.0.

Also, PLEASE make sure you've dealt with the recent security issue if you're on 3.2.8: https://groups.google.com/forum/?fromgroups=#!topic/rubyonrails-security/61bkgvnSGTQ

@antoineherzog

This comment has been minimized.

Show comment
Hide comment
@antoineherzog

antoineherzog Jan 13, 2013

ok thank you for your reply ! I will wait 4.0 ;)

ok thank you for your reply ! I will wait 4.0 ;)

@joemsak

This comment has been minimized.

Show comment
Hide comment
@joemsak

joemsak Jan 17, 2013

Is there any reason the image_tag helper couldn't just plain return nil?

I override image_tag in my ApplicationHelper to do this

def image_tag(source, options = {})
  super(source, options) unless source.blank?
end

works perfectly for me

joemsak commented Jan 17, 2013

Is there any reason the image_tag helper couldn't just plain return nil?

I override image_tag in my ApplicationHelper to do this

def image_tag(source, options = {})
  super(source, options) unless source.blank?
end

works perfectly for me

@tombh

This comment has been minimized.

Show comment
Hide comment
@tombh

tombh Jan 29, 2013

Just chiming in with a "image_tag and blank string throws and error!?" (on rails 3.2.9). This really surprised me, and the error of "ActionView::Template::Error ( isn't precompiled)" is not helpful.

+1

tombh commented Jan 29, 2013

Just chiming in with a "image_tag and blank string throws and error!?" (on rails 3.2.9). This really surprised me, and the error of "ActionView::Template::Error ( isn't precompiled)" is not helpful.

+1

@idelahoz

This comment has been minimized.

Show comment
Hide comment
@idelahoz

idelahoz Feb 22, 2013

it also, happens with audio_tag, it shouldnt send an error if you pass blank where an asset path is expected in no case, sometimes you wanna render that empty to fill it later with js code

it also, happens with audio_tag, it shouldnt send an error if you pass blank where an asset path is expected in no case, sometimes you wanna render that empty to fill it later with js code

@yagudaev

This comment has been minimized.

Show comment
Hide comment
@yagudaev

yagudaev Mar 5, 2013

+1. I think this violates the rails philosophy of no surprises.

yagudaev commented Mar 5, 2013

+1. I think this violates the rails philosophy of no surprises.

@rthbound

This comment has been minimized.

Show comment
Hide comment
@rthbound

rthbound May 3, 2013

Contributor

+1, it's an annoyance.

Also happens on the octothorpe:

image_tag("#", class: "icon-download-alt", alt: "Download")

ActionView::Template::Error (# isn't precompiled)

Contributor

rthbound commented May 3, 2013

+1, it's an annoyance.

Also happens on the octothorpe:

image_tag("#", class: "icon-download-alt", alt: "Download")

ActionView::Template::Error (# isn't precompiled)

@rderoldan1

This comment has been minimized.

Show comment
Hide comment
@rderoldan1

rderoldan1 May 15, 2013

I had partly the same problem, but in some cases in the data base I have null in the image path or the image does't exist, that is why I mixed the code of some people here, and here is my solution

  def image_tag(img , options={})
    path = "#{Rails.root}/app/assets/images/#{img}"
    img = "missing.png"  unless img.present? and File.file?(path)
    super(img, options)
  end

Now I validate if the path is "" and if the image exists

I had partly the same problem, but in some cases in the data base I have null in the image path or the image does't exist, that is why I mixed the code of some people here, and here is my solution

  def image_tag(img , options={})
    path = "#{Rails.root}/app/assets/images/#{img}"
    img = "missing.png"  unless img.present? and File.file?(path)
    super(img, options)
  end

Now I validate if the path is "" and if the image exists

@nleo

This comment has been minimized.

Show comment
Hide comment
@nleo

nleo Mar 12, 2014

+1 shoud not raise exeption

nleo commented Mar 12, 2014

+1 shoud not raise exeption

@jun1st

This comment has been minimized.

Show comment
Hide comment
@jun1st

jun1st Mar 21, 2014

+1 should not raise exception. at least not throw "isn't compiled"

jun1st commented Mar 21, 2014

+1 should not raise exception. at least not throw "isn't compiled"

@bcackerman

This comment has been minimized.

Show comment
Hide comment

++1

@rafaelfranca rafaelfranca locked and limited conversation to collaborators Aug 15, 2014

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