joining on instance depedent associations should not be deprecated, they can handle joins and instance invokation

[bughit]

begin
  require 'bundler/inline'
rescue LoadError => e
  $stderr.puts 'Bundler version 1.10 or later is required. Please update your Bundler'
  raise e
end

gemfile(true) do
  source 'https://rubygems.org'
  # Activate the gem you are reporting the issue against.
  gem 'activerecord', '4.2.5'
  gem 'sqlite3'
end

require 'active_record'
require 'minitest/autorun'
require 'logger'

# Ensure backward compatibility with Minitest 4
Minitest::Test = MiniTest::Unit::TestCase unless defined?(Minitest::Test)

# This connection will do for database-independent bug reports.
ActiveRecord::Base.establish_connection(adapter: 'sqlite3', database: ':memory:')
ActiveRecord::Base.logger = Logger.new(STDOUT)

ActiveRecord::Schema.define do

  create_table :posts

  create_table :comments do |t|
    t.belongs_to :commentable, polymorphic: true
  end

end

class Comment < ActiveRecord::Base
  belongs_to :commentable, polymorphic: true
  belongs_to :post, -> owner do

    if owner.is_a?(Comment)
      # calling assoc
      owner.commentable_type == 'Post' ? Post.all : Post.none
    else
      # joining
      where(comments: {commentable_type: 'Post'})
    end

  end, foreign_key: :commentable_id
end

class Post < ActiveRecord::Base


  has_many :comments, as: :commentable
end



class BugTest < Minitest::Test
  def test_association_stuff
    c = Comment.create!
    p = Post.create! comments: [c]

    assert c.post
    assert_equal 1, Comment.joins(:post).count
  end
end

[rafaelfranca]

It is working only because you are handling the case where owner is not present. We don't think it is a good idea to join associations like this one so it we will keep it deprecated. Thank you

[bughit]

because you are handling the case where owner is not present

Yes, you have to handle both cases, but why is that a bad thing?

We don't think it is a good idea to join associations like this one

Why don't you think it's a good idea? The alternative is to create two associations, one that only works for joins and another that only works for instance invoke. Is that supposed to be better that one association that can do both?

Thank you

Is that a sincere thank you for creating a good bug report?

[rafaelfranca]

Yes, you have to handle both cases, but why is that a bad thing?

Because you would have to remember that.

Why don't you think it's a good idea?

The main reason it is because it is confusing, a user would have to know about this special case were the instance is not passed to the association. Other reason to deprecate it is because it is surprising.

The alternative is to create two associations, one that only works for joins and another that only works for instance invoke. Is that supposed to be better that one association that can do both?

Other alternative is doing the join in the table instead of in the association.

See the original issue for more background about the deprecation. #15024.

Is that a sincere thank you for creating a good bug report?

Of course 😄