Add ActiveRecord::Base.suppressor #18847

Closed
dhh opened this Issue Feb 8, 2015 · 31 comments

Comments

Projects
None yet
@dhh
Member

dhh commented Feb 8, 2015

Here's a pattern of creating notifications when new comments are posted. (The notification may in turn trigger an email, a push notification, or just appear in the UI somewhere):

class Comment < ActiveRecord::Base
  belongs_to :commentable, polymorphic: true
  after_create -> { Notification.create! comment: self, recipients: commendable.recipients }
end

So that's what you want the bulk of the time. New comment creates a new Notification. But there may well be off cases, like copying a commentable and its comments, where you don't want that. So you'd have a concern something like this:

module Copyable
  def copy_to(destination)
    Notification.suppress do
      # Copy logic that creates new comments that we do not want triggering Notifications
    end
  end
end

Here's a naive implementation of ActiveRecord::Base.suppress:

module Suppressor
  extend ActiveSupport::Concern

  included do
    class_attribute :suppressed, instance_accessor: false
  end

  module ClassMethods
    # Supresses creation of events for the duration of the block.
    def suppress(&block)
      original, self.suppressed = suppressed, true
      yield
    ensure
      self.suppressed = original
    end
  end

  # Ignore saving events if we're in suppression mode.
  def save!(*args)
    self.class.suppressed ? self : super
  end
end

ActiveRecord::Base.send :include, Suppressor

May well need something more sophisticated, but that's a starting point for this feature.

(Yes, you could also accomplish this by having a separate factory for CreateCommentWithNotificationsFactory and not use that for copy. But I don't think that's at all an improvement to burden the common case with the work, rather than ask the uncommon case to make arrangements)

@dhh dhh added the activerecord label Feb 8, 2015

@dhh dhh added this to the 5.0.0 milestone Feb 8, 2015

@perceptec

This comment has been minimized.

Show comment
Hide comment
@perceptec

perceptec Feb 8, 2015

Contributor

Were you going to handle this one? (I'm willing to have a look-see if it's up for grabs, given the provided implementation.)

Contributor

perceptec commented Feb 8, 2015

Were you going to handle this one? (I'm willing to have a look-see if it's up for grabs, given the provided implementation.)

@sgrif

This comment has been minimized.

Show comment
Hide comment
@sgrif

sgrif Feb 8, 2015

Member

@perceptec Have at it.

Member

sgrif commented Feb 8, 2015

@perceptec Have at it.

@tenderlove

This comment has been minimized.

Show comment
Hide comment
@tenderlove

tenderlove Feb 8, 2015

Member

Should we have an unsuppress too?

Member

tenderlove commented Feb 8, 2015

Should we have an unsuppress too?

@dhh

This comment has been minimized.

Show comment
Hide comment
@dhh

dhh Feb 8, 2015

Member

How would that one be used, Aaron?

On Feb 8, 2015, at 1:02 PM, Aaron Patterson notifications@github.com wrote:

Should we have an unsuppress too?


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

Member

dhh commented Feb 8, 2015

How would that one be used, Aaron?

On Feb 8, 2015, at 1:02 PM, Aaron Patterson notifications@github.com wrote:

Should we have an unsuppress too?


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

@tenderlove

This comment has been minimized.

Show comment
Hide comment
@tenderlove

tenderlove Feb 8, 2015

Member

In case something inside your copy_to needs to send notifications (e.g. notify admins that it got copied). Like:

module Copyable
  def copy_to(destination)
    Notification.suppress do
      destination.comments << Comment.create
    end
  end
end

Where updating the comments on the destination needs to send a notification. I suppose you could just do this:

module Copyable
  def copy_to(destination)
    comment = Notification.suppress { Comment.create }
    destination.comments << comment
    end
  end
end

But now this Concern has to know that Comment will fire a notification in the callbacks, and that updating the destination may do that as well.... hrm.

Member

tenderlove commented Feb 8, 2015

In case something inside your copy_to needs to send notifications (e.g. notify admins that it got copied). Like:

