Fix link tag removal under Hpricot

Closes #149, #152

CHANGELOG.md

@@ -3,6 +3,7 @@
 ## HEAD
 
 - Add support for rails' `relative_url_root` config
+- Fix link tag removal under Hpricot
 
 ## v1.9.2
 

lib/premailer/rails/css_helper.rb

@@ -27,7 +27,11 @@ def css_for_url(url)
 
       def css_urls_in_doc(doc)
         doc.search('link[@rel="stylesheet"]:not([@data-premailer="ignore"])').map do |link|
-          link.remove
+          if link.respond_to?(:remove)
+            link.remove
+          else
+            link.parent.children.delete(link)
+          end
           link.attributes['href'].to_s
         end
       end

spec/integration/css_helper_spec.rb

@@ -1,180 +1,186 @@
 require 'spec_helper'
 
 describe Premailer::Rails::CSSHelper do
-  # Reset the CSS cache:
-  after do
-    Premailer::Rails::CSSLoaders::CacheLoader.clear!
-  end
-
-  def css_for_url(path)
-    Premailer::Rails::CSSHelper.css_for_url(path)
-  end
+  [ :Nokogiri, :Hpricot ].each do |adapter|
+    next if adapter == :Hpricot and RUBY_PLATFORM == 'java'
 
-  def css_for_doc(doc)
-    Premailer::Rails::CSSHelper.css_for_doc(doc)
-  end
+    context "when adapter is #{adapter}" do
+      # Reset the CSS cache:
+      after do
+        Premailer::Rails::CSSLoaders::CacheLoader.clear!
+      end
 
-  def expect_file(path, content='file content')
-    allow(File).to receive(:exist?).with(path).and_return(true)
-    expect(File).to receive(:read).with(path).and_return(content)
-  end
+      def css_for_url(path)
+        Premailer::Rails::CSSHelper.css_for_url(path)
+      end
 
-  describe '#css_for_doc' do
-    let(:html) { Fixtures::HTML.with_css_links(*files) }
-    let(:doc) { Nokogiri(html) }
+      def css_for_doc(doc)
+        Premailer::Rails::CSSHelper.css_for_doc(doc)
+      end
 
-    context 'when HTML contains linked CSS files' do
-      let(:files) { %w[ stylesheets/base.css stylesheets/font.css ] }
+      def expect_file(path, content='file content')
+        allow(File).to receive(:exist?).with(path).and_return(true)
+        expect(File).to receive(:read).with(path).and_return(content)
+      end
 
-      it 'returns the content of both files concatenated' do
-        allow(Premailer::Rails::CSSHelper).to \
-          receive(:css_for_url)
-            .with('http://example.com/stylesheets/base.css')
-            .and_return('content of base.css')
-        allow(Premailer::Rails::CSSHelper).to \
-          receive(:css_for_url)
-            .with('http://example.com/stylesheets/font.css')
-            .and_return('content of font.css')
+      describe '#css_for_doc' do
+        let(:html) { Fixtures::HTML.with_css_links(*files) }
+        let(:doc) { send(adapter, html) }
 
-        expect(css_for_doc(doc)).to eq("content of base.css\ncontent of font.css")
-      end
-    end
+        context 'when HTML contains linked CSS files' do
+          let(:files) { %w[ stylesheets/base.css stylesheets/font.css ] }
 
-    context 'when HTML contains ignored links' do
-      let(:files) { ['ignore.css', 'data-premailer' => 'ignore'] }
+          it 'returns the content of both files concatenated' do
+            allow(Premailer::Rails::CSSHelper).to \
+              receive(:css_for_url)
+                .with('http://example.com/stylesheets/base.css')
+                .and_return('content of base.css')
+            allow(Premailer::Rails::CSSHelper).to \
+              receive(:css_for_url)
+                .with('http://example.com/stylesheets/font.css')
+                .and_return('content of font.css')
 
-      it 'ignores links' do
-        expect(Premailer::Rails::CSSHelper).to_not receive(:css_for_url)
-        css_for_doc(doc)
-      end
-    end
-  end
+            expect(css_for_doc(doc)).to eq("content of base.css\ncontent of font.css")
+          end
+        end
 
