From 43a40647bd72f60b9a567aaee0a80d6ab301db38 Mon Sep 17 00:00:00 2001
From: DenysGonchar <denys.gonchar@erlang-solutions.com>
Date: Fri, 16 Jul 2021 15:08:16 +0200
Subject: [PATCH] changes according to the review comment.

This branch introduces initial rework of the hooks framework

The key changes:
* hook handler functions now have fixed number of arguments:
   - Acc - the same meaning as before
   - Parameters map - input parameters are now provided in a form of map instead of variable number of handler function arguments
   - Extra map - every hook handler now can have a static context, in the same way as packet handlers. This map is automatically extended with 'hook_name' and 'hook_tag' ('global' or HostType) parameters on hook registration, so there is no need to add HostType into Parameters map any more.
* the old format of hook handlers is still supported (until the full conversion of all the hook handlers is done)
* conversion of the hooks can be done hook by hook, the following steps must be done to convert the hook:
   - convert hook execution function at 'mongoose_hooks' module.
   - convert all the handlers of the hook (they must be registered using gen_hook interface instead of ejabberd_hook).
   - also, it's possible to mix old & new hook handlers formats (see example at mongoose_hooks:push_notifications/4).
* note that 'gen_hook' accepts only external function references (in format "fun Module:Function/Arity', e.g. "fun erlang:is_function/1") as hook handler function parameter.
---
 big_tests/tests/sm_SUITE.erl | 7 ++++---
 src/ejabberd_hooks.erl       | 2 +-
 src/gen_hook.erl             | 7 +++----
 3 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/big_tests/tests/sm_SUITE.erl b/big_tests/tests/sm_SUITE.erl
index 21df63604c..bfb24245bb 100644
--- a/big_tests/tests/sm_SUITE.erl
+++ b/big_tests/tests/sm_SUITE.erl
@@ -1327,11 +1327,12 @@ stop_hook_listener(HookExtra) ->
     rpc(mim(), ?MODULE, rpc_stop_hook_handler, [HookExtra, host_type()]).
 
 rpc_start_hook_handler(TestCasePid, User, HostType) ->
-    LUser=jid:nodeprep(User),
+    LUser = jid:nodeprep(User),
+    Extra = #{luser => LUser, pid => TestCasePid},
     gen_hook:add_handler(unacknowledged_message, HostType,
                          fun ?MODULE:hook_handler_fn/3,
-                         #{luser => LUser, pid => TestCasePid}, 50),
-    #{luser => LUser, pid => TestCasePid}.
+                         Extra, 50),
+    Extra.
 
 rpc_stop_hook_handler(HookExtra, HostType) ->
     gen_hook:delete_handler(unacknowledged_message, HostType,
diff --git a/src/ejabberd_hooks.erl b/src/ejabberd_hooks.erl
index 329ae543f0..58d53353cc 100644
--- a/src/ejabberd_hooks.erl
+++ b/src/ejabberd_hooks.erl
@@ -95,7 +95,7 @@ add_args(HookParams, LegacyArgsList) ->
 add_hook({HookName, HostType, Module, Function, Priority}) when is_atom(Function) ->
     gen_hook:add_handler(HookName, HostType,
                          fun ?MODULE:gen_hook_fn_wrapper/3,
-                         #{module => Module, function =>Function},
+                         #{module => Module, function => Function},
                          Priority).
 
 -spec delete_hook(hook()) -> ok.
diff --git a/src/gen_hook.erl b/src/gen_hook.erl
index 0fd4b72f25..e3a7f6b7f1 100644
--- a/src/gen_hook.erl
+++ b/src/gen_hook.erl
@@ -106,13 +106,13 @@ delete_handler(HookTuple) ->
     gen_server:call(?MODULE, {delete_handler, Handler}).
 
 %% @doc Run hook handlers in order of priority (lower number means higher priority).
-%%  * if hook handler returns {ok, NewAcc}, the NewAcc value is used
+%%  * if a hook handler returns {ok, NewAcc}, the NewAcc value is used
 %%    as an accumulator parameter for the following hook handler.
 %%  * if a hook handler returns {stop, NewAcc}, execution stops immediately
 %%    without invoking lower priority hook handlers.
-%%  * if hook handler crashes, the error is logged and the next hook handler
+%%  * if a hook handler crashes, the error is logged and the next hook handler
 %%    is executed.
-%% Note that every hook handler MUST return a valid Acc. If hook handler is not
+%% Note that every hook handler MUST return a valid Acc. If a hook handler is not
 %% interested in changing Acc parameter (or even if Acc is not used for a hook
 %% at all), it must return (pass through) an unchanged input accumulator value.
 -spec run_fold(HookName :: hook_name(),
@@ -255,7 +255,6 @@ check_hook_function(Function) when is_function(Function, 3) ->
 
 -spec throw_error(map()) -> no_return().
 throw_error(ErrorMap) ->
-    ?LOG_ERROR(ErrorMap),
     error(ErrorMap).
 
 -spec hook_key(HookName :: hook_name(), Tag :: hook_tag()) -> key().