Skip to content
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

Creating ApplicationModel #12162

Closed

Conversation

wangjohn
Copy link
Contributor

@wangjohn wangjohn commented Sep 8, 2013

This PR creates the ApplicationModel class. This class will serve as an intermediate between a model and ActiveRecord::Base. Currently, to create a model, one follows the following procedure:

class MyModelName < ActiveRecord::Base
end

This inherits directly from ActiveRecord::Base and means that all configurations on ActiveRecord::Base are global to all of its subclasses (unless the subclass redefines the configuration on its own).

I'm introducing ApplicationModel so that models have an intermediary layer for inheritance like so:

class MyModelName < ApplicationModel
end

What the ApplicationModel class does is allow different sets of configurations to be defined on multiple applications. Each Rails application will have its own ApplicationModel. The ApplicationModel figures out which application it belongs when it is pulled in through the railtie and configs_from is specified.

\cc @spastorino, @josevalim Can you guys take a look? I'm sure there's something I forgot so please let me know.

The application model will keep all of the Active Record configurations,
instead of sending the configurations directly to ActiveRecord::Base.
This means it will be possible to configure multiple applications if
each application has its own ApplicationModel.

Also, changing some tests to correspond to these changes.
The Active Record tests should now include inside of it an
ApplicationModel.
This replaces the current use of ActiveRecord::Base in most tests.
These methods will be used by the ApplicationModel to send the correct
configuration.
@robin850
Copy link
Member

robin850 commented Sep 8, 2013

Awesome pull request, thank you! I've noticed several things:

  • I think that the main README file should be updated since it states that models derived from ActiveRecord::Base, this is still true but not completely. The RDOC_MAIN.rdoc files should be updated as well in each project.
  • All the guides should be updated (mainly all the ActiveRecord ones and "Getting started with Rails"). Also, I think that we should mention the motivation behind the existence of this new class in the "Active Record Basics" guide.

Don't hesitate to ping me if you want some help. 😃 Thank you!

@egilburg
Copy link
Contributor

egilburg commented Sep 8, 2013

Thanks! Given the very large scope of this PR could this be a right time to discuss extension vs composition?

I've seen some feedback of people discussing that directly subclassing from ActiveRecord::Base goes counter to Domain-Driven Design, under which class hierarchy should be driven by the problem domain rather than technical implementation. Under this school of thought, the following would be preferable:

class Person
  # Person is the root object under the problem domain.
  # The fact it uses AR::Base as a persistence implementation, shouldn't drive the domain class hierarchy.
  include ActiveRecord::Base # Or ApplicationModel
end

class Client < Person
  # Client is a domain-driven subclass of Person, so it makes sense for it to be an actual subclass
end

The above would also make it more flexible to use include statements either before or after including ActiveRecord::Base (or ApplicationModel), for more granular control over inclusion order and module hierarchy.

This change going to be a disruptive change to matter what, so it's a good time as any to discuss what's the best approach. Thanks again!

@@ -92,7 +92,7 @@ def test_module_file_is_not_created

def test_adds_namespace_to_model
run_generator
assert_file "app/models/test_app/account.rb", /module TestApp/, / class Account < ActiveRecord::Base/
assert_file "app/models/test_app/account.rb", /module TestApp/, / class Account < ApplicationModel/
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should base model classes be generated per namespace? E.g.

module TestApp
  class AppicationModel < ::ApplicationModel
  end
end

module TestApp
  class Account < ApplicationModel
  end
end

Anyone remembers how controller generators work for namespaced controllers?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Good point @egilburg!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, you're right about this. I'll make the change.

@rafaelfranca
Copy link
Member

We already had this discussion in rails 4 and the conclusion was: there are
no real gain other than theoretical thought.

Also include also means inheritance in Ruby, so this change is not worth of
all the the complexity of maintaining the code for support both way
On Sep 8, 2013 3:42 PM, "Eugene Gilburg" notifications@github.com wrote:

Thanks! Given the very large scope of this PR, could this be a right time
to discuss extension vs composition?

I've seen some feedback of people discussing that directly subclassing
from AR::Base goes counter to domain-driven design, under which class
hierarchy should be driven by the problem domain rather than technical
implementation. Under this school of thought, the following would be
preferable:

class Person

Person is the root object under the problem domain.

The fact it uses AR::Base as a persistence implementation, shouldn't drive the domain class hierarchy.

include ActiveRecord::Base # Or ApplicationModelend
class Client < Person

Client is a domain-driven subclass of Person, so it makes sense for it to be an actual subclassend

The above would also make it more flexible to use include statements
either before or after including ActiveRecord::Base (or ApplicationModel),
for more granular control over inclusion order and module hierarchy.


Reply to this email directly or view it on GitHubhttps://github.com//pull/12162#issuecomment-24026590
.

@egilburg
Copy link
Contributor

egilburg commented Sep 8, 2013

What impact will there be on Rails Engines? Will there be a separate ApplicationModel per engine?

@fxn
Copy link
Member

fxn commented Sep 8, 2013

I am 👎 on this one, because ApplicationModel would take over a name whose scope is broader than what it means, because not every model is persisted.

@guilleiguaran
Copy link
Member

@fxn maybe this should be named ApplicationRecord ?

@fxn
Copy link
Member

fxn commented Sep 8, 2013

@guilleiguaran yeah, I believe ApplicationRecord is a better name.

@yfeldblum
Copy link

+1 on ApplicationRecord rather than ApplicationModel.

@thegrubbsian
Copy link

+1 on ApplicationRecord, but I'm also +1 on doing this as an included module.

@guilleiguaran
Copy link
Member

-1 about included module. See 9e4c41c for reason

@wangjohn
Copy link
Contributor Author

wangjohn commented Sep 8, 2013

So it seems that most people think it should be called ApplicationRecord, I'll go ahead and make that change. I'll also be starting to slowly change the rdocs and the guides. Thanks everyone for all of the comments/suggestions!

This method first checks to see if an application configuration is
defined, then gets the correct ApplicationModel. Reverts to
ActiveRecord::Base if it can't find anything.
The comments have become outdated since the creation of the
ApplicationModel. Now, the model classes should inherit from
ApplicationModel instead of ActiveRecord::Base.
This new name is less general and more appropriate since these changes
are limited to Active Record.
@spastorino
Copy link
Contributor

we agreed with @wangjohn to move his GSoC PRs to this branch https://github.com/rails/rails/tree/rework_initialization

@wangjohn wangjohn closed this Sep 9, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants