ActiveSupport::Concern included hook errors if Virtus.module is included before

[adamcooper]

If you include the Virtus.module before an ActiveSupport::Concern included hook it generates an exception.

Test Script

require 'virtus'
require 'active_support'

begin
  puts "Loading Module with error"
  module FormObject
    extend ActiveSupport::Concern
    include Virtus.module

    included do
      extend ActiveModel::Naming
      include ActiveModel::Conversion, ActiveModel::Validations
    end
  end
  puts "Finished loading module with error - you shouldn't see this!"
rescue StandardError => ex
  puts ex.message
  puts ex.backtrace
end

puts "\n\nLoading Module without error"
module FixedFormObject
  extend ActiveSupport::Concern

  included do
    extend ActiveModel::Naming
    include ActiveModel::Conversion, ActiveModel::Validations
  end

  include Virtus.module
end
puts "Finished loading module without error"

Ouptut

ruby lib/test.rb
Loading Module with error
wrong number of arguments (0 for 1)
/usr/local/opt/rbenv/versions/1.9.3-p448/lib/ruby/gems/1.9.1/bundler/gems/virtus-a9011d0b31fc/lib/virtus/module_extensions.rb:56:in `included'
lib/test.rb:10:in `<module:FormObject>'
lib/test.rb:6:in `<main>'

Loading Module without error
Finished loading module without error

[solnic]

I think this is actually a bug in ActiveSupport. Its Concern extension changes signature of module's included hook because it uses it with the block and doesn't expect an argument. Virtus defines included method too and calls super so if you include virtus module before concern stuff it will blow up when calling super with an argument because that's what Concern's included doesn't expect to receive.

[solnic]

This is weird but it looks like it actually should work when calling super: https://github.com/rails/rails/blob/4-0-stable/activesupport/lib/active_support/concern.rb#L118

I will investigate further...

EDIT: actually no it shouldn't because of what I wrote in my first comment :)

[solnic]

So, I must say I'm torn. My pragmatic self says "go and work around it" and my purist self says "go report an issue in rails and complain". Need to think about this a bit more.

[dkubb]

@solnic the question probably comes down to: Is virtus exposing a bug in activesupport, or is it the other way around?

If it's the former, and virtus is using the standard interfaces as intended, then a bug needs to be reported to activesupport. Virtus does not have any responsibility to work-around bugs in other libraries outside of it's explicit dependencies and ruby core/stdlib.

If it's the latter, and virtus is doing something that doesn't match the built-in interfaces, then it should be fixed in virtus. AS merely exposed a latent bug that would have affected virtus at some point later on anyway.

[solnic]

@dkubb ActiveSupport::Concern changes the signature of Module#included so I'm pretty much confident virtus should not be changed to workaround a design mistake in AS.

[mbj]

@solnic, @dkubb As I read the AS and virtus code:

Its the fault of AS. The interface Module#included(descendant) is in the ruby corelib. descendant is a required parameter. Once AS infects a module it changes the modules interface in an incompatible way. Client code of AS relies on this incompatible change. And virtus does not support that incompatible change. And it should not.

[solnic]

Yeah exactly @mbj

[dkubb]

@solnic @mbj agreed. It is not the responsibility of other libraries to cater to incompatible changes in AS. It is AS' responsibility to follow the standard interfaces or risk interop problems like this.

I would likely mark this issue as closed and ask that it be submitted to the AS issue tracker.

[solnic]

OK, I don't think I need any more arguments :) Thanks for chiming in guys.

I'm gonna close this issue, please report it on rails issue tracker.