New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce ApplicationRecord, an Active Record layer supertype #22567

Merged
merged 1 commit into from Dec 16, 2015

Conversation

Projects
None yet
9 participants
@gsamokovarov
Contributor

gsamokovarov commented Dec 12, 2015

It's pretty common for folks to monkey patch ActiveRecord::Base to
work around an issue or introduce extra functionality. Instead of
shoving even more stuff in ActiveRecord::Base, ApplicationRecord can
hold all those custom work the apps may need.

Now, we don't wanna encourage all of the application models to inherit
from ActiveRecord::Base, but we can encourage all the models that do,
to inherit from ApplicationRecord.

Newly generated applications have app/models/application_record.rb
present by default. The model generators are smart enough to recognize
that newly generated models have to inherit from ApplicationRecord,
but only if it's present.

@rails-bot

This comment has been minimized.

Show comment
Hide comment
@rails-bot

rails-bot Dec 12, 2015

r? @chancancode

(@rails-bot has picked a reviewer for you, use r? to override)

rails-bot commented Dec 12, 2015

r? @chancancode

(@rails-bot has picked a reviewer for you, use r? to override)

@gsamokovarov

This comment has been minimized.

Show comment
Hide comment
@gsamokovarov
Contributor

gsamokovarov commented Dec 12, 2015

@claudiob

This comment has been minimized.

Show comment
Hide comment
@claudiob

claudiob Dec 12, 2015

Member

Do we need to test the case in which a user creates a model called ApplicationRecord?

It might not be too far-fetched as an option. Maybe 'ApplicationRecord' can be included in the words that cannot be used as a name for a model. Or a warning could be displayed if such a model exists.

Member

claudiob commented Dec 12, 2015

Do we need to test the case in which a user creates a model called ApplicationRecord?

It might not be too far-fetched as an option. Maybe 'ApplicationRecord' can be included in the words that cannot be used as a name for a model. Or a warning could be displayed if such a model exists.

@gsamokovarov

This comment has been minimized.

Show comment
Hide comment
@gsamokovarov

gsamokovarov Dec 12, 2015

Contributor

Do we need to test the case in which a user creates a model called ApplicationRecord?

That may be an idea. Maybe we can warn on some "reserved" names in all the NamedBase generators. For example if you do bundle exec rails generate controller Application, you end up with:

class ApplicationController < ApplicationController
end
Contributor

gsamokovarov commented Dec 12, 2015

Do we need to test the case in which a user creates a model called ApplicationRecord?

That may be an idea. Maybe we can warn on some "reserved" names in all the NamedBase generators. For example if you do bundle exec rails generate controller Application, you end up with:

class ApplicationController < ApplicationController
end
@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Dec 12, 2015

Member

This is way too defensive to me. I don't think anyone would have a model
called ApplicationRecord

On Sat, Dec 12, 2015, 15:26 Genadi Samokovarov notifications@github.com
wrote:

Do we need to test the case in which a user creates a model called
ApplicationRecord?

That may be an idea. Maybe we can warn on some "reserved" names in all the
NamedBase generators. For example if you do bundle exec rails generate
controller Application, you end up with:

class ApplicationController < ApplicationControllerend


Reply to this email directly or view it on GitHub
#22567 (comment).

Member

rafaelfranca commented Dec 12, 2015

This is way too defensive to me. I don't think anyone would have a model
called ApplicationRecord

On Sat, Dec 12, 2015, 15:26 Genadi Samokovarov notifications@github.com
wrote:

Do we need to test the case in which a user creates a model called
ApplicationRecord?

That may be an idea. Maybe we can warn on some "reserved" names in all the
NamedBase generators. For example if you do bundle exec rails generate
controller Application, you end up with:

class ApplicationController < ApplicationControllerend


Reply to this email directly or view it on GitHub
#22567 (comment).

@claudiob

This comment has been minimized.

Show comment
Hide comment
@claudiob

claudiob Dec 13, 2015

Member

in that case, no objections 🎀

Member

claudiob commented Dec 13, 2015

in that case, no objections 🎀

Show outdated Hide outdated activerecord/CHANGELOG.md
It's pretty common for folks to monkey patch `ActiveRecord::Base` to
work around an issue or introduce extra functionality. Instead of
shoving even more stuff in `ActiveRecord::Base`, `ApplicationRecord` can
hold all those custom work the apps may need.

This comment has been minimized.

@kaspth

kaspth Dec 13, 2015

Member

I think we should rephrase the motivation. What I've understood to be the issue is that engines will force configuration changes onto an application, and that there's no way to keep them separate because both were configuring ActiveRecord::Base. That, in turn, results in monkey patches.

You are right about the convenience of where to include extra app wide record functionality.

cc @rafaelfranca

@kaspth

kaspth Dec 13, 2015

Member

I think we should rephrase the motivation. What I've understood to be the issue is that engines will force configuration changes onto an application, and that there's no way to keep them separate because both were configuring ActiveRecord::Base. That, in turn, results in monkey patches.

You are right about the convenience of where to include extra app wide record functionality.

cc @rafaelfranca

This comment has been minimized.

@gsamokovarov

gsamokovarov Dec 13, 2015

Contributor

Yes! We can and should advertise that to plugin authors. I have AR extensions myself, which I can distribute as modules to be mixed into ApplicationRecord instead of ActiveRecord::Base monkey patches.

@gsamokovarov

gsamokovarov Dec 13, 2015

Contributor

Yes! We can and should advertise that to plugin authors. I have AR extensions myself, which I can distribute as modules to be mixed into ApplicationRecord instead of ActiveRecord::Base monkey patches.

This comment has been minimized.

@rafaelfranca

rafaelfranca Dec 14, 2015

Member

Yes we need to rephrase the motivation to talk about engines.

@rafaelfranca

rafaelfranca Dec 14, 2015

Member

Yes we need to rephrase the motivation to talk about engines.

This comment has been minimized.

@kaspth

kaspth Dec 14, 2015

Member

My point wasn't whether or not we should advertise to engine authors. It was that the motivation stated here isn't why we want to add this class.

@kaspth

kaspth Dec 14, 2015

Member

My point wasn't whether or not we should advertise to engine authors. It was that the motivation stated here isn't why we want to add this class.

Show outdated Hide outdated activerecord/CHANGELOG.md
@@ -1,3 +1,17 @@
* Introduce ApplicationRecord, an Active Record layer supertype

This comment has been minimized.

@rafaelfranca

rafaelfranca Dec 14, 2015

Member

Missing . here.

@rafaelfranca

rafaelfranca Dec 14, 2015

Member

Missing . here.

@claudiob

This comment has been minimized.

Show comment
Hide comment
@claudiob

claudiob Dec 15, 2015

Member

As part of this commit, you might also want to change these lines of the README so people learn not to inherit directly from ActiveRecord::Base anymore:

The library provides a base class that, when subclassed, sets up a mapping between the new class and an existing table in the database.

Member

claudiob commented Dec 15, 2015

As part of this commit, you might also want to change these lines of the README so people learn not to inherit directly from ActiveRecord::Base anymore:

The library provides a base class that, when subclassed, sets up a mapping between the new class and an existing table in the database.

@claudiob

This comment has been minimized.

Show comment
Hide comment
@claudiob

claudiob Dec 15, 2015

Member

I am also wondering whether we should change tests/guides/docs to refrain from referencing ActiveRecord::Base whenever possible in favor of ApplicationRecord.

Member

claudiob commented Dec 15, 2015

I am also wondering whether we should change tests/guides/docs to refrain from referencing ActiveRecord::Base whenever possible in favor of ApplicationRecord.

@gsamokovarov

This comment has been minimized.

Show comment
Hide comment
@gsamokovarov

gsamokovarov Dec 15, 2015

Contributor

Yo! I tried to touch a bit on the documentation, I'm sure I haven't replaced all the references, but at least the Active Record guides are all covered. Still, the diff is reasonable and isn't blown up.

@kaspth can you check the changelog entry again?

Contributor

gsamokovarov commented Dec 15, 2015

Yo! I tried to touch a bit on the documentation, I'm sure I haven't replaced all the references, but at least the Active Record guides are all covered. Still, the diff is reasonable and isn't blown up.

@kaspth can you check the changelog entry again?

ActiveRecord::Base.include(Yaffle::ActsAsYaffle)
# test/dummy/app/models/application_record.rb
class ApplicationRecord < ActiveRecord::Base

This comment has been minimized.

@gsamokovarov

gsamokovarov Dec 15, 2015

Contributor

I tried to sell this, so I can hint the plugin folks. Tell me if you think it's an overkill.

@gsamokovarov