module Copyable
  def copy_to(destination)
    Notification.suppress do
      destination.comments << Comment.create
    end
  end
end

Where updating the comments on the destination needs to send a notification. I suppose you could just do this:

module Copyable
  def copy_to(destination)
    comment = Notification.suppress { Comment.create }
    destination.comments << comment
    end
  end
end

But now this Concern has to know that Comment will fire a notification in the callbacks, and that updating the destination may do that as well.... hrm.

@dhh

This comment has been minimized.

Show comment
Hide comment
@dhh

dhh Feb 8, 2015

Member

For that case, IMO, you'd do something like:

module Copyable
  def copy_to(destination)
    Notification.suppress do
      destination.comments << Comment.create
    end

    Notification.create # tell the admin
  end
end

Seems like it'd be a bit of a battle between the suppress/unsuppress otherwise. But maybe we just need to find a more compelling use case.

Member

dhh commented Feb 8, 2015

For that case, IMO, you'd do something like:

module Copyable
  def copy_to(destination)
    Notification.suppress do
      destination.comments << Comment.create
    end

    Notification.create # tell the admin
  end
end

Seems like it'd be a bit of a battle between the suppress/unsuppress otherwise. But maybe we just need to find a more compelling use case.

@pixeltrix

This comment has been minimized.

Show comment
Hide comment
@pixeltrix

pixeltrix Feb 8, 2015

Member

Just a word of caution here - let's not replay the issues we've had over the years with scopes and modifying state on the model class. The implementation needs to be threadsafe and not subject to strange side effects of lazy evaluation falling outside of the suppressed block.

Member

pixeltrix commented Feb 8, 2015

Just a word of caution here - let's not replay the issues we've had over the years with scopes and modifying state on the model class. The implementation needs to be threadsafe and not subject to strange side effects of lazy evaluation falling outside of the suppressed block.

@dhh

This comment has been minimized.

Show comment
Hide comment
@dhh

dhh Feb 8, 2015

Member

👍 to that AW. Use Thread.current in the real implementation.

On Feb 8, 2015, at 2:09 PM, Andrew White notifications@github.com wrote:

Just a word of caution here - let's not replay the issues we've had over the years with scopes and modifying state on the model class. The implementation needs to be threadsafe and not subject to strange side effects of lazy evaluation falling outside of the suppressed block.


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

Member

dhh commented Feb 8, 2015

👍 to that AW. Use Thread.current in the real implementation.

On Feb 8, 2015, at 2:09 PM, Andrew White notifications@github.com wrote:

Just a word of caution here - let's not replay the issues we've had over the years with scopes and modifying state on the model class. The implementation needs to be threadsafe and not subject to strange side effects of lazy evaluation falling outside of the suppressed block.


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

@cristianbica

This comment has been minimized.

Show comment
Hide comment
@cristianbica

cristianbica Feb 8, 2015

Member

@dhh in your example it seems to me more appropriate to disable the after_create callbacks than to completely disable save.
Anyway if we will go on with this Notification.supress isn't self-explanatory to me. Maybe we can use a more verbose syntax Notification.supress(:create) or Notification.supress(:save) or to combine with the above Notification.supress(:callbacks).

Member

cristianbica commented Feb 8, 2015

@dhh in your example it seems to me more appropriate to disable the after_create callbacks than to completely disable save.
Anyway if we will go on with this Notification.supress isn't self-explanatory to me. Maybe we can use a more verbose syntax Notification.supress(:create) or Notification.supress(:save) or to combine with the above Notification.supress(:callbacks).

@dhh

This comment has been minimized.

Show comment
Hide comment
@dhh

dhh Feb 8, 2015

Member

@cristianbica I'm not sure I follow. When I do a copy, I don't want Notifications to be created. These notifications are created by the Comment in an after_save, yes, but it may be doing lots of other stuff in callbacks that does need to happen. So you wouldn't want to do something like Comment.suppress(:callbacks) in this example.

What's the use case you're imagining for having a difference between create and save?

