Feature #2542

Plugin hooks should have access to the "request" variable

Added by Douglas Manley almost 9 years ago. Updated over 8 years ago.

Status:ClosedStart date:2009-01-19
Priority:NormalDue date:
Assignee:Eric Davis% Done:

100%

Category:Plugin API
Target version:0.8.2
Resolution:Fixed

Description

Plugin Hooks do not have access to the "request" variable; this makes linking from them difficult.

In my environment, we have to contend with the different ways that people access the site:
  • IP
  • redmine
  • redmine.myCompany.com

Referencing the Setting class works if the instance of Redmine is accessed only from one URL.

From Eric:

request would be best.  Problem is, request is setup in a Rails Controller but plugin hooks are not Controllers.


Redmine version: 0.8

hook.rb.diff Magnifier - Patch for hook.rb (2.47 KB) Thomas Löber, 2009-02-02 10:00

hook.rb.revised.diff Magnifier - Revised patch (3.29 KB) Thomas Löber, 2009-02-02 11:58

hook_test_diff_for_link_test.patch Magnifier (1.48 KB) Chaoqun Zou, 2009-02-22 08:32


Related issues

Related to Redmine - Defect #2754: Setting up the default_url_options :port in hook will dup... Closed 2009-02-15
Duplicates Redmine - Feature #2649: Need @controller context in plugin hook(view_layouts_base... Closed 2009-02-02

Associated revisions

Revision 2429
Added by Eric Davis over 8 years ago

Added request and controller objects to the hooks by default.

The request and controller objects are now added to all hook contexts by
default. This will also make url_for work better in hooks by setting up
the default_url_options :host, :port, and :protocol.

Finally a new helper method render_or has been added to ViewListener. This
will let a hook easily render a partial without a full method definition.

Thanks to Thomas Löber for the original patch. #2542

Revision 2430
Added by Eric Davis over 8 years ago

Renamed variables to be more descriptive. #2542

Revision 2489
Added by Eric Davis over 8 years ago

Refactored the HookTest methods to use Redmine::Hook::Helper instead
of the Redmine::Hook class directly. Redmine::Hook::Helper is the
public API so it should be one that is tested.

#2542

Revision 2490
Added by Eric Davis over 8 years ago

Implementing the missing tests now that HookTest has a cleaner setup.

Controller and Views are now tested through the Redmine::Hook::Helper module.

#2542

Revision 2491
Added by Eric Davis over 8 years ago

Refactored the mess known as Hook default_url_options in favor of the simpler
:only_path as suggested by splatteal on GitHub.

#2542

Revision 2522
Added by Eric Davis over 8 years ago

Fixing Plugin and Mailer default_url_options.

Both the plugin hooks and Mailer were setting default_url_options incorrectly
and causing ActionContoller::UrlWritter to cache the settings on the module
(mattr_accessor) causing several url generators to fail in either the plugin
hooks or the Mailer.

  • Replaced Mailer's use of the default_url_options accessor with the proper class method
  • Replaced Hook's use of the default_url_options accessor with the proper class method on the ViewListener class
  • Added a test to reproduce the bugs in the Mailer when a hook is registered (thanks Chaoqun Zou)

    #2542

History

#1 Updated by Douglas Manley almost 9 years ago

It would be sweet if something like this could be done so that we don't need (as plugin developers) to access the "request" variable.

include ActionController::UrlWriter

default_url_options[:host] = request.env[ 'SERVER_NAME' ]
default_url_options[:port] = request.env[ 'SERVER_PORT' ]

#2 Updated by Eric Davis almost 9 years ago

Douglas Manley wrote:

It would be sweet if something like this could be done so that we don't need (as plugin developers) to access the "request" variable.

That would solve the host and port issue (would also need protocol) but request has a lot of other useful information. For example, a plugin might want to sniff the User Agent and resent a mobile view.

I'd also love to be able to use render in a hook but I know it's not trivial. Putting "view" code in a class as a string feels so dirty to me.

#3 Updated by Thomas Löber almost 9 years ago

This is how I would change hook.rb:

module Helper
  def call_hook(hook, context={})
    Redmine::Hook.call_hook(hook, {:project => @project, :request => request}.merge(context))
  end
end

And Redmine::Hook.call_hook:

# Calls a hook.
# Returns the listeners response.
def call_hook(hook, context={})
  returning "" do |response|
    hls = hook_listeners(hook)
    if hls.any?
      request = context[:request]
      if request
        default_url_options[:host] = request.env["SERVER_NAME"]
        default_url_options[:port] = request.env["SERVER_PORT"]
      end
      hls.each do |listener|
        response << listener.send(hook, context).to_s
      end
    end
  end
end

This works for me at least. I can use "link_to" in a hook listener and I can query context[:response].

What do you think?

#4 Updated by Thomas Löber almost 9 years ago

Eric Davis wrote:

I'd also love to be able to use render in a hook but I know it's not trivial. Putting "view" code in a class as a string feels so dirty to me.

If the call_hook method is extended like this:

module Helper
  def call_hook(hook, context={})
    ctx_init = {:project => @project, :controller => controller, :request => request}
    Redmine::Hook.call_hook(hook, ctx_init.merge(context))
  end
end

you can use render_to_string in your hook listener method:

def view_issues_show_details_bottom(context)
  controller = context[:controller]
  issue = context[:issue]
  controller.send(:render_to_string, :partial => "show_more_data", :locals => {:issue => issue})
end

It works, but I wonder if this is the correct way to do it. At least the HTML is in a (partial) view again.

#5 Updated by Thomas Löber almost 9 years ago

Actually the above method has to be:

module Helper
  def call_hook(hook, context={})
    ctx_init = {:project => @project, :request => request}
    ctx_init[:controller] = self.is_a?(ActionController::Base) ? self : controller
    Redmine::Hook.call_hook(hook, ctx_init.merge(context))
  end
