diff --git a/app/models/optimized_image.rb b/app/models/optimized_image.rb index d745bc2d1b6..331c0716ca6 100644 --- a/app/models/optimized_image.rb +++ b/app/models/optimized_image.rb @@ -3,7 +3,9 @@ require "digest/sha1" class OptimizedImage < ActiveRecord::Base belongs_to :upload - def self.create_for(upload, width=nil, height=nil) + def self.create_for(upload, width, height) + return unless width && height + @image_sorcery_loaded ||= require "image_sorcery" original_path = "#{Rails.root}/public#{upload.url}" @@ -11,15 +13,13 @@ class OptimizedImage < ActiveRecord::Base temp_file = Tempfile.new(["discourse", File.extname(original_path)]) temp_path = temp_file.path - # do the resize when there is both dimensions - if width && height && ImageSorcery.new(original_path).convert(temp_path, resize: "#{width}x#{height}") - image_info = FastImage.new(temp_path) + if ImageSorcery.new(original_path).convert(temp_path, resize: "#{width}x#{height}") thumbnail = OptimizedImage.new({ upload_id: upload.id, sha1: Digest::SHA1.file(temp_path).hexdigest, extension: File.extname(temp_path), - width: image_info.size[0], - height: image_info.size[1] + width: width, + height: height }) # make sure the directory exists FileUtils.mkdir_p Pathname.new(thumbnail.path).dirname diff --git a/app/models/upload.rb b/app/models/upload.rb index 61c0cf2749c..38d5a8592b2 100644 --- a/app/models/upload.rb +++ b/app/models/upload.rb @@ -31,7 +31,6 @@ class Upload < ActiveRecord::Base def create_thumbnail! return unless SiteSetting.create_thumbnails? return if SiteSetting.enable_s3_uploads? - return if width < SiteSetting.max_image_width return if has_thumbnail? thumbnail = OptimizedImage.create_for(self, width, height) optimized_images << thumbnail if thumbnail @@ -48,10 +47,7 @@ class Upload < ActiveRecord::Base # compute the sha sha1 = Digest::SHA1.file(file.tempfile).hexdigest # check if the file has already been uploaded - upload = Upload.where(sha1: sha1).first - - # otherwise, create it - if upload.blank? + unless upload = Upload.where(sha1: sha1).first # retrieve image info image_info = FastImage.new(file.tempfile, raise_on_failure: true) # compute image aspect ratio @@ -111,6 +107,16 @@ class Upload < ActiveRecord::Base ActionController::Base.asset_host end + def self.get_from_url(url) + if has_been_uploaded?(url) + if m = LocalStore.uploaded_regex.match(url) + Upload.where(id: m[:upload_id]).first + elsif is_on_s3?(url) + Upload.where(url: url).first + end + end + end + end # == Schema Information diff --git a/lib/cooked_post_processor.rb b/lib/cooked_post_processor.rb index 6a188f43085..97507b86bed 100644 --- a/lib/cooked_post_processor.rb +++ b/lib/cooked_post_processor.rb @@ -1,5 +1,5 @@ # Post processing that we can do after a post has already been cooked. -# For example, inserting the onebox content, or image sizes. +# For example, inserting the onebox content, or image sizes/thumbnails. require_dependency 'oneboxer' @@ -21,43 +21,31 @@ class CookedPostProcessor end def post_process_images - images = @doc.css("img") - @doc.css(".onebox-result img") - @doc.css(".quote img") + images = extract_images return if images.blank? images.each do |img| - # keep track of the original src - src = img['src'] - # make sure the src is absolute (when working with locally uploaded files) - img['src'] = Discourse.base_url_no_prefix + img['src'] if img['src'] =~ /^\/[^\/]/ - - if src.present? - # make sure the img has both width and height attributes + if img['src'].present? + # keep track of the original src + src = img['src'] + # make sure the src is absolute (when working with locally uploaded files) + img['src'] = relative_to_absolute(src) + # make sure the img has proper width and height attributes update_dimensions!(img) # retrieve the associated upload, if any - upload = get_upload_from_url(img['src']) - if upload.present? + if upload = Upload.get_from_url(img['src']) # update reverse index - associate_to_post upload - # create a thumbnail - upload.create_thumbnail! - # optimize image - img['src'] = optimize_image(img) - # lightbox treatment - convert_to_link!(img, upload) - else - convert_to_link!(img) + associate_to_post(upload) end + # lightbox treatment + convert_to_link!(img, upload) # mark the post as dirty whenever the src has changed @dirty |= src != img['src'] end end # Extract the first image from the first post and use it as the 'topic image' - if @post.post_number == 1 - img = images.first - @post.topic.update_column :image_url, img['src'] if img['src'].present? - end - + extract_topic_image(images) end def post_process_oneboxes @@ -71,28 +59,31 @@ class CookedPostProcessor @dirty |= result.changed? end + def extract_images + # do not extract images inside a onebox or a quote + @doc.css("img") - @doc.css(".onebox-result img") - @doc.css(".quote img") + end + + def relative_to_absolute(src) + if src =~ /\A\/[^\/]/ + Discourse.base_url_no_prefix + src + else + src + end + end + def update_dimensions!(img) return if img['width'].present? && img['height'].present? w, h = get_size_from_image_sizes(img['src'], @opts[:image_sizes]) || image_dimensions(img['src']) if w && h - img['width'] = w.to_s - img['height'] = h.to_s + img['width'] = w + img['height'] = h @dirty = true end end - def get_upload_from_url(url) - if Upload.has_been_uploaded?(url) - if m = LocalStore.uploaded_regex.match(url) - Upload.where(id: m[:upload_id]).first - elsif Upload.is_on_s3?(url) - Upload.where(url: url).first - end - end - end - def associate_to_post(upload) return if PostUpload.where(post_id: @post.id, upload_id: upload.id).count > 0 PostUpload.create({ post_id: @post.id, upload_id: upload.id }) @@ -100,10 +91,10 @@ class CookedPostProcessor # do not care if it's already associated end - def optimize_image(img) - return img["src"] + def optimize_image!(img) + # TODO # 1) optimize using image_optim - # 2) .png vs. .jpg + # 2) .png vs. .jpg (> 1.5x) end def convert_to_link!(img, upload=nil) @@ -113,42 +104,59 @@ class CookedPostProcessor width, height = img["width"].to_i, img["height"].to_i original_width, original_height = get_size(src) - return unless original_width.to_i > width && original_height.to_i > height - return if original_width < SiteSetting.max_image_width + return if original_width.to_i <= width && original_height.to_i <= height + return if original_width.to_i <= SiteSetting.max_image_width + return if is_a_hyperlink(img) + if upload + # create a thumbnail + upload.create_thumbnail! + # optimize image + # TODO: optimize_image!(img) + end + + add_lightbox!(img, original_width, original_height, upload) + + @dirty = true + end + + def is_a_hyperlink(img) parent = img.parent while parent return if parent.name == "a" break unless parent.respond_to? :parent parent = parent.parent end + end - # not a hyperlink so we can apply - img['src'] = upload.thumbnail_url if (upload && upload.thumbnail_url.present?) + def add_lightbox!(img, original_width, original_height, upload=nil) # first, create a div to hold our lightbox - lightbox = Nokogiri::XML::Node.new "div", @doc - img.add_next_sibling lightbox - lightbox.add_child img + lightbox = Nokogiri::XML::Node.new("div", @doc) + img.add_next_sibling(lightbox) + lightbox.add_child(img) + # then, the link to our larger image - a = Nokogiri::XML::Node.new "a", @doc + a = Nokogiri::XML::Node.new("a", @doc) img.add_next_sibling(a) - a["href"] = src + a["href"] = img['src'] a["class"] = "lightbox" a.add_child(img) - # then, some overlay informations - meta = Nokogiri::XML::Node.new "div", @doc - meta["class"] = "meta" - img.add_next_sibling meta - filename = get_filename(upload, src) + # replace the image by its thumbnail + img['src'] = upload.thumbnail_url if upload && upload.has_thumbnail? + + # then, some overlay informations + meta = Nokogiri::XML::Node.new("div", @doc) + meta["class"] = "meta" + img.add_next_sibling(meta) + + filename = get_filename(upload, img['src']) informations = "#{original_width}x#{original_height}" informations << " | #{number_to_human_size(upload.filesize)}" if upload meta.add_child create_span_node("filename", filename) meta.add_child create_span_node("informations", informations) meta.add_child create_span_node("expand") - - @dirty = true end def get_filename(upload, src) @@ -158,12 +166,19 @@ class CookedPostProcessor end def create_span_node(klass, content=nil) - span = Nokogiri::XML::Node.new "span", @doc + span = Nokogiri::XML::Node.new("span", @doc) span.content = content if content span['class'] = klass span end + def extract_topic_image(images) + if @post.post_number == 1 + img = images.first + @post.topic.update_column :image_url, img['src'] if img['src'].present? + end + end + def get_size_from_image_sizes(src, image_sizes) if image_sizes.present? if dim = image_sizes[src] @@ -180,8 +195,8 @@ class CookedPostProcessor def get_size(url) # make sure s3 urls have a scheme (otherwise, FastImage will fail) - url = "http:" + url if Upload.is_on_s3? (url) - return unless is_valid_image_uri? url + url = "http:" + url if Upload.is_on_s3?(url) + return unless is_valid_image_uri?(url) # we can *always* crawl our own images return unless SiteSetting.crawl_images? || Upload.has_been_uploaded?(url) @size_cache[url] ||= FastImage.size(url) diff --git a/spec/components/cooked_post_processor_spec.rb b/spec/components/cooked_post_processor_spec.rb index 9b17a1a97c6..3fc9c0cfc0d 100644 --- a/spec/components/cooked_post_processor_spec.rb +++ b/spec/components/cooked_post_processor_spec.rb @@ -3,218 +3,164 @@ require 'cooked_post_processor' describe CookedPostProcessor do - def cpp(cooked = nil, options = {}) - post = Fabricate.build(:post_with_youtube) - post.cooked = cooked if cooked - post.id = 123 - CookedPostProcessor.new(post, options) - end + context "post_process" do - context 'process_onebox' do + let(:post) { build(:post) } + let(:cpp) { CookedPostProcessor.new(post) } + let(:post_process) { sequence("post_process") } - before do - @cpp = cpp(nil, invalidate_oneboxes: true) - Oneboxer.expects(:onebox).with("http://www.youtube.com/watch?v=9bZkp7q19f0", post_id: 123, invalidate_oneboxes: true).returns('
GANGNAM STYLE
') - @cpp.post_process_oneboxes - end - - it 'should be dirty' do - @cpp.should be_dirty - end - - it 'inserts the onebox without wrapping p' do - @cpp.html.should match_html "
GANGNAM STYLE
" + it "works on images before oneboxes" do + cpp.expects(:post_process_images).in_sequence(post_process) + cpp.expects(:post_process_oneboxes).in_sequence(post_process) + cpp.post_process end end + context "post_process_images" do - context 'process_images' do + context "with images in quotes and oneboxes" do - it "has no topic image if there isn't one in the post" do - @post = Fabricate(:post) - @post.topic.image_url.should be_blank - end + let(:post) { build(:post_with_images_in_quote_and_onebox) } + let(:cpp) { CookedPostProcessor.new(post) } + before { cpp.post_process_images } - context 'with sized images in the post' do - before do - @topic = Fabricate(:topic) - @post = Fabricate.build(:post_with_image_url, topic: @topic, user: @topic.user) - @cpp = CookedPostProcessor.new(@post, image_sizes: {'http://www.forumwarz.com/images/header/logo.png' => {'width' => 111, 'height' => 222}}) + it "does not process them" do + cpp.html.should match_html post.cooked + cpp.should_not be_dirty end + it "has no topic image if there isn't one in the post" do + post.topic.image_url.should be_blank + end + + end + + context "with uploaded images" do + + let(:upload) { Fabricate(:upload) } + let(:post) { Fabricate(:post_with_uploaded_images) } + let(:cpp) { CookedPostProcessor.new(post) } + before { FastImage.stubs(:size) } + + # all in one test to speed things up + it "works" do + Upload.expects(:get_from_url).returns(upload).twice + cpp.post_process_images + # ensures absolute urls on uploaded images + cpp.html.should =~ /#{Discourse.base_url_no_prefix}/ + # dirty + cpp.should be_dirty + # keeps the reverse index up to date + post.uploads.reload + post.uploads.count.should == 1 + end + + end + + context "width sized images" do + + let(:post) { build(:post_with_image_url) } + let(:cpp) { CookedPostProcessor.new(post, image_sizes: {'http://foo.bar/image.png' => {'width' => 111, 'height' => 222}}) } + + before { FastImage.stubs(:size).returns([150, 250]) } + it "doesn't call image_dimensions because it knows the size" do - @cpp.expects(:image_dimensions).never - @cpp.post_process_images + cpp.expects(:image_dimensions).never + cpp.post_process_images end it "adds the width from the image sizes provided" do - @cpp.post_process_images - @cpp.html.should =~ /width=\"111\"/ + cpp.post_process_images + cpp.html.should =~ /width=\"111\"/ + cpp.should be_dirty end end - context 'with uploaded images in the post' do - before do - @topic = Fabricate(:topic) - @post = Fabricate(:post_with_uploads, topic: @topic, user: @topic.user) - @cpp = CookedPostProcessor.new(@post) - @cpp.expects(:get_upload_from_url).returns(Fabricate(:upload)) - @cpp.expects(:get_size).returns([100,200]) - end + context "with unsized images" do - it "keeps reverse index up to date" do - @cpp.post_process_images - @post.uploads.reload - @post.uploads.count.should == 1 + let(:post) { build(:post_with_unsized_images) } + let(:cpp) { CookedPostProcessor.new(post) } + + it "adds the width and height to images that don't have them" do + FastImage.expects(:size).returns([123, 456]) + cpp.post_process_images + cpp.html.should =~ /width="123" height="456"/ + cpp.should be_dirty end end - context 'with unsized images in the post' do - let(:user) { Fabricate(:user) } - let(:topic) { Fabricate(:topic, user: user) } + context "with large images" do + + let(:upload) { Fabricate(:upload) } + let(:post) { build(:post_with_large_image) } + let(:cpp) { CookedPostProcessor.new(post) } before do - FastImage.stubs(:size).returns([123, 456]) - creator = PostCreator.new(user, raw: Fabricate.build(:post_with_images).raw, topic_id: topic.id) - @post = creator.create - end - - it "adds a topic image if there's one in the post" do - @post.topic.reload - @post.topic.image_url.should == "http://test.localhost/path/to/img.jpg" - end - - it "adds the height and width to images that don't have them" do - @post.reload - @post.cooked.should =~ /width=\"123\" height=\"456\"/ - end - - end - - context 'with an absolute image path without protocol' do - let(:user) { Fabricate(:user) } - let(:topic) { Fabricate(:topic, user: user) } - let(:post) { Fabricate.build(:post_with_s3_image_url, topic: topic, user: user) } - let(:processor) { CookedPostProcessor.new(post) } - - before do - processor.post_process_images - end - - it "doesn't change the protocol" do - processor.html.should =~ /src="\/\/bucket\.s3\.amazonaws\.com\/uploads\/6\/4\/123\.png"/ - end - end - - context 'with a oneboxed image' do - let(:user) { Fabricate(:user) } - let(:topic) { Fabricate(:topic, user: user) } - let(:post) { Fabricate.build(:post_with_oneboxed_image, topic: topic, user: user) } - let(:processor) { CookedPostProcessor.new(post) } - - before do - processor.post_process_images - end - - it "doesn't lightbox" do - processor.html.should_not =~ /class="lightbox"/ - end - end - - context "with a large image" do - - let(:user) { Fabricate(:user) } - let(:topic) { Fabricate(:topic, user: user) } - let(:post) { Fabricate.build(:post_with_uploads, topic: topic, user: user) } - let(:processor) { CookedPostProcessor.new(post) } - - before do - SiteSetting.stubs(:max_image_width).returns(10) - FastImage.stubs(:size).returns([1000, 1000]) - processor.post_process_images + SiteSetting.stubs(:create_thumbnails?).returns(true) + Upload.expects(:get_from_url).returns(upload) + cpp.stubs(:associate_to_post) + FastImage.stubs(:size).returns([1000, 2000]) + # optimized_image + FileUtils.stubs(:mkdir_p) + File.stubs(:open) + ImageSorcery.any_instance.expects(:convert).returns(true) end it "generates overlay information" do - processor.html.should =~ /class="lightbox"/ - processor.html.should =~ /class="meta"/ - processor.html.should =~ /class="filename"/ - processor.html.should =~ /class="informations"/ - processor.html.should =~ /class="expand"/ + cpp.post_process_images + cpp.html.should match_html '
+uploaded.jpg1000x2000 | 1.21 KB +
' + cpp.should be_dirty + end + + end + + context "topic image" do + + let(:topic) { build(:topic, id: 1) } + let(:post) { Fabricate(:post_with_uploaded_images, topic: topic) } + let(:cpp) { CookedPostProcessor.new(post) } + + it "adds a topic image if there's one in the post" do + FastImage.stubs(:size).returns([100, 100]) + cpp.post_process_images + post.topic.reload + post.topic.image_url.should == "http://test.localhost/uploads/default/2/3456789012345678.png" end end end - context 'link convertor' do + context "post_process_oneboxes" do + + let(:post) { build(:post_with_youtube, id: 123) } + let(:cpp) { CookedPostProcessor.new(post, invalidate_oneboxes: true) } + before do - SiteSetting.stubs(:crawl_images?).returns(true) + Oneboxer.expects(:onebox).with("http://www.youtube.com/watch?v=9bZkp7q19f0", post_id: 123, invalidate_oneboxes: true).returns('
GANGNAM STYLE
') + cpp.post_process_oneboxes end - let(:post_with_img) { Fabricate.build(:post, cooked: '

