Conversation
Fixes roidrage#385 Assumes that a built-in subscriber is nested under the component namespace, so under ActionView or ActionController.
| end.to change { | ||
| Lograge.notification_listeners_for('render_template.action_view') | ||
| } | ||
| }.to([]) |
There was a problem hiding this comment.
This spec failed after this change on Rails 7.1
| it "does not remove subscribers that aren't from Rails" do | ||
| proc_subscriber = ActiveSupport::Notifications.subscribe(event_name, proc {}) | ||
| custom_subscriber = | ||
| ActiveSupport::Notifications.subscribe(event_name, CustomListener.new) |
There was a problem hiding this comment.
Also check a custom class, in case there was some special handling of Proc instances
| delegate = listener.instance_variable_get('@delegate') | ||
|
|
||
| # Assume that anything nested under the component namespace is built in to Rails | ||
| delegate.class.name.start_with?("#{component.to_s.classify}::") |
There was a problem hiding this comment.
I don't think this is great, but I can't see a better way to check if the delegate is subscriber or (for Rails 7.1+) an instance of ActionView::LogSubscriber::Start for ActionView or subscriber for ActionController
owst
changed the title
|
Hey any progress on this? I'm using the branch in production to help our logs. Would love to see this merged. Anything I can help with? |
|
@trobrock I've recently tapped in to help with this project. thanks for the PR, i will be taking a look shortly. i know its been kicking around for quite some time so let me know if anything about the situation has changed. |
|
I think no change, this is still required 👍 |
Replace the namespace-prefix check with an explicit match on the subscriber instance or ActionView::LogSubscriber::Start (Rails 7.1+). The prefix check raised NoMethodError for listeners whose class is anonymous (Class#name is nil), and would also have removed any non-log-subscriber listener nested under ActionView:: or ActionController::. Also add a spec covering anonymous-class listeners and the missing frozen string literal comment in CustomListener. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Thanks for this @owst, and sorry it sat for so long. I've pushed a couple of commits on top so we can get it merged:
Tested locally on Rails 7.0, 7.1 and 8.1 (all green), and RuboCop is clean. Your original commit is preserved as-is. |
|
these changes landed in the |
4 participants
Fixes #385
Assumes that a built-in subscriber is nested under the component namespace, so under ActionView or ActionController.