Suppress is the same name used elsewhere, like logging, to prevent something from being saved. So seems to fit from that angle.

Member

dhh commented Feb 8, 2015

@cristianbica I'm not sure I follow. When I do a copy, I don't want Notifications to be created. These notifications are created by the Comment in an after_save, yes, but it may be doing lots of other stuff in callbacks that does need to happen. So you wouldn't want to do something like Comment.suppress(:callbacks) in this example.

What's the use case you're imagining for having a difference between create and save?

Suppress is the same name used elsewhere, like logging, to prevent something from being saved. So seems to fit from that angle.

@cristianbica

This comment has been minimized.

Show comment
Hide comment
@cristianbica

cristianbica Feb 8, 2015

Member

@dhh
I don't see a use case for separate supress for :create or :save but using just supress it doesn't tell me it won't save records (maybe because I'm not an english native). For the Notification model it's a bit more clear because you're thinking "supress notifications" but for other model names it might not be so clear. For logger the method id named .silence which is explanatory.

Member

cristianbica commented Feb 8, 2015

@dhh
I don't see a use case for separate supress for :create or :save but using just supress it doesn't tell me it won't save records (maybe because I'm not an english native). For the Notification model it's a bit more clear because you're thinking "supress notifications" but for other model names it might not be so clear. For logger the method id named .silence which is explanatory.

@dhh

This comment has been minimized.

Show comment
Hide comment
@dhh

dhh Feb 8, 2015

Member

The dictionary definition seems to fit what we're doing to a tee: http://dictionary.reference.com/browse/suppress. I think it works with all sorts of classes. In my code base, I'm currently using three different versions of this is different places: Event.suppress, Mention.suppress, Notification.suppress. All those work for me.

Member

dhh commented Feb 8, 2015

The dictionary definition seems to fit what we're doing to a tee: http://dictionary.reference.com/browse/suppress. I think it works with all sorts of classes. In my code base, I'm currently using three different versions of this is different places: Event.suppress, Mention.suppress, Notification.suppress. All those work for me.

@eileencodes eileencodes self-assigned this Feb 11, 2015

@perceptec

This comment has been minimized.

Show comment
Hide comment
@perceptec

perceptec Feb 12, 2015

Contributor

I'd appreciate some grown-up eyes on my last commit--I've tried to account for the threading discussion above (and wonder if it couldn't be more idiomatic in its current form, though that's a lesser consideration).

Contributor

perceptec commented Feb 12, 2015

I'd appreciate some grown-up eyes on my last commit--I've tried to account for the threading discussion above (and wonder if it couldn't be more idiomatic in its current form, though that's a lesser consideration).

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Feb 12, 2015

Member

What if, we use contexts to implement this feature instead of adding a new method and a new concept?

class Comment < ActiveRecord::Base
  belongs_to :commentable, polymorphic: true
  after_create -> { Notification.create! comment: self, recipients: commendable.recipients },
   except_on: :copying
end
module Copyable
  def copy_to(destination)
    comment = Comment.new
    comment.save(context: :copying)
  end
end
Member

rafaelfranca commented Feb 12, 2015

What if, we use contexts to implement this feature instead of adding a new method and a new concept?

class Comment < ActiveRecord::Base
  belongs_to :commentable, polymorphic: true
  after_create -> { Notification.create! comment: self, recipients: commendable.recipients },
   except_on: :copying
end
module Copyable
  def copy_to(destination)
    comment = Comment.new
    comment.save(context: :copying)
  end
end
@dhh

This comment has been minimized.

Show comment
Hide comment
@dhh

dhh Feb 12, 2015

Member

Then the comment has to know about all the concerns that might do stuff to it. That seems highly coupled.

On Feb 12, 2015, at 08:56, Rafael Mendonça França notifications@github.com wrote:

What if, we use contexts to implement this feature instead of adding a new method and a new concept?

class Comment < ActiveRecord::Base

belongs_to
:commentable, polymorphic: true

after_create
-> { Notification.create! comment: self, recipients: commendable.recipients }, except_on: :copying
end
module Copyable

def copy_to(destination
)
comment
= Comment.new

comment.save(

context: :copying
)

end
end

Reply to this email directly or view it on GitHub.

Member

dhh commented Feb 12, 2015

Then the comment has to know about all the concerns that might do stuff to it. That seems highly coupled.

On Feb 12, 2015, at 08:56, Rafael Mendonça França notifications@github.com wrote:

What if, we use contexts to implement this feature instead of adding a new method and a new concept?

class Comment < ActiveRecord::Base

belongs_to
:commentable, polymorphic: true

after_create
-> { Notification.create! comment: self, recipients: commendable.recipients }, except_on: :copying
end
module Copyable

def copy_to(destination
)
comment
= Comment.new

comment.save(

context: :copying
)

end
end

Reply to this email directly or view it on GitHub.

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Feb 12, 2015

Member

Right, make sense.

I have a bad feeling about this feature and I believe it will lead to a lot of bad usages but it is just a feeling at this point.

I don't have any more arguments besides the one already presented by you in the issue description about the factories (or whatever people want to call that pattern).

I already tried both approaches (the factory and the suppressing using contexts) in my applications and both went fine. I don't have a strong argument against suppressing so I'll stand back with my feelings and try this implementation.

👍

Member

rafaelfranca commented Feb 12, 2015

Right, make sense.

I have a bad feeling about this feature and I believe it will lead to a lot of bad usages but it is just a feeling at this point.

I don't have any more arguments besides the one already presented by you in the issue description about the factories (or whatever people want to call that pattern).

I already tried both approaches (the factory and the suppressing using contexts) in my applications and both went fine. I don't have a strong argument against suppressing so I'll stand back with my feelings and try this implementation.

👍

@perceptec

This comment has been minimized.

Show comment
Hide comment
@perceptec

perceptec Feb 12, 2015

Contributor

✈️ Holding at this flight level . . .

Contributor

perceptec commented Feb 12, 2015

✈️ Holding at this flight level . . .

@dhh

This comment has been minimized.

Show comment
Hide comment
@dhh

dhh Feb 12, 2015

Member

I’m using this pattern at the moment in Basecamp and like it. As a general rule, I don’t find “people might cut themselves with a sharp knife if they wield it wrong” as a good direction for design. Rails should be full of both spoons, forks, and also some sharp knifes.

On Feb 12, 2015, at 09:08, Rafael Mendonça França notifications@github.com wrote:

Right, make sense.

I have a bad feeling about this feature and I believe it will lead to a lot of bad usages but it is just a feeling at this point.

I don't have any more arguments besides the one already presented by you in the issue description about the factories (or whatever people want to call that pattern).

I already tried both approaches (the factory and the suppressing using contexts) in my applications and both went fine. I don't have a strong argument against suppressing so I'll stand back with my feelings and try this implementation.


Reply to this email directly or view it on GitHub.

Member

dhh commented Feb 12, 2015

I’m using this pattern at the moment in Basecamp and like it. As a general rule, I don’t find “people might cut themselves with a sharp knife if they wield it wrong” as a good direction for design. Rails should be full of both spoons, forks, and also some sharp knifes.

On Feb 12, 2015, at 09:08, Rafael Mendonça França notifications@github.com wrote:

Right, make sense.

I have a bad feeling about this feature and I believe it will lead to a lot of bad usages but it is just a feeling at this point.

I don't have any more arguments besides the one already presented by you in the issue description about the factories (or whatever people want to call that pattern).

I already tried both approaches (the factory and the suppressing using contexts) in my applications and both went fine. I don't have a strong argument against suppressing so I'll stand back with my feelings and try this implementation.


Reply to this email directly or view it on GitHub.

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Feb 12, 2015

Member

As a general rule, I don’t find “people might cut themselves with a sharp knife if they wield it wrong” as a good direction for design. Rails should be full of both spoons, forks, and also some sharp knifes.