end

Then it will work for controller and view hooks.

#6 Updated by Eric Davis almost 9 years ago

  • Assignee set to Eric Davis

Thomas thanks for the suggestions, I'll see what I can do.

#7 Updated by Thomas Löber over 8 years ago

This is how we can make calling render_to_string more straightforward:

In hook.rb

class ViewListener < Listener
  [...]
  def self.render_on(hook, options={})
    define_method hook do |context|
      context[:controller].send(:render_to_string, {:locals => context}.merge(options))
    end
  end
end

Then we can define a hook that renders its result using a partial view like this:

class MyHook < Redmine::Hook::ViewListener
  render_on :view_issues_show_details_bottom, :partial => "show_more_data" 
end

The partial view will have access to issue because it is given as a parameter in the call_hook call.

#8 Updated by Thomas Löber over 8 years ago

This is the complete patch.

I split the "call_hook" helper into two methods. One for controllers and one for views. The controller method returns an array of results (which is more appropriate if I want to handle the result inside a controller). The view method uses "join" to convert the array into a string.

#9 Updated by Thomas Löber over 8 years ago

As I just saw it was a bad idea to split the "call_hook" helper. It doesn't work if a controller includes the ApplicationHelper.

So here's a revised patch.

#10 Updated by Eric Davis over 8 years ago

  • Status changed from New to 7
  • Target version set to 0.8.1

Thomas Löber wrote:

As I just saw it was a bad idea to split the "call_hook" helper. It doesn't work if a controller includes the ApplicationHelper.

So here's a revised patch.

I'd like to apply this but I'm having a hard time testing it. Could you provide some tests for the different paths of:

  • Redmine::Hook.call_hook
  • Redmine::Hook::Helper.call_hook
  • Redmine::Hook::ViewListener.render_on

#11 Updated by Eric Davis over 8 years ago

  • % Done changed from 0 to 80

I found a way to test a few of the methods and committed your patch with some modifications in r2429 and r2430. I'd still like to add some tests for Redmine::Hook::Helper.call_hook and Redmine::Hook::ViewListener.render_on before closing this issue. You can find the tests at the bottom of test/unit/lib/redmine/hook_test.rb.

Commit details

Added request and controller objects to the hooks by default.

The request and controller objects are now added to all hook contexts by default. This will also make url_for work better in hooks by setting up the default_url_options :host, :port, and :protocol.

Finally a new helper method render_or has been added to ViewListener. This will let a hook easily render a partial without a full method definition.

#12 Updated by Jean-Philippe Lang over 8 years ago

  • Target version deleted (0.8.1)

See #2754

#13 Updated by Eric Davis over 8 years ago

  • Status changed from 7 to Resolved
  • % Done changed from 80 to 100

This should be finished now. Instead of using the request and setting host, port, and protocol directly I made the default_url_options use :only_path. This was a much cleaner implementation and should work for the majority of the plugin cases.

Any non standard case (like Douglas Manley) can set the host, port, and protocol in their plugin itself using the request context that's passed in:

context[:request].env["SERVER_NAME"]
context[:request].env["SERVER_PORT"]

A big thanks goes to Peter Suschlik for the idea.

Implemented in r2489 through r2491

#14 Updated by Eric Davis over 8 years ago

  • Target version set to 0.8.2

Adding to 0.8.2, since I couldn't reproduce #2754 and I don't think #2754 would be valid anymore after my refactoring.

#15 Updated by Peter Suschlik over 8 years ago

Great, thank you!

#16 Updated by Douglas Manley over 8 years ago

This should work just fine; thanks!

#17 Updated by Chaoqun Zou over 8 years ago

default_url_options[:only_path] ||= true

This will make the default_url_options[:only_path] to be true permanently. So the link in the notification mail will lost it's protocal, host and port.

#18 Updated by Eric Davis over 8 years ago

Chaoqun Zou wrote:

> default_url_options[:only_path] ||= true >

This will make the default_url_options[:only_path] to be true permanently. So the link in the notification mail will lost it's protocal, host and port.

Can you provide a test case showing the incorrect behavior? The email urls were being generated correctly for me (see #2754).

#19 Updated by Chaoqun Zou over 8 years ago

Here is a test (in hook_test unit test) for :only_path :

  fixtures :issues

  def test_call_hook_will_break_issue_link_in_mail
    issue = Issue.find(1)

    ActionMailer::Base.deliveries.clear
    Mailer.deliver_issue_add(issue)
    mail = ActionMailer::Base.deliveries.last
    puts mail.body

    @hook_module.add_listener(TestLinkToHook)
    @hook_helper.call_hook(:view_layouts_base_html_head)

    ActionMailer::Base.deliveries.clear
    Mailer.deliver_issue_add(issue)
    mail2 = ActionMailer::Base.deliveries.last
    puts mail2.body

    assert_equal mail.body, mail2.body
  end

This test will fail before the line 11-12 be commented out.

A diff patch file is attatched.

#20 Updated by Eric Davis over 8 years ago

I believe I fixed the Plugin and Mailer default_url_options in r2522. It turned out to be a tricky bug that was caused by using default_url_options incorrectly.

Both the plugin hooks and Mailer were setting default_url_options incorrectly and causing ActionContoller::UrlWritter to cache the settings on the module (mattr_accessor) causing several url generators to fail in either the plugin hooks or the Mailer. I found a good description of why this was happening on Stackoverflow.

Thanks for reporting the bug and providing the test case, it helped debugging.

#21 Updated by Jean-Philippe Lang over 8 years ago

  • Status changed from Resolved to Closed
  • Resolution set to Fixed

Merged in 0.8-stable in r2558.

Also available in: Atom PDF