gsamokovarov Dec 15, 2015

Contributor

I tried to sell this, so I can hint the plugin folks. Tell me if you think it's an overkill.

Show outdated Hide outdated activerecord/CHANGELOG.md
* Introduce ApplicationRecord, an Active Record layer supertype.
Plugin authors can now distribute Active Record extensions as
modules to be included into `ApplicationRecrod`, instead of monkey

This comment has been minimized.

@rafaelfranca

rafaelfranca Dec 15, 2015

Member

ApplicationRecord

@rafaelfranca

rafaelfranca Dec 15, 2015

Member

ApplicationRecord

Show outdated Hide outdated activerecord/CHANGELOG.md
@@ -1,3 +1,19 @@
* Introduce ApplicationRecord, an Active Record layer supertype.
Plugin authors can now distribute Active Record extensions as

This comment has been minimized.

@rafaelfranca

rafaelfranca Dec 15, 2015

Member

Other motivation, and I believe the main one, is to isolate engine models from application models. Now engine are free to use Active Record extensions, change configurations, etc without reflecting in how the application works. Can you expend this to talk about this motivation?

@rafaelfranca

rafaelfranca Dec 15, 2015

Member

Other motivation, and I believe the main one, is to isolate engine models from application models. Now engine are free to use Active Record extensions, change configurations, etc without reflecting in how the application works. Can you expend this to talk about this motivation?

Introduce ApplicationRecord, an Active Record layer supertype
It's pretty common for folks to monkey patch `ActiveRecord::Base` to
work around an issue or introduce extra functionality. Instead of
shoving even more stuff in `ActiveRecord::Base`, `ApplicationRecord` can
hold all those custom work the apps may need.

Now, we don't wanna encourage all of the application models to inherit
from `ActiveRecord::Base`, but we can encourage all the models that do,
to inherit from `ApplicationRecord`.

Newly generated applications have `app/models/application_record.rb`
present by default. The model generators are smart enough to recognize
that newly generated models have to inherit from `ApplicationRecord`,
but only if it's present.
@@ -1,3 +1,18 @@
* Introduce ApplicationRecord, an Active Record layer super type.
An `ApplicationRecord` let's engines have models, isolated from the main

This comment has been minimized.

@gsamokovarov

gsamokovarov Dec 16, 2015

Contributor

Can you folks pass over the motivation again?

@gsamokovarov

gsamokovarov Dec 16, 2015

Contributor

Can you folks pass over the motivation again?

rafaelfranca added a commit that referenced this pull request Dec 16, 2015

Merge pull request #22567 from gsamokovarov/introduce-application-record
Introduce ApplicationRecord, an Active Record layer supertype

@rafaelfranca rafaelfranca merged commit 1d7d806 into rails:master Dec 16, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Dec 16, 2015

Member

❤️ 💚 💙 💛 💜

Member

rafaelfranca commented Dec 16, 2015

❤️ 💚 💙 💛 💜

@gsamokovarov

This comment has been minimized.

Show comment
Hide comment
@gsamokovarov

gsamokovarov Dec 16, 2015

Contributor

Thanks for the help, guys! I'll try to follow up with more documentation changes.

Contributor

gsamokovarov commented Dec 16, 2015

Thanks for the help, guys! I'll try to follow up with more documentation changes.

@bschaeffer

This comment has been minimized.

Show comment
Hide comment
@bschaeffer

bschaeffer Dec 16, 2015

Shouldn't this be "inherits from ActiveRecord::Base"?

Shouldn't this be "inherits from ActiveRecord::Base"?

This comment has been minimized.

Show comment
Hide comment
@gsamokovarov

gsamokovarov Dec 17, 2015

Owner

It should. :-) Will fix it today.

Owner

gsamokovarov replied Dec 17, 2015

It should. :-) Will fix it today.

@bschaeffer

This comment has been minimized.

Show comment
Hide comment
@bschaeffer

bschaeffer Dec 16, 2015

Contributor

Shouldn't this be "inherits from ActiveRecord::Base"?

Shouldn't this be "inherits from ActiveRecord::Base"?

end
def determine_default_parent_class
if File.exist?('app/models/application_record.rb')

This comment has been minimized.

@jeremy

jeremy Dec 16, 2015

Member

Is it safe to expect that the app root is the current working directory here, or should it check for the file presence relative to application root?

@jeremy

jeremy Dec 16, 2015

