solidus icon indicating copy to clipboard operation
solidus copied to clipboard

Make all model classes replaceable

Open aldesantis opened this issue 4 years ago • 3 comments
trafficstars

The problem

Most of the service objects used in Solidus are configurable, so that users can easily swap them with their own implementation which either inherits from or completely replaces the original. This allows users to customize the business logic in a very clean and easily testable way.

When it comes to models, things are a bit more complicated. Our official recommendation is to customize them through monkey patches, e.g.

# app/decorators/user_decorator.rb
module UserDecorator
  # Your overrides here...

  Spree::User.prepend(self)
end

This works, but it has a few drawbacks:

  • It pollutes the Solidus domain with logic that belongs to the host app. In large codebases, this makes it hard to distinguish what belongs to Solidus and what belongs to the business domain, and might even create confusion at the ecosystem level (e.g. if a developer moves between Solidus projects).
  • It doesn't play well with static typing (e.g. Sorbet doesn't support prepend).
  • It messes with the ancestors chain in a non-obvious way (especially for developers who come from other languages), making it harder to debug issues.

The solution

I would like to suggest a different solution: right now, it's already possible to override the user model class. So instead of the above decorator, I could do the following:

# app/models/user.rb
class User < Spree::User
  # Your overrides here...
end

# config/initializers/solidus.rb
Spree.user_class = 'User'

With this configuration, Solidus will use my custom User model instead of the default Spree::User. Because my model inherits from the built-in model, I still get all the out-of-the-box functionality, but I can add new features or override existing ones however I want.

This creates a clearer separation between the Solidus domain and the host app domain, it plays well with static typing and it uses class inheritance, which is a well understood pattern in all OOP languages.

Nothing prevents us from simply extending this pattern to all models. See the following example:

# app/models/order.rb
class Order < Spree::Order
  # Your overrides here...
end

# config/initializers/solidus.rb
Spree.config do |config|
  config.models.order_class = 'Order'
end

This would be a very straightforward and convenient way of replacing/extending any models with your own. It has all of the advantages of prepend with none of the drawbacks.

I'd like to gather feedback from the group and see how everyone feels about this, and whether it's worth a spike.

aldesantis avatar Jul 31 '21 14:07 aldesantis

I think this would be very interesting and not break existing extensions or implementation that work with eval or prepend. I am wondering how you envision this for (new) extensions? They will still need to rely on monkey patching.

victorth avatar Jul 31 '21 19:07 victorth

That's a great idea! I just wrote something related but for the service objects: https://github.com/solidusio/solidus/pull/4138#issuecomment-890825954

We could do the same for models. We can auto-register all of them and allow users to inject the ones they need.

waiting-for-dev avatar Aug 02 '21 08:08 waiting-for-dev

I like the idea but I have the same concerns that @victorth raised:

I am wondering how you envision this for (new) extensions? They will still need to rely on monkey patching.

  • I think it won't work with existing extensions either because when a store changes the model class used for a specific resource, it will lose all the extension's money patches breaking the extension behavior (unless we change the extensions code to prepend into Spree::Config.models.resource_class, but at that point, we are reintroducing some of the drawbacks outlined here).

kennyadsl avatar Aug 03 '21 07:08 kennyadsl