Create versions lazily to reflect subclass configurations properly

Fixes #1957, Fixes #2619

features/step_definitions/general_steps.rb

@@ -45,7 +45,7 @@ def filename
 end
 
 Given /^that the version '(.*?)' has the store_dir overridden to '(.*?)'$/ do |version, store_dir|
-  @klass.versions[version.to_sym].class_eval do
+  @klass.version(version.to_sym) do
     define_method(:store_dir) do
       file_path(store_dir)
     end

lib/carrierwave/mount.rb

@@ -441,7 +441,6 @@ def build_uploader(uploader, column, &block)
       const_set("CarrierWave#{column.to_s.camelize}Uploader", uploader)
 
       uploader.class_eval(&block)
-      uploader.recursively_apply_block_to_versions(&block)
 
       uploader
     end

lib/carrierwave/uploader/versions.rb

@@ -1,6 +1,71 @@
 module CarrierWave
   module Uploader
     module Versions
+      class Builder
+        def initialize(name)
+          @name = name
+          @options = {}
+          @blocks = []
+          @klass = nil
+        end
+
+        def configure(options, &block)
+          @options.merge!(options)
+          @blocks << block if block
+          @klass = nil
+        end
+
+        def build(superclass)
+          return @klass if @klass
+          @klass = Class.new(superclass)
+          superclass.const_set("#{@name.to_s.camelize}VersionUploader", @klass)
+
+          @klass.version_names += [@name]
+          @klass.versions = {}
+          @klass.processors = []
+          @klass.version_options = @options
+          @klass.class_eval <<-RUBY, __FILE__, __LINE__ + 1
+            # Regardless of what is set in the parent uploader, do not enforce the
+            # move_to_cache config option on versions because it moves the original
+            # file to the version's target file.
+            #
+            # If you want to enforce this setting on versions, override this method
+            # in each version:
+            #
+            # version :thumb do
+            #   def move_to_cache
+            #     true
+            #   end
+            # end
+            #
+            def move_to_cache
+              false
+            end
+          RUBY
+          @blocks.each { |block| @klass.class_eval(&block) }
+          @klass
+        end
+
+        def deep_dup
+          other = dup
+          other.instance_variable_set(:@blocks, @blocks.dup)
+          other
+        end
+
+        def method_missing(name, *args)
+          super
+        rescue NoMethodError => e
+          raise e.exception <<~ERROR
+            #{e.message}
+            If you're trying to configure a version, do it inside a block like `version(:thumb) { self.#{name} #{args.map(&:inspect).join(', ')} }`.
+          ERROR
+        end
+
+        def respond_to_missing?(*)
+          super
+        end
+      end
+
       extend ActiveSupport::Concern
 
       include CarrierWave::Uploader::Callbacks
@@ -58,79 +123,25 @@ module ClassMethods
         #
         def version(name, options = {}, &block)
           name = name.to_sym
-          build_version(name, options)
+          versions[name] ||= Builder.new(name)
+          versions[name].configure(options, &block)
 
-          versions[name].class_eval(&block) if block
-          versions[name]
-        end
+          class_eval <<-RUBY, __FILE__, __LINE__ + 1
+            def #{name}
+              versions[:#{name}]
+            end
+          RUBY
 
-        def recursively_apply_block_to_versions(&block)
-          versions.each do |name, version|
-            version.class_eval(&block)
-            version.recursively_apply_block_to_versions(&block)
-          end
+          versions[name]
         end
 
       private
 
-        def build_version(name, options)
-          class_name = "#{name.to_s.camelize}VersionUploader"
-
-          if !versions.has_key?(name)
-            uploader = Class.new(self)
-            const_set(class_name, uploader)
-            uploader.version_names += [name]
-            uploader.versions = {}
-            uploader.processors = []
-            uploader.version_options = options
-
-            uploader.class_eval <<-RUBY, __FILE__, __LINE__ + 1
-              # Define the enable_processing method for versions so they get the
-              # value from the parent class unless explicitly overwritten
-              def self.enable_processing(value=nil)
-                self.enable_processing = value if value
-                if defined?(@enable_processing) && !@enable_processing.nil?
-                  @enable_processing
-                else
-                  superclass.enable_processing
-                end
-              end
-
-              # Regardless of what is set in the parent uploader, do not enforce the
-              # move_to_cache config option on versions because it moves the original
-              # file to the version's target file.
-              #
-              # If you want to enforce this setting on versions, override this method
-              # in each version:
-              #
-              # version :thumb do
-              #   def move_to_cache
-              #     true
-              #   end
-              # end
-              #
-              def move_to_cache
-                false
-              end
-            RUBY
-
-            class_eval <<-RUBY, __FILE__, __LINE__ + 1
-              def #{name}
-                versions[:#{name}]
-              end
-            RUBY
-          else
-            parent = versions[name]
-            uploader = Class.new(parent)
-            parent.const_set(class_name, uploader)
-            uploader.processors = []
-            uploader.version_options = uploader.version_options.merge(options)
-          end
-
-          # Add the current version hash to class attribute :versions
-          self.versions = versions.merge(name => uploader)
+        def inherited(subclass)
+          # To prevent subclass version changes affecting superclass versions
+          subclass.versions = versions.deep_dup
+          super
         end
-
       end # ClassMethods
 
       ##
@@ -144,7 +155,7 @@ def versions
         return @versions if @versions
         @versions = {}
         self.class.versions.each do |name, version|
-          @versions[name] = version.new(model, mounted_as)
+          @versions[name] = version.build(self.class).new(model, mounted_as)
           @versions[name].parent_version = self
         end
         @versions
@@ -172,10 +183,10 @@ def version_name
       def version_exists?(name)
         name = name.to_sym
 
-        return false unless self.class.versions.has_key?(name)
+        return false unless versions.has_key?(name)
 
-        if_condition = self.class.versions[name].version_options[:if]
-        unless_condition = self.class.versions[name].version_options[:unless]
+        if_condition = versions[name].class.version_options[:if]
+        unless_condition = versions[name].class.version_options[:unless]
 
         if if_condition
           if if_condition.respond_to?(:call)

spec/mount_multiple_spec.rb

@@ -118,11 +118,11 @@ def rotate
 
         it { expect(uploader_2.processors).to eq([[:rotate, [], nil, nil], [:shrink, [], nil, nil]]) }
 
-        it { expect(uploader_1.versions[:thumb].processors).to eq([[:compress, [], nil, nil]]) }
+        it { expect(uploader_1.new.versions[:thumb].processors).to eq([[:compress, [], nil, nil]]) }
 
-        it { expect(uploader_2.versions[:thumb].processors).to eq([[:compress, [], nil, nil]]) }
+        it { expect(uploader_2.new.versions[:thumb].processors).to eq([[:compress, [], nil, nil]]) }
 
-        it { expect(uploader_2.versions[:secret].processors).to eq([[:encrypt, [], nil, nil]]) }
+        it { expect(uploader_2.new.versions[:secret].processors).to eq([[:encrypt, [], nil, nil]]) }
       end
     end
 

spec/mount_single_spec.rb

@@ -86,9 +86,9 @@ def rotate
       it "should inherit defined processors properly" do
         expect(@uploader1.processors).to eq([[:rotate, [], nil, nil]])
         expect(@uploader2.processors).to eq([[:rotate, [], nil, nil], [:shrink, [], nil, nil]])
-        expect(@uploader1.versions[:thumb].processors).to eq([[:compress, [], nil, nil]])
-        expect(@uploader2.versions[:thumb].processors).to eq([[:compress, [], nil, nil]])
-        expect(@uploader2.versions[:secret].processors).to eq([[:encrypt, [], nil, nil]])
+        expect(@uploader1.new.versions[:thumb].processors).to eq([[:compress, [], nil, nil]])
+        expect(@uploader2.new.versions[:thumb].processors).to eq([[:compress, [], nil, nil]])
+        expect(@uploader2.new.versions[:secret].processors).to eq([[:encrypt, [], nil, nil]])
       end
     end