Member

Is it safe to expect that the app root is the current working directory here, or should it check for the file presence relative to application root?

This comment has been minimized.

@rafaelfranca

rafaelfranca Dec 16, 2015

Member

I'll check if thor has a helper for that

@rafaelfranca

rafaelfranca Dec 16, 2015

Member

I'll check if thor has a helper for that

This comment has been minimized.

@rafaelfranca

rafaelfranca Dec 16, 2015

Member

I made the check to be relative to the application root here 342221b

@rafaelfranca

rafaelfranca Dec 16, 2015

Member

I made the check to be relative to the application root here 342221b

else
"ActiveRecord::Base"
end
end

This comment has been minimized.

@jeremy

jeremy Dec 16, 2015

Member

Wonder if we could always generate with ApplicationRecord superclass and provide a fallback (perhaps via const_missing? alias to AR::Base) and warning message when the app doesn't have an app/models/application_record.rb

@jeremy

jeremy Dec 16, 2015

Member

Wonder if we could always generate with ApplicationRecord superclass and provide a fallback (perhaps via const_missing? alias to AR::Base) and warning message when the app doesn't have an app/models/application_record.rb

This comment has been minimized.

@gsamokovarov

gsamokovarov Dec 17, 2015

Contributor

Did that mainly for apps upgrading to Rails 5. On the one hand this may push people into using ApplicationRecord which may have ignored it before, but on the other one – should we be that defensive?

@gsamokovarov

gsamokovarov Dec 17, 2015

Contributor

Did that mainly for apps upgrading to Rails 5. On the one hand this may push people into using ApplicationRecord which may have ignored it before, but on the other one – should we be that defensive?

@@ -168,11 +168,12 @@ What if you need to follow a different naming convention or need to use your
Rails application with a legacy database? No problem, you can easily override
the default conventions.
You can use the `ActiveRecord::Base.table_name=` method to specify the table
name that should be used:
`ApplicationRecord` inherits from `ActionController::Base`, which defines a

This comment has been minimized.

@santry

santry Dec 16, 2015

I believe you mean ActiveRecord::Base, not ActionController::Base.

@santry

santry Dec 16, 2015

I believe you mean ActiveRecord::Base, not ActionController::Base.

This comment has been minimized.

@gsamokovarov

gsamokovarov Dec 17, 2015

Contributor

😵

@gsamokovarov

gsamokovarov Dec 17, 2015

Contributor

😵

@@ -1,3 +1,18 @@
* Introduce ApplicationRecord, an Active Record layer super type.

This comment has been minimized.

@jeremy

jeremy Dec 16, 2015

Member

A "Active Record layer super type" = ??? to most people.

ApplicationRecord is a new superclass for all app models, analogous to app controllers subclassing ApplicationController instead of ActionController::Base. This gives apps a single spot to configure app-wide model behavior.

@jeremy

jeremy Dec 16, 2015

Member

A "Active Record layer super type" = ??? to most people.

ApplicationRecord is a new superclass for all app models, analogous to app controllers subclassing ApplicationController instead of ActionController::Base. This gives apps a single spot to configure app-wide model behavior.

This comment has been minimized.

@gsamokovarov

gsamokovarov Dec 17, 2015

Contributor

Way more concise. Will pass over the documentation nits tomorrow morning.

@gsamokovarov

gsamokovarov Dec 17, 2015

Contributor

Way more concise. Will pass over the documentation nits tomorrow morning.

application. Plugin authors can use it to distribute extensions as modules
to be included into `ApplicationRecord`, instead of monkey patches. It can
also serve as a place for applications to customize the default
`ActiveRecord::Base` model behaviour.

This comment has been minimized.

@jeremy

jeremy Dec 16, 2015

Member

This is important, but not so relevant to app developers reading the change log. Good discussion for a guide on plugin/engine development.

@jeremy

jeremy Dec 16, 2015

Member

This is important, but not so relevant to app developers reading the change log. Good discussion for a guide on plugin/engine development.

Newly generated applications have `app/models/application_record.rb`
present by default. Generators are smart enough to recognize that
newly generated models have to inherit from `ApplicationRecord` only if
it's present.

This comment has been minimized.

@jeremy

jeremy Dec 16, 2015

Member

Implementation info, not relevant to the change log.

@jeremy

jeremy Dec 16, 2015

Member

Implementation info, not relevant to the change log.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment