Extend image_tag to accept ActiveStorage Attachments and Variants #30084

colorfulfool · 2017-08-05T10:25:22Z

To use an ActiveStorage attachment in image_tag, you have to write

<%= image_tag url_for(user.avatar) %>

This change simplifies it to

<%= image_tag user.avatar %>

And a bonus: if you pass a Blob, not a Variant, and specify the size arg, the image will be automatically downsampled by applying variant(resize: size) behind the scenes. This one I'm not sure of, is it too implicit?

rails-bot · 2017-08-05T10:25:25Z

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @kaspth (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

This repository is being automatically checked for code quality issues using Code Climate. You can see results for this analysis in the PR status below. Newly introduced issues should be fixed before a Pull Request is considered ready to review.

Please see the contribution instructions for more information.

georgeclaghorn · 2017-08-05T12:50:11Z

This one I'm not sure of, is it too implicit?

Yeah, I think that’s a bridge too far. Let’s scope this down to just accepting blobs and variants as arguments and consider automatic resizing separately.

colorfulfool · 2017-08-05T13:14:31Z

Where do you think the tests should be? Together with other image_tag tests or with other Blob tests? The latter has easier access to create_blob and ActiveStorage models. If the former, what is the right way to load the models in there?

georgeclaghorn · 2017-08-05T13:52:14Z

The tests for this probably belong in Active Storage’s test suite.

dhh · 2017-08-05T14:05:07Z

actionview/lib/action_view/helpers/asset_tag_helper.rb

@@ -364,6 +364,14 @@ def multiple_sources_tag_builder(type, sources)
          end
        end

+        def resolve_image_source(source, skip_pipeline)
+          if source.class.in? [ActiveStorage::Variant, ActiveStorage::Blob, ActiveStorage::Attachment]


Wondering if we can't do it the other way around. If the source is a string/symbol or whatever was reasonable before to use with image_tag, we'll call the existing path_to_image. If it's not, we try polymorphic_url. Then we wouldn't have to hardcode this to Active Storage and would open the door for people to build their own direct URLs for images.

Ha! That's how I was going to do it, but thought it will be rejected as too far-fetched. Along the lines of "if someone goes as far as to write their own image framework with their own custom routes, they surely can override image_tag themselves".

colorfulfool · 2017-08-05T20:27:00Z

activestorage/test/template/image_tag_test.rb

+  test "attachment on a model" do
+    @user.avatar.attach @blob
+    assert_dom_equal %(<img alt="Racecar" src="#{polymorphic_url @user.avatar}" />), image_tag(@user.avatar)
+  end


Which of the Attachment tests is better?

I would keep the plain attachment. A bit clearer.

dhh · 2017-08-05T20:43:57Z

Need to update documentation as well. Also, wonder what the error state looks like if you pass something in that polymorphic_url can't resolve?

colorfulfool · 2017-08-05T21:12:30Z

something polymorphic_url can't resolve => undefined method to_model
attachment is empty => undefined method to_model

The first case is okay I think, pass the wrong object — get an undefined method. But the empty attachment needs a more relevant message. Can delegate_missing_to :attachment be made to raise to_model is delegated to attachment, but attachment is nil instead of undefined method to_model?

georgeclaghorn · 2017-08-06T17:01:43Z

actionview/lib/action_view/helpers/asset_tag_helper.rb

@@ -200,7 +200,7 @@ def favicon_link_tag(source = "favicon.ico", options = {})
      end

      # Returns an HTML image tag for the +source+. The +source+ can be a full
-      # path or a file.
+      # path, a file or an ActiveStorage attachment.


When referring to the library and not the Ruby constant, leave a space between the words “Active” and “Storage.”

dhh · 2017-08-06T18:22:03Z

Don't love the first error state. We should catch and make it more specific to this usage. Because someone may well have passed something they thought would just auto-convert to a string or whatever, and then they see this instead.

Like the proposed fix for the second case.

colorfulfool · 2017-08-06T20:18:09Z

It should be a change to polymorphic_url, then. People may pass stuff into link_to and others just as well.

Is it okay to amend delegate_missing_to right in this PR?

dhh · 2017-08-06T20:25:34Z

I think we catch the exception raised by polymorphic_url and reraise our own here.

…

On Sun, Aug 6, 2017 at 3:18 PM, Anton Khamets ***@***.***> wrote: I think it should be a change to polymorphic_url, then. People may pass stuff into link_to just as well. Is it okay to amend delegate_missing_to right in this same PR? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#30084 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAKtU2OPmfRQhq-nEgZilxcjfMgGCdvks5sVh-lgaJpZM4OubPc> .

colorfulfool · 2017-08-07T14:03:40Z

activestorage/test/template/image_tag_test.rb

+    @user = User.create!(name: "DHH")
+
+    assert_not @user.avatar.attached?
+    assert_raises(ArgumentError) { image_tag(@user.avatar) }


This now raises ArgumentError: Can't resolve image into URL: to_model delegated to attachment, but attachment is nil. Not exactly what I wanted, but good enough I suppose?

dhh · 2017-08-07T14:38:59Z

Ace 👌

colorfulfool · 2017-08-08T08:41:02Z

Heeey, I haven't even had the chance to add myself into changelog!

dhh · 2017-08-08T17:18:09Z

Ah, do another PR with the changelog change and I'll merge that 👍 (assign it to me)

rafaelfranca · 2017-08-08T17:38:56Z

actionview/test/template/asset_tag_helper_test.rb

@@ -565,9 +565,6 @@ class PlaceholderImage
    def blank?; true; end
    def to_s; "no-image-yet.png"; end
  end
-  def test_image_tag_with_blank_placeholder


Why this test was removed?

It was testing a rudimentary version of the functionality this PR implements: an object with overloaded to_s.

Yeah, but there is no reason to delete this test, even more because it was testing a different kind of object.

rafaelfranca · 2017-08-08T17:39:08Z

actionview/lib/action_view/helpers/asset_tag_helper.rb

+          else
+            polymorphic_url(source)
+          end
+        rescue NoMethodError => e


Why we are rescuing this exception?

rafaelfranca · 2017-08-08T17:49:12Z

actionview/lib/action_view/helpers/asset_tag_helper.rb

+          if source.is_a?(Symbol) || source.is_a?(String)
+            path_to_image(source, skip_pipeline: skip_pipeline)
+          else
+            polymorphic_url(source)


Can we use url_for here?

url_for seems to truncate absolute URLs the same way polymorphic_path does (and polymorhic_url doesn't). That is, resolve('Attachment') { "http://dropbox.com/box456" } would turn out as /box456 instead of http://dropbox.com/box456.

rafaelfranca · 2017-08-08T17:49:57Z

actionview/lib/action_view/helpers/asset_tag_helper.rb

@@ -364,6 +375,16 @@ def multiple_sources_tag_builder(type, sources)
          end
        end

+        def resolve_image_source(source, skip_pipeline)
+          if source.is_a?(Symbol) || source.is_a?(String)


I'd invert this conditional. I'm sure we can check if source respond to a method like to_model before calling url_for.

ActiveStorage::Variant doesn't respond to to_model and it doesn't have to:

rails/actionpack/lib/action_dispatch/routing/polymorphic_routes.rb

Lines 169 to 175 in d3c0dcf

def polymorphic_mapping(record)

if record.respond_to?(:to_model)

_routes.polymorphic_mappings[record.to_model.model_name.name]

else

_routes.polymorphic_mappings[record.class.name]

end

end

We need to change ActiveStorage::Variant to respond to to_model instead. If we are expecting to use that class with URLs it needs to follow the ActiveModel::Model API.

Oh, to be fair that is not a requirement indeed. But I think in this case it is better to check to_model. It will avoid the rescue of NoMethodError and it also would avoid checking the type of the arguments.

In fact the only think that is annoying me here is the NoMethodError. Do you know why we need to rescue this exception?

But what if user passes an object that usually can be resolved into url, but right now can't? E.g. user.avatar when no avatar is attached. It will say Attachment is not a model or has a resolve, which is misleading.

My version will raise Can't resolve image into URL: to_model delegated to attachment, but attachment is nil which, I think, gives user the right clue.

I'm not sure the "can't resolve" part is necessary there, given an existing reasonable exception from the delegation. But even so, apart from the word "image", that all seems like reasonable behaviour for polymorphic_url.

Yeah, can't resolve object into URL + the underlaying exception should be doing the trick. If polymorphic_url raises that, we can remove the rescue here. Would that satisfy you, @rafaelfranca?

That seems good to me. 👍

@colorfulfool @rafaelfranca Fwiw, this broke our use of CarrierWave. Prior to this change, we could pass the thumbnail directly into the image_tag method:

<%= image_tag(user.avatar.tiny) %>

After this change, it throws Can't resolve image into URL: undefined method 'to_model' for #<AvatarUploader...

Adding .url fixes it, and it's arguable that we were using CarrierWave wrong, but passing an object that gets to_s called on it seems pretty common. I was surprised that this broke on a minor point release.

Do you think it's worth changing to support that behavior, or should we keep it as is?

rafaelfranca · 2017-08-08T18:25:18Z

actionview/test/template/asset_tag_helper_test.rb

@@ -565,9 +565,6 @@ class PlaceholderImage
    def blank?; true; end
    def to_s; "no-image-yet.png"; end
  end
-  def test_image_tag_with_blank_placeholder


Yeah, but there is no reason to delete this test, even more because it was testing a different kind of object.

rafaelfranca · 2017-08-08T18:26:27Z

activestorage/test/template/image_tag_test.rb

+  has_one_attached :avatar
+end
+
+class ActiveStorage::ImageTagTest < ActionView::TestCase


This entire file is coupling ActiveStorage to ActionView that is not a dependency of activestorage. We need to move those tests to actionview in the same way we do with activerecord.

Should they go under test/activerecord and test with generic models or under test/activestorage and stay as is, with Blobs and stuff?

I'm more inclined with test/activestorage since we may change the implementation in the future to not use Active Record (not that I see this happening but better to be safe)

rafaelfranca · 2017-08-08T18:53:00Z

activesupport/lib/active_support/core_ext/module/delegation.rb

@@ -273,6 +273,8 @@ def respond_to_missing?(name, include_private = false)
      def method_missing(method, *args, &block)
        if #{target}.respond_to?(method)
          #{target}.public_send(method, *args, &block)
+        elsif #{target}.nil?
+          raise DelegationError, "\#{method} delegated to #{target}, but #{target} is nil"


This is changing the behavior of delegating_to_missing so it needs a changelog and a test for it.

It merely replaces NoMethodError with its subclass DelegationError in come cases. Any code that relies on it will still work. Does this constitute a change in behavior, then?

That assumes super will raise; the class may well have other method_missing definitions in play.

rails-bot assigned kaspth Aug 5, 2017

colorfulfool changed the title ~~Extend image_tag to accept an ActiveStorage Blobs and Variants~~ Extend image_tag to accept ActiveStorage Blobs and Variants Aug 5, 2017

colorfulfool changed the title ~~Extend image_tag to accept ActiveStorage Blobs and Variants~~ Extend image_tag to accept ActiveStorage Attachments and Variants Aug 5, 2017

colorfulfool force-pushed the image-tag-blob branch from 7d78e35 to dae0a3f Compare August 5, 2017 10:30

Extend image_tag to accept ActiveStorage's Attachments and Variants

c6cfa59

colorfulfool force-pushed the image-tag-blob branch from dae0a3f to c6cfa59 Compare August 5, 2017 13:22

dhh reviewed Aug 5, 2017

View reviewed changes

Flip resolve_image_source around

31f1c39

colorfulfool force-pushed the image-tag-blob branch 2 times, most recently from 52aa0f6 to 8f24336 Compare August 5, 2017 20:22

colorfulfool commented Aug 5, 2017

View reviewed changes

Add tests for the new use-cases of image_tag

8bb4793

colorfulfool force-pushed the image-tag-blob branch from 8f24336 to 8bb4793 Compare August 5, 2017 20:31

Remove the higher-level test

5654366

georgeclaghorn reviewed Aug 6, 2017

View reviewed changes

colorfulfool force-pushed the image-tag-blob branch 3 times, most recently from 02c3ce3 to f44e7a4 Compare August 6, 2017 17:06

Update image_tag documentation

65cda40

colorfulfool force-pushed the image-tag-blob branch from f44e7a4 to 65cda40 Compare August 6, 2017 17:10

Add error states into the test suite

2b15d35

colorfulfool force-pushed the image-tag-blob branch 2 times, most recently from 242b85c to f22891f Compare August 7, 2017 11:59

Re-raise polymorhic_url's NoMethodError as ArgumentError

e563e69

colorfulfool force-pushed the image-tag-blob branch from f22891f to 01aff75 Compare August 7, 2017 12:23

delegate_missing_to will raise DelegationError instead of NoMethodError

8fae81e

colorfulfool force-pushed the image-tag-blob branch from 01aff75 to 8fae81e Compare August 7, 2017 13:11

colorfulfool commented Aug 7, 2017

View reviewed changes

dhh merged commit 7c89948 into rails:master Aug 7, 2017

colorfulfool deleted the image-tag-blob branch August 8, 2017 10:00

rafaelfranca reviewed Aug 8, 2017

View reviewed changes

colorfulfool mentioned this pull request Aug 11, 2017

Test for the new exception of delegate_missing_to #30191

Merged

colorfulfool pushed a commit to colorfulfool/rails that referenced this pull request Aug 17, 2017

Belated changelog for rails#30084

e6bdfdf

	def polymorphic_mapping(record)
	if record.respond_to?(:to_model)
	_routes.polymorphic_mappings[record.to_model.model_name.name]
	else
	_routes.polymorphic_mappings[record.class.name]
	end
	end

Extend image_tag to accept ActiveStorage Attachments and Variants #30084

Extend image_tag to accept ActiveStorage Attachments and Variants #30084

Conversation

colorfulfool commented Aug 5, 2017 • edited

rails-bot commented Aug 5, 2017

georgeclaghorn commented Aug 5, 2017

colorfulfool commented Aug 5, 2017 • edited

georgeclaghorn commented Aug 5, 2017

Choose a reason for hiding this comment

colorfulfool Aug 5, 2017 • edited

Choose a reason for hiding this comment

colorfulfool Aug 5, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhh commented Aug 5, 2017

colorfulfool commented Aug 5, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhh commented Aug 6, 2017

colorfulfool commented Aug 6, 2017 • edited

dhh commented Aug 6, 2017 via email

colorfulfool Aug 7, 2017 • edited

Choose a reason for hiding this comment

dhh commented Aug 7, 2017

colorfulfool commented Aug 8, 2017

dhh commented Aug 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colorfulfool Aug 13, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colorfulfool Aug 15, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colorfulfool Aug 16, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colorfulfool Aug 16, 2017 • edited

Choose a reason for hiding this comment

rafaelfranca Aug 16, 2017 • edited

Choose a reason for hiding this comment

jhubert Jul 20, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colorfulfool Aug 11, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colorfulfool commented Aug 5, 2017 •

edited

colorfulfool commented Aug 5, 2017 •

edited

colorfulfool Aug 5, 2017 •

edited

colorfulfool Aug 5, 2017 •

edited

colorfulfool commented Aug 5, 2017 •

edited

colorfulfool commented Aug 6, 2017 •

edited

colorfulfool Aug 7, 2017 •

edited

colorfulfool Aug 13, 2017 •

edited

colorfulfool Aug 15, 2017 •

edited

colorfulfool Aug 16, 2017 •

edited

colorfulfool Aug 16, 2017 •

edited

rafaelfranca Aug 16, 2017 •

edited

jhubert Jul 20, 2018 •

edited

colorfulfool Aug 11, 2017 •

edited