cleaned up CookedPostProcessor and improved specs

lib/cooked_post_processor.rb

@@ -12,52 +12,34 @@ class CookedPostProcessor
     @post = post
     @doc = Nokogiri::HTML::fragment(post.cooked)
     @size_cache = {}
+    @has_been_uploaded_cache = {}
   end
 
-  def dirty?
+  def post_process
-    @dirty
+    return unless @doc.present?
+    post_process_images
+    post_process_oneboxes
   end
 
-  # Bake onebox content into the post
-  def post_process_oneboxes
-    args = {post_id: @post.id}
-    args[:invalidate_oneboxes] = true if @opts[:invalidate_oneboxes]
-
-    result = Oneboxer.apply(@doc) do |url, element|
-      Oneboxer.onebox(url, args)
-    end
-    @dirty ||= result.changed?
-  end
-
-  # First let's consider the images
   def post_process_images
     images = @doc.search("img")
     return unless images.present?
 
     images.each do |img|
+      # keep track of the src
       src = img['src']
-      src = Discourse.base_url_no_prefix + src if 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?
-
+        # update img dimensions if at least one is missing
-        if img['width'].blank? || img['height'].blank?
+        update_dimensions!(img)
-          w, h = get_size_from_image_sizes(src, @opts[:image_sizes]) || image_dimensions(src)
+        # optimize image
-
-          if w && h
-            img['width'] = w.to_s
-            img['height'] = h.to_s
-            @dirty = true
-          end
-        end
-
-        if src != img['src']
-          img['src'] = src
-          @dirty = true
-        end
-
-        convert_to_link!(img)
         img['src'] = optimize_image(img)
-
+        # lightbox treatment
+        convert_to_link!(img)
+        # mark the post as dirty whenever the src has changed
+        @dirty |= src != img['src']
       end
     end
 
@@ -69,35 +51,32 @@ class CookedPostProcessor
 
   end
 
+  def post_process_oneboxes
+    args = { post_id: @post.id }
+    args[:invalidate_oneboxes] = true if @opts[:invalidate_oneboxes]
+    # bake onebox content into the post
+    result = Oneboxer.apply(@doc) do |url, element|
+      Oneboxer.onebox(url, args)
+    end
+    # mark the post as dirty whenever a onebox as been baked
+    @dirty |= result.changed?
+  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
+      @dirty = true
+    end
+  end
+
   def optimize_image(img)
-    src = img["src"]
+    return img["src"]
-    return src
+    # TODO: needs some <3
-
-    # implementation notes: Sam
-    #
-    # I have disabled this for now, would like the following addressed.
-    #
-    # 1. We need a db record pointing the files on the file system to the post they are on,
-    #   if we do not do that we have no way of purging any local optimised copies
-    #
-    # 2. We should be storing images in /uploads/site-name/_optimised ... it simplifies configuration
-    #
-    # 3. I don't want to have a folder with 10 million images, let split it so /uploads/site-name/_optimised/ABC/DEF/AAAAAAAA.jpg
-    #
-    # 4. We shoul confirm that that we test both saving as jpg and png and pick the more efficient format ... tricky to get right
-    #
-    # 5. All images should also be optimised using image_optim, it ensures that best compression is used
-    #
-    # 6. Admin screen should alert users of any missing dependencies (image magick, etc, and explain what it is for)
-    #
-    # 7. Optimise images should be a seperate site setting.
-
-    # supports only local uploads
-    return src if SiteSetting.enable_s3_uploads?
-
-    width, height = img["width"].to_i, img["height"].to_i
-
-    ImageOptimizer.new(src).optimized_image_url(width, height)
   end
 
   def convert_to_link!(img)
@@ -135,52 +114,44 @@ class CookedPostProcessor
     end
   end
 
-  def post_process
+  # Retrieve the image dimensions for a url
-    return unless @doc.present?
+  def image_dimensions(url)
-    post_process_images
+    uri = get_image_uri(url)
-    post_process_oneboxes
+    return unless uri
+    w, h = get_size(url)
+    ImageSizer.resize(w, h) if w && h
+  end
+
+  def get_size(url)
+    # we can always crawl our own images
+    return unless SiteSetting.crawl_images? || has_been_uploaded?(url)
+    @size_cache[url] ||= FastImage.size(url)
+  rescue Zlib::BufError # FastImage.size raises BufError for some gifs
+  end
+
+  def get_image_uri(url)
+    uri = URI.parse(url)
+    uri if %w(http https).include?(uri.scheme)
+  end
+
+  def has_been_uploaded?(url)
+    @has_been_uploaded_cache[url] ||= url.start_with?(base_url)
+  end
+
+  def base_url
+    asset_host.present? ? asset_host : Discourse.base_url_no_prefix
+  end
+
+  def asset_host
+    ActionController::Base.asset_host
+  end
+
+  def dirty?
+    @dirty
   end
 
   def html
     @doc.try(:to_html)
   end
 
-  def doc
-    @doc
-  end
-
-  def get_size(url)
-    # we need to find out whether it's an external image or an uploaded one
-    # an external image would be: http://google.com/logo.png
-    # an uploaded image would be: http://my.discourse.com/uploads/default/12345.png or http://my.cdn.com/uploads/default/12345.png
-    uri = url
-    # this will transform `http://my.discourse.com/uploads/default/12345.png` into a local uri
-    uri = "#{Rails.root}/public#{url[Discourse.base_url.length..-1]}" if url.start_with?(Discourse.base_url)
-    # this will do the same but when CDN has been defined in the configuration
-    uri = "#{Rails.root}/public#{url[ActionController::Base.asset_host.length..-1]}" if ActionController::Base.asset_host && url.start_with?(ActionController::Base.asset_host)
-    # return nil when it's an external image *and* crawling is disabled
-    return nil unless SiteSetting.crawl_images? || uri[0] == "/"
-    @size_cache[uri] ||= FastImage.size(uri)
-  rescue Zlib::BufError
-    # FastImage.size raises BufError for some gifs
-    return nil
-  end
-
-  def get_image_uri(url)
-    uri = URI.parse(url)
-    if %w(http https).include?(uri.scheme)
-      uri
-    else
-      nil
-    end
-  end
-
-  # Retrieve the image dimensions for a url
-  def image_dimensions(url)
-    uri = get_image_uri(url)
-    return nil unless uri
-    w, h = get_size(url)
-    ImageSizer.resize(w, h) if w && h
-  end
-
 end

lib/image_sizer.rb

@@ -3,7 +3,7 @@ module ImageSizer
   # Resize an image to the aspect ratio we want
   def self.resize(width, height)
     max_width = SiteSetting.max_image_width.to_f
-    return nil if width.blank? || height.blank?
+    return if width.blank? || height.blank?
 
     w = width.to_f
     h = height.to_f

lib/pretty_text.rb

@@ -151,20 +151,18 @@ module PrettyText
   def self.apply_cdn(html, url)
     return html unless url
 
-    image = /\.(jpg|jpeg|gif|png|tiff|tif)$/
+    image = /\.(jpg|jpeg|gif|png|tiff|tif|bmp)$/
 
     doc = Nokogiri::HTML.fragment(html)
+
     doc.css("a").each do |l|
-      href = l.attributes["href"].to_s
+      href = l["href"].to_s
-      if href[0] == '/' && href =~ image
+      l["href"] = url + href if href[0] == '/' && href =~ image
-        l["href"] = url + href
-      end
     end
+
     doc.css("img").each do |l|
-      src = l.attributes["src"].to_s
+      src = l["src"].to_s
-      if src[0] == '/'
+      l["src"] = url + src if src[0] == '/'
-        l["src"] = url + src
-      end
     end
 
     doc.to_s

spec/components/cooked_post_processor_spec.rb

@@ -41,7 +41,6 @@ describe CookedPostProcessor do
       before do
         @topic = Fabricate(:topic)
         @post = Fabricate.build(:post_with_image_url, topic: @topic, user: @topic.user)
-        ImageSorcery.any_instance.stubs(:convert).returns(false)
         @cpp = CookedPostProcessor.new(@post, image_sizes: {'http://www.forumwarz.com/images/header/logo.png' => {'width' => 111, 'height' => 222}})
         @cpp.expects(:get_size).returns([111,222])
       end
@@ -65,7 +64,6 @@ describe CookedPostProcessor do
       before do
         FastImage.stubs(:size).returns([123, 456])
         ImageSorcery.any_instance.stubs(:convert).returns(false)
-        CookedPostProcessor.any_instance.expects(:image_dimensions).returns([123, 456])
         creator = PostCreator.new(user, raw: Fabricate.build(:post_with_images).raw, topic_id: topic.id)
         @post = creator.create
       end
@@ -89,7 +87,6 @@ describe CookedPostProcessor do
       let(:processor) { CookedPostProcessor.new(post) }
 
       before do
-        ImageSorcery.any_instance.stubs(:convert).returns(false)
         processor.post_process_images
       end
 
@@ -151,4 +148,35 @@ describe CookedPostProcessor do
     end
   end
 
+  context 'get_image_uri' do
+
+    it "returns nil unless the scheme is either http or https" do
+      cpp.get_image_uri("http://domain.com").should   == URI.parse("http://domain.com")
+      cpp.get_image_uri("https://domain.com").should  == URI.parse("https://domain.com")
+      cpp.get_image_uri("ftp://domain.com").should    == nil
+      cpp.get_image_uri("ftps://domain.com").should   == nil
+      cpp.get_image_uri("//domain.com").should        == nil
+      cpp.get_image_uri("/tmp/image.png").should      == nil
+    end
+
+  end
+
+  context 'has_been_uploaded?' do
+
+    it "identifies internal urls" do
+      Discourse.expects(:base_url_no_prefix).returns("http://my.discourse.com")
+      cpp.has_been_uploaded?("http://my.discourse.com").should == true
+    end
+
+    it "identifies internal urls when using a CDN" do
+      ActionController::Base.expects(:asset_host).returns("http://my.cdn.com").twice
+      cpp.has_been_uploaded?("http://my.cdn.com").should == true
+    end
+
+    it "identifies external urls" do
+      cpp.has_been_uploaded?("http://domain.com").should == false
+    end
+
+  end
+
 end

spec/models/post_action_spec.rb

@@ -2,11 +2,6 @@ require 'spec_helper'
 require_dependency 'post_destroyer'
 
 describe PostAction do
-
-  before do
-    ImageSorcery.any_instance.stubs(:convert).returns(false)
-  end
-
   it { should belong_to :user }
   it { should belong_to :post }
   it { should belong_to :post_action_type }

spec/models/post_spec.rb

@@ -2,11 +2,6 @@ require 'spec_helper'
 require_dependency 'post_destroyer'
 
 describe Post do
-
-  before do
-    ImageSorcery.any_instance.stubs(:convert).returns(false)
-  end
-
   # Help us build a post with a raw body
   def post_with_body(body, user=nil)
     args = post_args.merge(raw: body)