') } - let(:cpp_for_post) { CookedPostProcessor.new(post_with_img) } - - it 'convert img tags to links if they are sized down' do - cpp_for_post.expects(:get_size).returns([2000,2000]).twice - cpp_for_post.post_process - cpp_for_post.html.should =~ /a href/ + it "should be dirty" do + cpp.should be_dirty end - it 'does not convert img tags to links if they are small' do - cpp_for_post.expects(:get_size).returns([200,200]).twice - cpp_for_post.post_process - (cpp_for_post.html !~ /a href/).should be_true - end - - end - - context "image_dimensions" do - - it "returns unless called with a http or https url" do - cpp.image_dimensions("/tmp/image.jpg").should be_blank - end - - context "with valid url" do - - let(:url) { "http://www.forumwarz.com/images/header/logo.png" } - - it "doesn't call FastImage if image crawling is disabled" do - SiteSetting.expects(:crawl_images?).returns(false) - FastImage.expects(:size).never - cpp.image_dimensions(url) - end - - it "calls FastImage if image crawling is enabled" do - SiteSetting.expects(:crawl_images?).returns(true) - FastImage.expects(:size).with(url) - cpp.image_dimensions(url) - end - end - - end - - context "is_valid_image_uri?" do - - it "needs the scheme to be either http or https" do - cpp.is_valid_image_uri?("http://domain.com").should == true - cpp.is_valid_image_uri?("https://domain.com").should == true - cpp.is_valid_image_uri?("ftp://domain.com").should == false - cpp.is_valid_image_uri?("ftps://domain.com").should == false - cpp.is_valid_image_uri?("//domain.com").should == false - cpp.is_valid_image_uri?("/tmp/image.png").should == false - end - - it "doesn't throw an exception with a bad URI" do - cpp.is_valid_image_uri?("http://doGANGNAM STYLE" end end context "get_filename" do + let(:post) { build(:post) } + let(:cpp) { CookedPostProcessor.new(post) } + it "returns the filename of the src when there is no upload" do cpp.get_filename(nil, "http://domain.com/image.png").should == "image.png" end @@ -231,4 +177,80 @@ describe CookedPostProcessor do end + context "image_dimensions" do + + let(:post) { build(:post) } + let(:cpp) { CookedPostProcessor.new(post) } + + it "calls the resizer" do + SiteSetting.stubs(:max_image_width).returns(200) + cpp.expects(:get_size).returns([1000, 2000]) + cpp.image_dimensions("http://foo.bar/image.png").should == [200, 400] + end + + it "doesn't call the resizer when there is no size" do + cpp.expects(:get_size).returns(nil) + cpp.image_dimensions("http://foo.bar/image.png").should == nil + end + + end + + context "get_size" do + + let(:post) { build(:post) } + let(:cpp) { CookedPostProcessor.new(post) } + + it "ensures s3 urls have a default scheme" do + Upload.stubs(:is_on_s3?).returns(true) + FastImage.stubs(:size) + cpp.expects(:is_valid_image_uri?).with("http://bucket.s3.aws.amazon.com/image.jpg") + cpp.get_size("//bucket.s3.aws.amazon.com/image.jpg") + end + + context "crawl_images is disabled" do + + before { SiteSetting.stubs(:crawl_images?).returns(false) } + + it "doesn't call FastImage" do + FastImage.expects(:size).never + cpp.get_size("http://foo.bar/image.png").should == nil + end + + it "is always allowed to crawled our own images" do + Upload.expects(:has_been_uploaded?).returns(true) + FastImage.expects(:size).returns([100, 200]) + cpp.get_size("http://foo.bar/image.png").should == [100, 200] + end + + end + + it "caches the results" do + SiteSetting.stubs(:crawl_images?).returns(true) + FastImage.expects(:size).returns([200, 400]) + cpp.get_size("http://foo.bar/image.png") + cpp.get_size("http://foo.bar/image.png").should == [200, 400] + end + + end + + context "is_valid_image_uri?" do + + let(:post) { build(:post) } + let(:cpp) { CookedPostProcessor.new(post) } + + it "needs the scheme to be either http or https" do + cpp.is_valid_image_uri?("http://domain.com").should == true + cpp.is_valid_image_uri?("https://domain.com").should == true + cpp.is_valid_image_uri?("ftp://domain.com").should == false + cpp.is_valid_image_uri?("ftps://domain.com").should == false + cpp.is_valid_image_uri?("//domain.com").should == false + cpp.is_valid_image_uri?("/tmp/image.png").should == false + end + + it "doesn't throw an exception with a bad URI" do + cpp.is_valid_image_uri?("http://do -![Alt text](/second_image.jpg) - " -end - -Fabricator(:post_with_image_url, from: :post) do - cooked " - - " -end - -Fabricator(:post_with_s3_image_url, from: :post) do - cooked " - - " -end - -Fabricator(:post_with_uploads, from: :post) do - cooked " - - " -end - -Fabricator(:post_with_oneboxed_image, from: :post) do - cooked " -
- -
- " -end - - Fabricator(:basic_reply, from: :post) do user(:coding_horror) reply_to_post_number 1 @@ -71,6 +36,35 @@ Fabricator(:reply, from: :post) do ' end +Fabricator(:post_with_images_in_quote_and_onebox, from: :post) do + cooked ' + +
+' +end + +Fabricator(:post_with_uploaded_images, from: :post) do + cooked ' + + +' +end + +Fabricator(:post_with_unsized_images, from: :post) do + cooked ' + + +' +end + +Fabricator(:post_with_image_url, from: :post) do + cooked '' +end + +Fabricator(:post_with_large_image, from: :post) do + cooked '' +end + Fabricator(:post_with_external_links, from: :post) do user topic diff --git a/spec/models/optimized_image_spec.rb b/spec/models/optimized_image_spec.rb index bed715a7c7f..2d2cd0c1ae5 100644 --- a/spec/models/optimized_image_spec.rb +++ b/spec/models/optimized_image_spec.rb @@ -10,20 +10,19 @@ describe OptimizedImage do describe ".create_for" do before(:each) do - ImageSorcery.any_instance.stubs(:convert).returns(true) - FastImage.any_instance.stubs(:size).returns([244, 66]) + ImageSorcery.any_instance.expects(:convert).returns(true) # make sure we don't hit the filesystem FileUtils.stubs(:mkdir_p) File.stubs(:open) end it "works" do - Tempfile.any_instance.expects(:close).once - Tempfile.any_instance.expects(:unlink).once + Tempfile.any_instance.expects(:close) + Tempfile.any_instance.expects(:unlink) oi.sha1.should == "da39a3ee5e6b4b0d3255bfef95601890afd80709" oi.extension.should == ".jpg" - oi.width.should == 244 - oi.height.should == 66 + oi.width.should == 100 + oi.height.should == 100 end end