Remove always use prepend instead of alias_method for patches

MiniProfiler / rack-mini-profiler

Profiler for your development and production Ruby rack apps.

MIT License

3.71k stars 404 forks source link

Remove always use prepend instead of alias_method for patches #538

Closed johnnyshields closed 1 year ago

johnnyshields commented 2 years ago

There are two patches which have options to use prepend instead of alias_method (alias_method is currently used by default):

prepend_net_http_patch
prepend_mysql2_patch

It would be much better if Rack Mini Profiler always uses prepend for these, as there is no disadvantage to doing so. I spent 3 hours putzing around trying to figure out why Rack Mini Profiler wasn't working, and I discovered I had to include the Net::HTTP prepend patch (which Rack Mini Profiler could have just done by default.)

OsamaSayegh commented 2 years ago

@johnnyshields I wish we could remove the options and just use prepend to apply our patches, but unfortunately I don't think we can because if another gem is installed alongside MP and happens to also patch a method that MP patches using alias_method, then that method will error when it's called. See this issue on ruby's bug tracker.

johnnyshields commented 2 years ago

@OsamaSayegh maybe prepend should be the default then, and we have a patch to enable alias_method instead?

prepend is Ruby's recommended way of doing this form of monkey patching.
Rails removed usage of alias_method_chain in favor of prepend.
The OpenTelemetry gem (which was the conflict I experienced) does it to Net::HTTP here.
Most other gems should use prepend if not doing so already.

nateberkopec commented 1 year ago

I've dealt with this one personally and I think it's not worth the effort to change the default here. Maybe someday in an alias_method-less world 5 years from now (someone bookmark this in 5 years and ping me, you legend).