👍 I was just trying to give a better shape to our Ginsu knife and not hurt ourselves leaving it without a handhold.

Member

rafaelfranca commented Feb 12, 2015

As a general rule, I don’t find “people might cut themselves with a sharp knife if they wield it wrong” as a good direction for design. Rails should be full of both spoons, forks, and also some sharp knifes.

👍 I was just trying to give a better shape to our Ginsu knife and not hurt ourselves leaving it without a handhold.

@dhh

This comment has been minimized.

Show comment
Hide comment
@dhh

dhh Feb 12, 2015

Member

Haha, it sounded like "they can't handle the Ginsu, give them a fisher price" :).

If you don't mind tying your Comment implementation to a concern like Copyable, then context is a fine approach imo. I don't think comment should know about Copyable, unless it's Copyable itself (which in my case it is not).

Member

dhh commented Feb 12, 2015

Haha, it sounded like "they can't handle the Ginsu, give them a fisher price" :).

If you don't mind tying your Comment implementation to a concern like Copyable, then context is a fine approach imo. I don't think comment should know about Copyable, unless it's Copyable itself (which in my case it is not).

@sharvy

This comment has been minimized.

Show comment
Hide comment
@sharvy

sharvy Feb 12, 2015

@dhh, if I want to suppress more than one model? And I think if we could do something like this, wouldn't it be simpler?

comment.save!({suppress: [:notification, :unreads]})

sharvy commented Feb 12, 2015

@dhh, if I want to suppress more than one model? And I think if we could do something like this, wouldn't it be simpler?

comment.save!({suppress: [:notification, :unreads]})

@dhh

This comment has been minimized.

Show comment
Hide comment
@dhh

dhh Feb 12, 2015

Member

Sharvy, I prefer the suppression to be explcitly from the perspective of the model being suppressed. So:

Notification.suppressed do
Unreads.suppressed do
  copy
end
end
Member

dhh commented Feb 12, 2015

Sharvy, I prefer the suppression to be explcitly from the perspective of the model being suppressed. So:

Notification.suppressed do
Unreads.suppressed do
  copy
end
end
@sharvy

This comment has been minimized.

Show comment
Hide comment
@sharvy

sharvy Feb 12, 2015

@dhh got it. Thanks a lot. And yes your implementation makes sense, Comment has nothing to do with Copyable, therefore the suppression has to be explicitly done.

sharvy commented Feb 12, 2015

@dhh got it. Thanks a lot. And yes your implementation makes sense, Comment has nothing to do with Copyable, therefore the suppression has to be explicitly done.

@kaspth kaspth removed the openacademy label Feb 20, 2015

@pacoguzman

This comment has been minimized.

Show comment
Hide comment
@pacoguzman

pacoguzman Feb 21, 2015

Contributor

@rafaelfranca I was reviewing this issue and I would like to know if why you suggest of using context, is it already possible or should be implemented inside active_record/active_model?

Contributor

pacoguzman commented Feb 21, 2015

@rafaelfranca I was reviewing this issue and I would like to know if why you suggest of using context, is it already possible or should be implemented inside active_record/active_model?

@henrik

This comment has been minimized.

Show comment
Hide comment
@henrik

henrik Feb 21, 2015

Contributor

(Yes, you could also accomplish this by having a separate factory for CreateCommentWithNotificationsFactory and not use that for copy. But I don't think that's at all an improvement to burden the common case with the work, rather than ask the uncommon case to make arrangements)

Common case vs. uncommon case is one factor. Another is the consequence of triggering the side effect: if you write a migration and accidentally notify all your users, is that something you can shrug off as inconsequential, or an embarrassing mistake that you could and should have avoided with different habits?