-  describe '#css_for_url' do
-    context 'when path is a url' do
-      it 'loads the CSS at the local path' do
-        expect_file('public/stylesheets/base.css')
+        context 'when HTML contains ignored links' do
+          let(:files) { ['ignore.css', 'data-premailer' => 'ignore'] }
 
-        css_for_url('http://example.com/stylesheets/base.css?test')
+          it 'ignores links' do
+            expect(Premailer::Rails::CSSHelper).to_not receive(:css_for_url)
+            css_for_doc(doc)
+          end
+        end
       end
-    end
 
-    context 'when path is a relative url' do
-      it 'loads the CSS at the local path' do
-        expect_file('public/stylesheets/base.css')
-        css_for_url('/stylesheets/base.css?test')
-      end
-    end
+      describe '#css_for_url' do
+        context 'when path is a url' do
+          it 'loads the CSS at the local path' do
+            expect_file('public/stylesheets/base.css')
 
-    context 'when file is cached' do
-      it 'returns the cached value' do
-        Premailer::Rails::CSSLoaders::CacheLoader.store(
-          'http://example.com/stylesheets/base.css',
-          'content of base.css'
-        )
+            css_for_url('http://example.com/stylesheets/base.css?test')
+          end
+        end
 
-        expect(css_for_url('http://example.com/stylesheets/base.css')).to \
-          eq('content of base.css')
-      end
-    end
+        context 'when path is a relative url' do
+          it 'loads the CSS at the local path' do
+            expect_file('public/stylesheets/base.css')
+            css_for_url('/stylesheets/base.css?test')
+          end
+        end
 
-    context 'when in development mode' do
-      it 'does not return cached values' do
-        Premailer::Rails::CSSLoaders::CacheLoader.store(
-          'http://example.com/stylesheets/base.css',
-          'cached content of base.css'
-        )
-        content = 'new content of base.css'
-        expect_file('public/stylesheets/base.css', content)
-        allow(Rails.env).to receive(:development?).and_return(true)
-
-        expect(css_for_url('http://example.com/stylesheets/base.css')).to eq(content)
-      end
-    end
+        context 'when file is cached' do
+          it 'returns the cached value' do
+            Premailer::Rails::CSSLoaders::CacheLoader.store(
+              'http://example.com/stylesheets/base.css',
+              'content of base.css'
+            )
 
-    context 'when Rails asset pipeline is used' do
-      before do
-        allow(Rails.configuration)
-          .to receive(:assets).and_return(double(prefix: '/assets'))
-        allow(Rails.configuration)
-          .to receive(:relative_url_root).and_return(nil)
-      end
+            expect(css_for_url('http://example.com/stylesheets/base.css')).to \
+              eq('content of base.css')
+          end
+        end
 
-      context 'and a precompiled file exists' do
-        it 'returns that file' do
-          path = '/assets/email-digest.css'
-          content = 'read from file'
-          expect_file("public#{path}", content)
-          expect(css_for_url(path)).to eq(content)
+        context 'when in development mode' do
+          it 'does not return cached values' do
+            Premailer::Rails::CSSLoaders::CacheLoader.store(
+              'http://example.com/stylesheets/base.css',
+              'cached content of base.css'
+            )
+            content = 'new content of base.css'
+            expect_file('public/stylesheets/base.css', content)
+            allow(Rails.env).to receive(:development?).and_return(true)
+
+            expect(css_for_url('http://example.com/stylesheets/base.css')).to eq(content)
+          end
         end
-      end
 
-      it 'returns the content of the file compiled by Rails' do
-        expect(Rails.application.assets).to \
-          receive(:find_asset)
-            .with('base.css')
-            .and_return(double(to_s: 'content of base.css'))
+        context 'when Rails asset pipeline is used' do
+          before do
+            allow(Rails.configuration)
+              .to receive(:assets).and_return(double(prefix: '/assets'))
+            allow(Rails.configuration)
+              .to receive(:relative_url_root).and_return(nil)
+          end
 
-        expect(css_for_url('http://example.com/assets/base.css')).to \
-          eq('content of base.css')
-      end
+          context 'and a precompiled file exists' do
+            it 'returns that file' do
+              path = '/assets/email-digest.css'
+              content = 'read from file'
+              expect_file("public#{path}", content)
+              expect(css_for_url(path)).to eq(content)
+            end
+          end
 
-      it 'returns same file when path contains file fingerprint' do
-        expect(Rails.application.assets).to \
-          receive(:find_asset)
-            .with('base.css')
-            .and_return(double(to_s: 'content of base.css'))
+          it 'returns the content of the file compiled by Rails' do
+            expect(Rails.application.assets).to \
+              receive(:find_asset)
+                .with('base.css')
+                .and_return(double(to_s: 'content of base.css'))
 
-        expect(css_for_url(
-          'http://example.com/assets/base-089e35bd5d84297b8d31ad552e433275.css'
-        )).to eq('content of base.css')
-      end
+            expect(css_for_url('http://example.com/assets/base.css')).to \
+              eq('content of base.css')
+          end
 
-      context 'when asset can not be found' do
-        let(:response) { 'content of base.css' }
-        let(:path) { '/assets/base-089e35bd5d84297b8d31ad552e433275.css' }
-        let(:url) { "http://assets.example.com#{path}" }
-        let(:asset_host) { 'http://assets.example.com' }
+          it 'returns same file when path contains file fingerprint' do
+            expect(Rails.application.assets).to \
+              receive(:find_asset)
+                .with('base.css')
+                .and_return(double(to_s: 'content of base.css'))
 
-        before do
-          allow(Rails.application.assets).to \
-            receive(:find_asset).and_return(nil)
+            expect(css_for_url(
+              'http://example.com/assets/base-089e35bd5d84297b8d31ad552e433275.css'
+            )).to eq('content of base.css')
+          end
 
-          config = double(asset_host: asset_host)
-          allow(Rails.configuration).to \
-            receive(:action_controller).and_return(config)
+          context 'when asset can not be found' do
+            let(:response) { 'content of base.css' }
+            let(:path) { '/assets/base-089e35bd5d84297b8d31ad552e433275.css' }
+            let(:url) { "http://assets.example.com#{path}" }
+            let(:asset_host) { 'http://assets.example.com' }
 
-          uri_satisfaction = satisfy { |uri| uri.to_s == url }
-          allow(Net::HTTP).to \
-            receive(:get).with(uri_satisfaction).and_return(response)
-        end
+            before do
+              allow(Rails.application.assets).to \
+                receive(:find_asset).and_return(nil)
 
-        it 'requests the file' do
-          expect(css_for_url(url)).to eq('content of base.css')
-        end
+              config = double(asset_host: asset_host)
+              allow(Rails.configuration).to \
+                receive(:action_controller).and_return(config)
 
-        context 'when file url does not include the host' do
-          it 'requests the file using the asset host as host' do
-            expect(css_for_url(path)).to eq('content of base.css')
-          end
+              uri_satisfaction = satisfy { |uri| uri.to_s == url }
+              allow(Net::HTTP).to \
+                receive(:get).with(uri_satisfaction).and_return(response)
+            end
 
-          context 'and the asset host uses protocol relative scheme' do
-            let(:asset_host) { '//assets.example.com' }
+            it 'requests the file' do
+              expect(css_for_url(url)).to eq('content of base.css')
+            end
 
-            it 'requests the file using http as the scheme' do
-              expect(css_for_url(path)).to eq('content of base.css')
+            context 'when file url does not include the host' do
+              it 'requests the file using the asset host as host' do
+                expect(css_for_url(path)).to eq('content of base.css')
+              end
+
+              context 'and the asset host uses protocol relative scheme' do
+                let(:asset_host) { '//assets.example.com' }
+
+                it 'requests the file using http as the scheme' do
+                  expect(css_for_url(path)).to eq('content of base.css')
+                end
+              end
             end
           end
         end
-      end
-    end
 
-    context 'when static stylesheets are used' do
-      it 'returns the content of the static file' do
-        content = 'content of base.css'
-        expect_file('public/stylesheets/base.css', content)
-        loaded_content = css_for_url('http://example.com/stylesheets/base.css')
-        expect(loaded_content).to eq(content)
+        context 'when static stylesheets are used' do
+          it 'returns the content of the static file' do
+            content = 'content of base.css'
+            expect_file('public/stylesheets/base.css', content)
+            loaded_content = css_for_url('http://example.com/stylesheets/base.css')
+            expect(loaded_content).to eq(content)
+          end
+        end
       end
     end
   end