To rephrase, you should weigh the inconvenience of having to write something like CreateComment.call(params[:comment]) (or, if you're a strawman, CreateCommentWithNotificationsFactory.harhar(params[:comment])) instead of Comment.create(params[:comment]) against the fact that you're now burdening your fragile human faculties with a system that is dangerous-by-default (whatever level of "danger" the side effect amounts to).

If the cost is relatively low as compared to what dangers you avoid, I think being safer by default is the sensible thing.

Contributor

henrik commented Feb 21, 2015

(Yes, you could also accomplish this by having a separate factory for CreateCommentWithNotificationsFactory and not use that for copy. But I don't think that's at all an improvement to burden the common case with the work, rather than ask the uncommon case to make arrangements)

Common case vs. uncommon case is one factor. Another is the consequence of triggering the side effect: if you write a migration and accidentally notify all your users, is that something you can shrug off as inconsequential, or an embarrassing mistake that you could and should have avoided with different habits?

To rephrase, you should weigh the inconvenience of having to write something like CreateComment.call(params[:comment]) (or, if you're a strawman, CreateCommentWithNotificationsFactory.harhar(params[:comment])) instead of Comment.create(params[:comment]) against the fact that you're now burdening your fragile human faculties with a system that is dangerous-by-default (whatever level of "danger" the side effect amounts to).

If the cost is relatively low as compared to what dangers you avoid, I think being safer by default is the sensible thing.

@kaspth

This comment has been minimized.

Show comment
Hide comment
@kaspth

kaspth Feb 21, 2015

Member

@henrik I think the underlying thing is that you have a different view on people than Rails does. We trust them to handle that possibility of danger.

But you want a fisher price knife, so to speak, and that's perfectly fine. That's just not what Rails wants.

Thanks for writing. Your point has been heard, but we're going in a different direction ❤️

Member

kaspth commented Feb 21, 2015

@henrik I think the underlying thing is that you have a different view on people than Rails does. We trust them to handle that possibility of danger.

But you want a fisher price knife, so to speak, and that's perfectly fine. That's just not what Rails wants.

Thanks for writing. Your point has been heard, but we're going in a different direction ❤️

@henrik

This comment has been minimized.

Show comment
Hide comment
@henrik

henrik Feb 21, 2015

Contributor

@kaspth I can definitely accept the general idea - this is of course just my opinion and not the only valid one.

I don't share the view that Rails has a preference for trusting people. We're now HTML safe by default instead of trusting the person to always get it right. IIRC sanitize was changed to whitelist, not blacklist.

I see this as related to that line of thinking.

Rails DOES have a preference for callbacks in some cases where I do not – this change is certainly in line with that.

Thanks for the thoughtful reply! ❤️

Contributor

henrik commented Feb 21, 2015

@kaspth I can definitely accept the general idea - this is of course just my opinion and not the only valid one.

I don't share the view that Rails has a preference for trusting people. We're now HTML safe by default instead of trusting the person to always get it right. IIRC sanitize was changed to whitelist, not blacklist.

I see this as related to that line of thinking.

Rails DOES have a preference for callbacks in some cases where I do not – this change is certainly in line with that.

Thanks for the thoughtful reply! ❤️

@radar

This comment has been minimized.

Show comment
Hide comment
@radar

radar May 23, 2016

Contributor

I agree with @rafaelfranca.

This feature will be mis-used in quite a lot of Rails applications. Suppressing callbacks in this way should be something that the framework actively discourages, rather than encourages. I have worked with enough live Rails applications (that aren't just simple CRUD apps) to know that this feature is going to cause issues in the long run.

From the examples given in the PR, this is what I understand it to be doing:

  1. There is an after_create callback on the Comment model which creates new Notification objects every time a comment is created. This is a pretty "traditional" callback in Rails-land.
  2. To stop notifications being created you wrap it in a suppress block:
Notification.suppress do
  # code that calls something like Comment.create
end

This problem could just as easily be solved by not having the after_create callback at all. As other people have suggested, there's two better solutions:

  1. Move the logic of creating the comment + sending a notification out to a "service class". This is the direction the Rails community has been headed recently, and the choice that I personally would pick, regardless of any BDFL's quips.
  2. In the one place where I care about both creating a comment and sending a notification (probably CommentsController), have this code:
def create
  comment = Comment.new(comment_params)
  if comment.save
    Notification.create(thing: comment)
    # etc.
  end
end

Both of these solutions are more preferable (to me, at least) than adding more code to the Rails framework which encourages "magical" behaviour. Both of the solutions I've provided make it explicit that when the comment is created, the notification is created. If Notification.create isn't called after Comment.create, then there will be no notifications. This is particularly handy in tests, when we don't necessarily want to create a Comment and a Notification together because of tight coupling.

Consider reverting this feature. It is going to cause headaches because it will be misused in Rails applications.

Contributor

radar commented May 23, 2016

I agree with @rafaelfranca.

This feature will be mis-used in quite a lot of Rails applications. Suppressing callbacks in this way should be something that the framework actively discourages, rather than encourages. I have worked with enough live Rails applications (that aren't just simple CRUD apps) to know that this feature is going to cause issues in the long run.

From the examples given in the PR, this is what I understand it to be doing:

  1. There is an after_create callback on the Comment model which creates new Notification objects every time a comment is created. This is a pretty "traditional" callback in Rails-land.
  2. To stop notifications being created you wrap it in a suppress block:
Notification.suppress do
  # code that calls something like Comment.create
end

This problem could just as easily be solved by not having the after_create callback at all. As other people have suggested, there's two better solutions:

  1. Move the logic of creating the comment + sending a notification out to a "service class". This is the direction the Rails community has been headed recently, and the choice that I personally would pick, regardless of any BDFL's quips.
  2. In the one place where I care about both creating a comment and sending a notification (probably CommentsController), have this code:
def create
  comment = Comment.new(comment_params)
  if comment.save
    Notification.create(thing: comment)
    # etc.
  end
end

Both of these solutions are more preferable (to me, at least) than adding more code to the Rails framework which encourages "magical" behaviour. Both of the solutions I've provided make it explicit that when the comment is created, the notification is created. If Notification.create isn't called after Comment.create, then there will be no notifications. This is particularly handy in tests, when we don't necessarily want to create a Comment and a Notification together because of tight coupling.

Consider reverting this feature. It is going to cause headaches because it will be misused in Rails applications.

@dhh

This comment has been minimized.

Show comment
Hide comment
@dhh

dhh May 23, 2016

Member

You're a year late to the discussion and you didn't bring any new arguments to the party. Feel free to write your own code without callbacks, but this chef's knife is staying in the drawer.

Member

dhh commented May 23, 2016

You're a year late to the discussion and you didn't bring any new arguments to the party. Feel free to write your own code without callbacks, but this chef's knife is staying in the drawer.

@mollrow

This comment has been minimized.

Show comment
Hide comment
@mollrow

mollrow May 23, 2016

Honestly @radar has a fantastic point, and one I didn't see anywhere else in this thread. I would also strongly disagree with there being a time limit on discussing the merits of a public API feature.

mollrow commented May 23, 2016

Honestly @radar has a fantastic point, and one I didn't see anywhere else in this thread. I would also strongly disagree with there being a time limit on discussing the merits of a public API feature.

@rails rails locked and limited conversation to collaborators May 23, 2016

@dhh

This comment has been minimized.

Show comment
Hide comment
@dhh

dhh May 23, 2016

Member

Locking this thread along with the other outlets. If you want to start a discussion on the mailing list to take this further, be welcome. All the same considerations apply. Arguments already considered aren't going sway things by being restated. But you're welcome to provide novel arguments.

"This might get misused" was addressed today in the ninth pillar added to the doctrine: http://rubyonrails.org/doctrine/#provide-sharp-knives. So consider that line of argument addressed there.

Member

dhh commented May 23, 2016

Locking this thread along with the other outlets. If you want to start a discussion on the mailing list to take this further, be welcome. All the same considerations apply. Arguments already considered aren't going sway things by being restated. But you're welcome to provide novel arguments.

"This might get misused" was addressed today in the ninth pillar added to the doctrine: http://rubyonrails.org/doctrine/#provide-sharp-knives. So consider that line of argument addressed there.

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