Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-44078

Proposal: API standard in Moodle that uses autoloading (hooks)

    Details

    • Testing Instructions:
      Hide

      Add db/hooks.php to some plugin with a callback for \core\hook\pre_delete_course
      Output something in this callback.
      Create and delete the course
      Make sure the output was printed

      Show
      Add db/hooks.php to some plugin with a callback for \core\hook\pre_delete_course Output something in this callback. Create and delete the course Make sure the output was printed
    • Affected Branches:
      MOODLE_26_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-44078-master

      Description

      At the moment Moodle has plenty of little APIs (consisting of one-two functions each) that require plugins to define PLUGINNAME_CALLBACKNAME() functions in their lib.php files.

      Examples:

      • xxx_print_recent_activity
      • xxx_get_recent_mod_activity
      • xxx_reset_course_form_definition
      • xxx_comment_display
      • xxx_grade_item_update
      • xxx_add_istance_hook
      • xxx_restore_group_member
      • xxx_restore_group_assignment
      • xxx_delete_instance
      • xxx_delete_course
      • xxx_supports
        ... (and so on, just grep by "component_callback" or "function_exists" )

      There are several problems with this approach:

      • bad documentation, hard to know about all various existing callbacks
      • easy to confuse the arguments because there is no interface
      • necessary to include all lib.php files (which are sometimes huge) to find handful of plugins implementing functions - SLOW!
      • limiting implementation to one plugin type because otherwise it's too expensive to look for callbacks

      Suggested approach that was discussed on backend meetings, especially with Petr Skoda is to use hooks and class autoloading.

      There is no goal to convert all existing APIs into the new format ASAP but will be nice to gradually do it, slowly deprecating old-style callbacks.

      There are at least two issues that would benefit from the agreement on standard:

      • MDL-43742 converting recent activity callbacks (atm we need to implement two very similar callbacks in each module)
      • MDL-24359 extending course reset callbacks to other plugin types (i.e. blocks)

      Forum post: https://moodle.org/mod/forum/discuss.php?d=254508

        Gliffy Diagrams

          Attachments

            Issue Links

              Activity

              Hide
              marina Marina Glancy added a comment -

              Proposal for specification: http://docs.moodle.org/dev/Hooks_spec

              Show
              marina Marina Glancy added a comment - Proposal for specification: http://docs.moodle.org/dev/Hooks_spec
              Hide
              skodak Petr Skoda added a comment -

              here is my little demo of the code (totally untested) https://github.com/skodak/moodle/commits/wip_MDL-44078_m27_hooks

              Show
              skodak Petr Skoda added a comment - here is my little demo of the code (totally untested) https://github.com/skodak/moodle/commits/wip_MDL-44078_m27_hooks
              Hide
              dougiamas Martin Dougiamas added a comment -

              It's a nice idea for a new system but also quite low priority IMO

              Show
              dougiamas Martin Dougiamas added a comment - It's a nice idea for a new system but also quite low priority IMO
              Hide
              timhunt Tim Hunt added a comment -

              There is no point doing this unless we actually clean up all the old ways of doing things. Your complaint seems to be that there are many different ways to do things in Moodle, and you plan to make it worse by adding another way.

              I do not see the need. In the past we have succeeded in making all add-ons in Moodle more consistent without major changes. E.g. all plugins use db folder in the same way. All plugins use settings.php. Let us continue that, and not add a new way of doing things that is applied inconsistently.

              Show
              timhunt Tim Hunt added a comment - There is no point doing this unless we actually clean up all the old ways of doing things. Your complaint seems to be that there are many different ways to do things in Moodle, and you plan to make it worse by adding another way. I do not see the need. In the past we have succeeded in making all add-ons in Moodle more consistent without major changes. E.g. all plugins use db folder in the same way. All plugins use settings.php. Let us continue that, and not add a new way of doing things that is applied inconsistently.
              Hide
              damyon Damyon Wiese added a comment -

              That proposal seems a bit messy to me.

              I would prefer something tied to plugininfo.

              It is more natural to say a plugin supports an API which means it must implement ALL of the multiple hooks to match that API. The best way I can imagine is for plugininfo to implement an interface.

              Show
              damyon Damyon Wiese added a comment - That proposal seems a bit messy to me. I would prefer something tied to plugininfo. It is more natural to say a plugin supports an API which means it must implement ALL of the multiple hooks to match that API. The best way I can imagine is for plugininfo to implement an interface.
              Hide
              damyon Damyon Wiese added a comment -

              Actually the hooks thing could work with using an interface for an API - ie - you define a handler for a hook and it must extend an abstract class/interface.

              Show
              damyon Damyon Wiese added a comment - Actually the hooks thing could work with using an interface for an API - ie - you define a handler for a hook and it must extend an abstract class/interface.
              Hide
              philipl-catalyst Phil Lello added a comment -

              I quite like the hooks proposal, however I'd be tempted to add a weight parameter to tweak execution order of a hook relative to other hooks (ideally some core functionality would migrate to hooks to support this). I'd also be tempted to have a plugin register callbacks via install.php/upgrade.php, although I suspect that was previously discussed for the events API.

              Show
              philipl-catalyst Phil Lello added a comment - I quite like the hooks proposal, however I'd be tempted to add a weight parameter to tweak execution order of a hook relative to other hooks (ideally some core functionality would migrate to hooks to support this). I'd also be tempted to have a plugin register callbacks via install.php/upgrade.php, although I suspect that was previously discussed for the events API.
              Hide
              marina Marina Glancy added a comment -

              Hi Phil, thanks. The 'priority' property can be set in observer, as it does in http://docs.moodle.org/dev/Event_2#Event_observers
              (while looking for a reference I noticed that I put the wrong link to Events API in the hooks document, correcting now)

              Show
              marina Marina Glancy added a comment - Hi Phil, thanks. The 'priority' property can be set in observer, as it does in http://docs.moodle.org/dev/Event_2#Event_observers (while looking for a reference I noticed that I put the wrong link to Events API in the hooks document, correcting now)
              Hide
              quen Sam Marshall added a comment -

              I like this proposal & have voted for it. It, or something like it, would make it possible to tidy up a current hardcoded hack in the new conditional availability stuff.

              Show
              quen Sam Marshall added a comment - I like this proposal & have voted for it. It, or something like it, would make it possible to tidy up a current hardcoded hack in the new conditional availability stuff.
              Hide
              simoncoggins Simon Coggins added a comment -

              I think this would be extremely helpful for us at Totara (or anyone wanting to do extensive customisation of Moodle) to allow us to extend Moodle without having to hack core files.

              Anything which would make the process of merging new versions easier is likely to benefit Moodle customers as it makes it easier for clients with customised sites to stay up to date with recent releases (often the work of merging new versions into a customised site is what puts people off upgrading).

              One example of the kind of uses we would have are, for example, when we want to add additional fields to the course editing page we need to customise core code:

              https://github.com/moodlehq/totara/blob/t2-release-2.5/course/edit_form.php#L379
              https://github.com/moodlehq/totara/blob/t2-release-2.5/course/edit_form.php#L428
              https://github.com/moodlehq/totara/blob/t2-release-2.5/course/edit_form.php#L468

              There are many, many other places like this where we have to include a library and insert a function call - it would be really great to have a standard way to do this.

              Show
              simoncoggins Simon Coggins added a comment - I think this would be extremely helpful for us at Totara (or anyone wanting to do extensive customisation of Moodle) to allow us to extend Moodle without having to hack core files. Anything which would make the process of merging new versions easier is likely to benefit Moodle customers as it makes it easier for clients with customised sites to stay up to date with recent releases (often the work of merging new versions into a customised site is what puts people off upgrading). One example of the kind of uses we would have are, for example, when we want to add additional fields to the course editing page we need to customise core code: https://github.com/moodlehq/totara/blob/t2-release-2.5/course/edit_form.php#L379 https://github.com/moodlehq/totara/blob/t2-release-2.5/course/edit_form.php#L428 https://github.com/moodlehq/totara/blob/t2-release-2.5/course/edit_form.php#L468 There are many, many other places like this where we have to include a library and insert a function call - it would be really great to have a standard way to do this.
              Hide
              dougiamas Martin Dougiamas added a comment -

              It's useful for open source code to be openly available, eh?

              Show
              dougiamas Martin Dougiamas added a comment - It's useful for open source code to be openly available, eh?
              Hide
              simoncoggins Simon Coggins added a comment -

              Yeah very handy, thanks Martin

              Show
              simoncoggins Simon Coggins added a comment - Yeah very handy, thanks Martin
              Hide
              mudrd8mz David Mudrák added a comment -

              I did not spend enough time on thinking about this to comment on particular implementation details (such as the name of the "create" method etc). But conceptually, I vote +1 for this or something similar to this.

              I can confirm that, from a module's developer point of view, the current way of lib.php callbacks got out of control. We don't know or have a full list of all supported (or even required!) callbacks. One example is MDL-38210 where it turned out that after all the years of Workshop 2.x being in production, it is still missing some callbacks like that. And I remember I paid extra attention to collect them all when I was writing the Workshop lib. Attempting to maintain the list of these callbacks (little apies) in places like NEWMODULE.zip is unmaintainable (I tried, it never ends and does not really help for existing plugins).

              Also, the fact we have to load lib.php from all installed plugins in almost all requests, just smells.

              So, on behalf of all those great people who spend their free time on contributing to Moodle plugins, please :Make it in a way that the plugin developer has easy access of all required and supported APIs - and is informed if the required callback implementation is missing.

              Show
              mudrd8mz David Mudrák added a comment - I did not spend enough time on thinking about this to comment on particular implementation details (such as the name of the "create" method etc). But conceptually, I vote +1 for this or something similar to this. I can confirm that, from a module's developer point of view, the current way of lib.php callbacks got out of control. We don't know or have a full list of all supported (or even required!) callbacks. One example is MDL-38210 where it turned out that after all the years of Workshop 2.x being in production, it is still missing some callbacks like that. And I remember I paid extra attention to collect them all when I was writing the Workshop lib. Attempting to maintain the list of these callbacks (little apies) in places like NEWMODULE.zip is unmaintainable (I tried, it never ends and does not really help for existing plugins). Also, the fact we have to load lib.php from all installed plugins in almost all requests, just smells. So, on behalf of all those great people who spend their free time on contributing to Moodle plugins, please :Make it in a way that the plugin developer has easy access of all required and supported APIs - and is informed if the required callback implementation is missing.
              Hide
              marina Marina Glancy added a comment -

              ok, I guess everybody agrees to close this issue as "Won't fix".

              Let's introduce 1001st and 1002nd callbacks in moodle in MDL-24359 and MDL-40457 and include all plugins on each page in order to check if they implement callbacks. Modern computers are too fast to care about performance. Developers are smart enough to read millions of code lines to know which callback to implement.

              Thanks to everybody who supported this idea but unfortunately it is not happenning.

              Show
              marina Marina Glancy added a comment - ok, I guess everybody agrees to close this issue as "Won't fix". Let's introduce 1001st and 1002nd callbacks in moodle in MDL-24359 and MDL-40457 and include all plugins on each page in order to check if they implement callbacks. Modern computers are too fast to care about performance. Developers are smart enough to read millions of code lines to know which callback to implement. Thanks to everybody who supported this idea but unfortunately it is not happenning.
              Hide
              marina Marina Glancy added a comment - - edited

              lol, just as I wrote it another issue was reported - MDL-44678. Well, course reset now has to include lib.php from all plugins of 3 plugin types now (module, block and plagiarism)

              Show
              marina Marina Glancy added a comment - - edited lol, just as I wrote it another issue was reported - MDL-44678 . Well, course reset now has to include lib.php from all plugins of 3 plugin types now (module, block and plagiarism)
              Hide
              raymor Ray Morris added a comment -

              "everybody agrees to close this issue as "Won't fix"."

              Tabulating the comments above, I count 7 yay and 2 nay. Did "everybody" agree on some other forum?
              I'm with David Mudrak - I haven't analyzed the proposal in detail, but I agree that as Moodle grows the existing approaced is being stretched. Something similar to this proposal would increase the modularity of Moodle.

              However, I also see Tim's point, also made by Randall Munroe https://xkcd.com/927/
              Considering Damyon's comment, is it possible to improve this proposal to the point where we'd definitely want it to be the new standard Moodle way, and therefore plan to replace API using lib.php over time?

              Show
              raymor Ray Morris added a comment - "everybody agrees to close this issue as "Won't fix"." Tabulating the comments above, I count 7 yay and 2 nay. Did "everybody" agree on some other forum? I'm with David Mudrak - I haven't analyzed the proposal in detail, but I agree that as Moodle grows the existing approaced is being stretched. Something similar to this proposal would increase the modularity of Moodle. However, I also see Tim's point, also made by Randall Munroe https://xkcd.com/927/ Considering Damyon's comment, is it possible to improve this proposal to the point where we'd definitely want it to be the new standard Moodle way, and therefore plan to replace API using lib.php over time?
              Hide
              raymor Ray Morris added a comment - - edited

              After having read over the proposal with some care, I like it generally. It reminds me of the Apache hook based module API, which has worked very well for many years.

              I wouldn't argue with Damyon that perhaps they should implement an interface (http://www.php.net/manual/en/language.oop5.interfaces.php) rather than hooking single function. It may be reasonable to think that a plugin which hooks send_query() might be required to hook get_results(). However, hooking a function is a little different than implementing the functionality. The fact that the current system (and the Apache system) effectively hook individual functions shows that doing that works fine too.

              Show
              raymor Ray Morris added a comment - - edited After having read over the proposal with some care, I like it generally. It reminds me of the Apache hook based module API, which has worked very well for many years. I wouldn't argue with Damyon that perhaps they should implement an interface ( http://www.php.net/manual/en/language.oop5.interfaces.php ) rather than hooking single function. It may be reasonable to think that a plugin which hooks send_query() might be required to hook get_results(). However, hooking a function is a little different than implementing the functionality. The fact that the current system (and the Apache system) effectively hook individual functions shows that doing that works fine too.
              Hide
              dougiamas Martin Dougiamas added a comment -

              Eh? I think most people think this is a great idea, Marina. It just needs some cooking and a considered approach to rollout.

              Show
              dougiamas Martin Dougiamas added a comment - Eh? I think most people think this is a great idea, Marina. It just needs some cooking and a considered approach to rollout.
              Hide
              damyon Damyon Wiese added a comment -

              And to clarify my comment - I am not against changing the way we implement APIs - the current lib.php approach has many problems and I had exactly the same experience as David when writing the new mod_assign.

              That is why I favour an interface based approach - which is not what is proposed here - and not covered by Petrs patch.

              I would rather a plugin provides a handler for an API and that handler is required to be a subclass that implements an interface (or extends an abstract class - no difference). That is the only way you can be sure that a plugin that implements grading contains all the required callbacks to properly implement grading.

              IMO this hooks patch is too weak and is not much better than the lib.php approach (it will perform better - but that's not my main concern).

              Show
              damyon Damyon Wiese added a comment - And to clarify my comment - I am not against changing the way we implement APIs - the current lib.php approach has many problems and I had exactly the same experience as David when writing the new mod_assign. That is why I favour an interface based approach - which is not what is proposed here - and not covered by Petrs patch. I would rather a plugin provides a handler for an API and that handler is required to be a subclass that implements an interface (or extends an abstract class - no difference). That is the only way you can be sure that a plugin that implements grading contains all the required callbacks to properly implement grading. IMO this hooks patch is too weak and is not much better than the lib.php approach (it will perform better - but that's not my main concern).
              Hide
              skodak Petr Skoda added a comment -

              Interfaces are definitely not a solution for general hacking of core functionality from arbitrary plugin, that is a separate topic. Interfaces may get pretty ugly during upgrades because the mandate all plugins to implement them otherwise you get fatal error. In any case we cannot load classes from all plugins in the system and start asking them if they implement this or that interface.

              So yes, interfaces and abstract classes are good for activity modules, but we cannot use them for general hacks, sorry.

              Show
              skodak Petr Skoda added a comment - Interfaces are definitely not a solution for general hacking of core functionality from arbitrary plugin, that is a separate topic. Interfaces may get pretty ugly during upgrades because the mandate all plugins to implement them otherwise you get fatal error. In any case we cannot load classes from all plugins in the system and start asking them if they implement this or that interface. So yes, interfaces and abstract classes are good for activity modules, but we cannot use them for general hacks, sorry.
              Hide
              stronk7 Eloy Lafuente (stronk7) added a comment -

              Just to state that I'd love to see organized (explicitly defined, described and used) hooks landing and getting the baton over current callbacks. Surely in a similar way to events. Although the trickiest part will be to define which information are they able to handle/modify and also, at which level (example: a hook(s) for a given form, or a hook(s) for all forms).

              In the other side, I think this only can be considered together with the adaptation of a lot of current callbacks to use the new system, breaking everything the minimum number of times.

              About when an imminent 2.7 LTS is a good moment for that, or which are the policies that will follow that LTS (try keep things unmodified or freedom to break), I still don't know about them, so I'm not able to recommend a good moment for this to land, just now it does not seem to be the best moment.

              Ciao

              Show
              stronk7 Eloy Lafuente (stronk7) added a comment - Just to state that I'd love to see organized (explicitly defined, described and used) hooks landing and getting the baton over current callbacks. Surely in a similar way to events. Although the trickiest part will be to define which information are they able to handle/modify and also, at which level (example: a hook(s) for a given form, or a hook(s) for all forms). In the other side, I think this only can be considered together with the adaptation of a lot of current callbacks to use the new system, breaking everything the minimum number of times. About when an imminent 2.7 LTS is a good moment for that, or which are the policies that will follow that LTS (try keep things unmodified or freedom to break), I still don't know about them, so I'm not able to recommend a good moment for this to land, just now it does not seem to be the best moment. Ciao
              Hide
              marina Marina Glancy added a comment -

              Well, this proposal is around since April 2013. Since then it's always "not a good time now" and "not a high priority" and so on. For me it only means that it's not happening.

              My initial idea was also interface but I tend to agree with Petr now that having hook as a class and callback as a method is faster and easier for plugin developers.

              Show
              marina Marina Glancy added a comment - Well, this proposal is around since April 2013. Since then it's always "not a good time now" and "not a high priority" and so on. For me it only means that it's not happening. My initial idea was also interface but I tend to agree with Petr now that having hook as a class and callback as a method is faster and easier for plugin developers.
              Hide
              dougiamas Martin Dougiamas added a comment -

              Personally I think it's a requirement of a big API change like this that we DO roll it out all at once and make the change fully and completely. With backward compatibility for 3rd party plugins.

              That's why I see this is as probably a huge decision, and we need more data (how much work this is, how much faster it will be, how the interfaces will be for developers, what the migration looks like) and more consensus from lots of developers.

              Show
              dougiamas Martin Dougiamas added a comment - Personally I think it's a requirement of a big API change like this that we DO roll it out all at once and make the change fully and completely. With backward compatibility for 3rd party plugins. That's why I see this is as probably a huge decision, and we need more data (how much work this is, how much faster it will be, how the interfaces will be for developers, what the migration looks like) and more consensus from lots of developers.
              Hide
              damyon Damyon Wiese added a comment -

              faster and easier for plugin hook developers

              and will lead to the same situation we have now that the complete list of available hooks is not listed/documented anywhere.

              Show
              damyon Damyon Wiese added a comment - faster and easier for plugin hook developers and will lead to the same situation we have now that the complete list of available hooks is not listed/documented anywhere.
              Hide
              marina Marina Glancy added a comment -

              no, hook as class is more difficult for hook developer and easier for plugin developer who interacts with this hook because plugin developer does not need to define a class and all methods in it, he just registers one function with one argument.

              Show
              marina Marina Glancy added a comment - no, hook as class is more difficult for hook developer and easier for plugin developer who interacts with this hook because plugin developer does not need to define a class and all methods in it, he just registers one function with one argument.
              Hide
              damyon Damyon Wiese added a comment -

              I still disagree - but I won't keep harping on - the specific example listed in the dev docs shows why it would be better to use an interface/api approach rather than lots of once off functions. This is because you never want to just add fields to a form - you want to add fields, and then have custom validation for them, and save the submitted data somewhere. So that would be 3 hooks required to extend a form - they are all really required (validation could be optional) - but 3 having separate hooks for the form does not link them together in any meaningful way. It's really the same as a list of functions in lib.php.

              Show
              damyon Damyon Wiese added a comment - I still disagree - but I won't keep harping on - the specific example listed in the dev docs shows why it would be better to use an interface/api approach rather than lots of once off functions. This is because you never want to just add fields to a form - you want to add fields, and then have custom validation for them, and save the submitted data somewhere. So that would be 3 hooks required to extend a form - they are all really required (validation could be optional) - but 3 having separate hooks for the form does not link them together in any meaningful way. It's really the same as a list of functions in lib.php.
              Hide
              philipl-catalyst Phil Lello added a comment -

              OK, how about we have the following:

              (at core install/upgrade time) : register_hook_provider("hookable-class", "hookable-function", "description");

              (at plugin install/upgrade time) : register_hook_client("hook-client-class", "hook-client-function", "description");

              That way we can have a status page listing all available hooks and all hook clients. It would be useful to have a "rebuild" option on this page to re-run register_hook_provider/register_hook_client code while new ones are developed.

              I haven't thought through the exact syntax, so the signatures are probably wrong, but hopefully the idea is clear enough

              Show
              philipl-catalyst Phil Lello added a comment - OK, how about we have the following: (at core install/upgrade time) : register_hook_provider("hookable-class", "hookable-function", "description"); (at plugin install/upgrade time) : register_hook_client("hook-client-class", "hook-client-function", "description"); That way we can have a status page listing all available hooks and all hook clients. It would be useful to have a "rebuild" option on this page to re-run register_hook_provider/register_hook_client code while new ones are developed. I haven't thought through the exact syntax, so the signatures are probably wrong, but hopefully the idea is clear enough
              Hide
              marina Marina Glancy added a comment - - edited

              2Damyon:

              this is still possible with one hook:

              class block_myblock_hook_callbacks {
                  public static function courseresetform(\core_course\hook\courseresetform $courseresetformhook) {
                      $courseresetformhook->mform->addElement('checkbox', 'Reset contents of block Myblock');
                      $courseresetformhook->register_validation_function('block_myblock_hook_callbacks::validate');
                      $courseresetformhook->register_submit_function('block_myblock_hook_callbacks::submit');
                  }
                  public static function validate($mform) {}
                  public static function submit($data) {}
              }
              

              maybe not very attractive in this particular case but most of existing callbacks just ask to return some values

              Show
              marina Marina Glancy added a comment - - edited 2Damyon: this is still possible with one hook: class block_myblock_hook_callbacks { public static function courseresetform(\core_course\hook\courseresetform $courseresetformhook) { $courseresetformhook->mform->addElement('checkbox', 'Reset contents of block Myblock'); $courseresetformhook->register_validation_function('block_myblock_hook_callbacks::validate'); $courseresetformhook->register_submit_function('block_myblock_hook_callbacks::submit'); } public static function validate($mform) {} public static function submit($data) {} } maybe not very attractive in this particular case but most of existing callbacks just ask to return some values
              Hide
              marina Marina Glancy added a comment -

              2Phil:
              what are advantages of this approach comparing to suggested way http://docs.moodle.org/dev/Hooks_spec ?

              Show
              marina Marina Glancy added a comment - 2Phil: what are advantages of this approach comparing to suggested way http://docs.moodle.org/dev/Hooks_spec ?
              Hide
              damyon Damyon Wiese added a comment -

              Something like that would be fine Marina - and even better if the code defining the hooks could make some callbacks required, and some optional.

              Show
              damyon Damyon Wiese added a comment - Something like that would be fine Marina - and even better if the code defining the hooks could make some callbacks required, and some optional.
              Hide
              marina Marina Glancy added a comment -

              Any validation is up to hook developer. Also it might be a good practice to enclose invoking of plugin functions into try/catch. In this case some hacky 3rd party plugin can't break the page completely. Which btw can happen in current moodle callbacks

              Show
              marina Marina Glancy added a comment - Any validation is up to hook developer. Also it might be a good practice to enclose invoking of plugin functions into try/catch. In this case some hacky 3rd party plugin can't break the page completely. Which btw can happen in current moodle callbacks
              Hide
              stronk7 Eloy Lafuente (stronk7) added a comment -

              I'd love to see organized (explicitly defined, described and used)...
              — Eloy, some comments above.

              I think the proposed spec is fair enough with that:

              1) all hooks are self-documented (coz all them are using @category and level 2 namespace "hook")
              2) all uses are well defined (coz require db/xxxx.php describing them). Not sure if "hooks.php" is the name for them, and how strictly they should be normalized, as far as they are going to call really heterogeneous code, but that's another story.

              Show
              stronk7 Eloy Lafuente (stronk7) added a comment - I'd love to see organized (explicitly defined, described and used)... — Eloy, some comments above. I think the proposed spec is fair enough with that: 1) all hooks are self-documented (coz all them are using @category and level 2 namespace "hook") 2) all uses are well defined (coz require db/xxxx.php describing them). Not sure if "hooks.php" is the name for them, and how strictly they should be normalized, as far as they are going to call really heterogeneous code, but that's another story.
              Hide
              marina Marina Glancy added a comment - - edited

              Upon Martin's request, here is the branch that shows all existing callbacks in Moodle at the moment (more appear in each integration cycle):
              https://github.com/marinaglancy/moodle/compare/x-int-master...wip-MDL-44078-master-hooks

              And just plain list of non-deprecated callbacks:

              COMPONENT_pluginfile
              FULLPLUGINNAME_add_instance_hook
              FULLPLUGINNAME_allow_group_member_remove
              (FULL)PLUGINNAME_cm_info_dynamic
              (FULL)PLUGINNAME_cm_info_view
              FULLPLUGINNAME_comment_add
              FULLPLUGINNAME_comment_display
              FULLPLUGINNAME_comment_permissions
              FULLPLUGINNAME_comment_template
              FULLPLUGINNAME_comment_url
              FULLPLUGINNAME_comment_validate
              FULLPLUGINNAME_delete_course
              FULLPLUGINNAME_dndupload_handle
              (FULL)PLUGINNAME_dndupload_register
              FULLPLUGINNAME_extend_navigation_course
              FULLPLUGINNAME_extend_navigation_module
              FULLPLUGINNAME_extend_navigation_user
              FULLPLUGINNAME_extend_settings_navigation
              FULLPLUGINNAME_extends_navigation
              FULLPLUGINNAME_extends_settings_navigation
              (FULL)PLUGINNAME_get_file_areas
              (FULL)PLUGINNAME_get_file_info
              FULLPLUGINNAME_get_post_actions
              FULLPLUGINNAME_get_question_bank_search_conditions
              FULLPLUGINNAME_get_types
              FULLPLUGINNAME_get_view_actions
              FULLPLUGINNAME_global_db_replace
              FULLPLUGINNAME_grade_item_update
              FULLPLUGINNAME_grading_areas_list
              FULLPLUGINNAME_page_init
              (FULL)PLUGINNAME_page_type_list
              FULLPLUGINNAME_page_type_list
              FULLPLUGINNAME_params_for_js
              (FULL)PLUGINNAME_pluginfile
              FULLPLUGINNAME_pluginfile
              (FULL)PLUGINNAME_print_overview
              FULLPLUGINNAME_print_recent_activity
              FULLPLUGINNAME_question_list_instances
              (FULL)PLUGINNAME_question_pluginfile
              FULLPLUGINNAME_question_pluginfile
              FULLPLUGINNAME_question_preview_pluginfile
              FULLPLUGINNAME_questions_in_use
              FULLPLUGINNAME_rating_get_item_fields
              FULLPLUGINNAME_rating_permissions
              FULLPLUGINNAME_rating_validate
              FULLPLUGINNAME_refresh_events
              FULLPLUGINNAME_restore_group_member
              FULLPLUGINNAME_restore_role_assignment
              FULLPLUGINNAME_strings_for_js
              (FULL)PLUGINNAME_supports
              FULLPLUGINNAME_supports_logstore
              FULLPLUGINNAME_XXX
              glossary_print_entry_FORMATNAME
              glossary_show_entry_FORMATNAME
              grade_export_PLUGINNAME_settings_definition
              grade_import_PLUGINNAME_settings_definition
              grade_report_PLUGINNAME_profilereport
              grade_report_PLUGINNAME_settings_definition
              PLUGINNAME_delete_course
              PLUGINNAME_delete_instance
              PLUGINNAME_export_contents
              PLUGINNAME_extend_settings_navigation
              PLUGINNAME_extends_navigation
              PLUGINNAME_get_completion_state
              PLUGINNAME_get_coursemodule_info
              PLUGINNAME_get_extra_capabilities
              PLUGINNAME_get_post_actions
              PLUGINNAME_get_recent_mod_activity
              PLUGINNAME_get_types
              PLUGINNAME_get_view_actions
              PLUGINNAME_grade_item_update
              PLUGINNAME_print_overview
              PLUGINNAME_print_recent_activity
              PLUGINNAME_print_recent_mod_activity
              PLUGINNAME_report_extend_navigation
              PLUGINNAME_report_extends_navigation
              PLUGINNAME_reset_course_form_defaults
              PLUGINNAME_reset_course_form_definition
              PLUGINNAME_reset_userdata
              PLUGINNAME_rss_get_feed
              PLUGINNAME_scale_used_anywhere
              PLUGINNAME_supports
              PLUGINNAME_update_grades
              PLUGINNAME_user_complete
              PLUGINNAME_user_outline
              $THEME->csspostprocess
              $THEME->extralesscallback
              $THEME->lessvariablescallback
              xmldb_FULLPLUGINNAME_install_recovery
              xmldb_(FULL)PLUGINNAME_uninstall
              

              where
              FULLPLUGINNAME = mod_workshop, block_course_overview, report_participation
              PLUGINNAME = workshop, course_overview, participation
              (FULL)PLUGINNAME = mod_workshop, workshop, block_course_overview, report_participation

              Show
              marina Marina Glancy added a comment - - edited Upon Martin's request, here is the branch that shows all existing callbacks in Moodle at the moment (more appear in each integration cycle): https://github.com/marinaglancy/moodle/compare/x-int-master...wip-MDL-44078-master-hooks And just plain list of non-deprecated callbacks: COMPONENT_pluginfile FULLPLUGINNAME_add_instance_hook FULLPLUGINNAME_allow_group_member_remove (FULL)PLUGINNAME_cm_info_dynamic (FULL)PLUGINNAME_cm_info_view FULLPLUGINNAME_comment_add FULLPLUGINNAME_comment_display FULLPLUGINNAME_comment_permissions FULLPLUGINNAME_comment_template FULLPLUGINNAME_comment_url FULLPLUGINNAME_comment_validate FULLPLUGINNAME_delete_course FULLPLUGINNAME_dndupload_handle (FULL)PLUGINNAME_dndupload_register FULLPLUGINNAME_extend_navigation_course FULLPLUGINNAME_extend_navigation_module FULLPLUGINNAME_extend_navigation_user FULLPLUGINNAME_extend_settings_navigation FULLPLUGINNAME_extends_navigation FULLPLUGINNAME_extends_settings_navigation (FULL)PLUGINNAME_get_file_areas (FULL)PLUGINNAME_get_file_info FULLPLUGINNAME_get_post_actions FULLPLUGINNAME_get_question_bank_search_conditions FULLPLUGINNAME_get_types FULLPLUGINNAME_get_view_actions FULLPLUGINNAME_global_db_replace FULLPLUGINNAME_grade_item_update FULLPLUGINNAME_grading_areas_list FULLPLUGINNAME_page_init (FULL)PLUGINNAME_page_type_list FULLPLUGINNAME_page_type_list FULLPLUGINNAME_params_for_js (FULL)PLUGINNAME_pluginfile FULLPLUGINNAME_pluginfile (FULL)PLUGINNAME_print_overview FULLPLUGINNAME_print_recent_activity FULLPLUGINNAME_question_list_instances (FULL)PLUGINNAME_question_pluginfile FULLPLUGINNAME_question_pluginfile FULLPLUGINNAME_question_preview_pluginfile FULLPLUGINNAME_questions_in_use FULLPLUGINNAME_rating_get_item_fields FULLPLUGINNAME_rating_permissions FULLPLUGINNAME_rating_validate FULLPLUGINNAME_refresh_events FULLPLUGINNAME_restore_group_member FULLPLUGINNAME_restore_role_assignment FULLPLUGINNAME_strings_for_js (FULL)PLUGINNAME_supports FULLPLUGINNAME_supports_logstore FULLPLUGINNAME_XXX glossary_print_entry_FORMATNAME glossary_show_entry_FORMATNAME grade_export_PLUGINNAME_settings_definition grade_import_PLUGINNAME_settings_definition grade_report_PLUGINNAME_profilereport grade_report_PLUGINNAME_settings_definition PLUGINNAME_delete_course PLUGINNAME_delete_instance PLUGINNAME_export_contents PLUGINNAME_extend_settings_navigation PLUGINNAME_extends_navigation PLUGINNAME_get_completion_state PLUGINNAME_get_coursemodule_info PLUGINNAME_get_extra_capabilities PLUGINNAME_get_post_actions PLUGINNAME_get_recent_mod_activity PLUGINNAME_get_types PLUGINNAME_get_view_actions PLUGINNAME_grade_item_update PLUGINNAME_print_overview PLUGINNAME_print_recent_activity PLUGINNAME_print_recent_mod_activity PLUGINNAME_report_extend_navigation PLUGINNAME_report_extends_navigation PLUGINNAME_reset_course_form_defaults PLUGINNAME_reset_course_form_definition PLUGINNAME_reset_userdata PLUGINNAME_rss_get_feed PLUGINNAME_scale_used_anywhere PLUGINNAME_supports PLUGINNAME_update_grades PLUGINNAME_user_complete PLUGINNAME_user_outline $THEME->csspostprocess $THEME->extralesscallback $THEME->lessvariablescallback xmldb_FULLPLUGINNAME_install_recovery xmldb_(FULL)PLUGINNAME_uninstall where FULLPLUGINNAME = mod_workshop, block_course_overview, report_participation PLUGINNAME = workshop, course_overview, participation (FULL)PLUGINNAME = mod_workshop, workshop, block_course_overview, report_participation
              Hide
              dougiamas Martin Dougiamas added a comment -

              Great data, thanks Marina.

              Show
              dougiamas Martin Dougiamas added a comment - Great data, thanks Marina.
              Hide
              marina Marina Glancy added a comment -

              just saw issue MDL-40457 (currently in integration), it checks class_exists() rather than function_exists(), this is something I did not grep for in my list. (TODO)

              Show
              marina Marina Glancy added a comment - just saw issue MDL-40457 (currently in integration), it checks class_exists() rather than function_exists(), this is something I did not grep for in my list. (TODO)
              Hide
              marina Marina Glancy added a comment -

              hi, just want to comment here about recent activity that may be related to this issue.

              First of all, there was a big discussion about plugin-plugin communication where this proposal was mentioned: https://moodle.org/mod/forum/discuss.php?d=261673
              Second, we have an upcoming meeting to discuss the "correct" way of api-plugins and plugin-plugin communication (last one is still under question).

              Interesting issue that was reported recently: MDL-46207 (Scheduled tasks should consider the enabled state of a plugin). Just noting that almost all existing methods to look for callbacks do not take into account if plugin is enabled or not. Whatever way of api standard we agree on, there should be an option of filtering by plugin availability and hopefully one central core_component method to check the plugin availability.

              Also there is an issue MDL-46155 (Core method to retrieve list of classes), inspired by MDL-40457 (finally integrated this week) and existing report_eventlist. It is work in progress atm and one of the things that is missing is the check if plugin is enabled.

              Show
              marina Marina Glancy added a comment - hi, just want to comment here about recent activity that may be related to this issue. First of all, there was a big discussion about plugin-plugin communication where this proposal was mentioned: https://moodle.org/mod/forum/discuss.php?d=261673 Second, we have an upcoming meeting to discuss the "correct" way of api-plugins and plugin-plugin communication (last one is still under question). Interesting issue that was reported recently: MDL-46207 (Scheduled tasks should consider the enabled state of a plugin). Just noting that almost all existing methods to look for callbacks do not take into account if plugin is enabled or not. Whatever way of api standard we agree on, there should be an option of filtering by plugin availability and hopefully one central core_component method to check the plugin availability. Also there is an issue MDL-46155 (Core method to retrieve list of classes), inspired by MDL-40457 (finally integrated this week) and existing report_eventlist. It is work in progress atm and one of the things that is missing is the check if plugin is enabled.
              Hide
              marina Marina Glancy added a comment -

              People need hooks... they create crazy stuff for it - see MDL-51396 for example

              Show
              marina Marina Glancy added a comment - People need hooks... they create crazy stuff for it - see MDL-51396 for example
              Hide
              brendanheywood Brendan Heywood added a comment -

              Apologies if this is a bit long and a little out of context as this is a partial continuation of a discussion started in MDL-28030 and is just another couple data points for any hook API proposal. I'll add a quick summary so you don't have to trawl through the other issue.

              In MDL-28030 I've proposed adding a hook called pre_head_content using get_plugins_with_function which enables any plugin to add content to the start of the head tag. On the surface this is very similar to simply appending stuff to $CFG->additionalhtmlhead but the critical different is in that particular use case it needed to render first, and from a local plugin and fairly late in the overall execution ie as the final page view is being generated.

              David Monllaó replied:

              > I understand why we need pre_head_content, this is fine for me, the problem is that you are justifying the callback approach because "pre_head_content is a generic useful hook that I could see being used simultaneously by other plugin types completely unrelated to clean urls or url rewriting" and this is not right; as Dan mentioned in the previous review, multiple plugins hooking that process might lead to crazy and uncontrolled results, if people wants to change standard_head_html they have a setting for it, if they need PHP they can override the renderer, adding an extra new way to do it will only confuse people, it is not 100% the same result, but the result is more concise and controlled, proper hooks would require many considerations and should be discussed in detail, MDL-44078 seems the correct place to do it IMO and its solution would be consistent across components, would define a strict interface, would be maintained (no surprises in hooked functions)...

              Lets break this down:

              > multiple plugins hooking that process might lead to crazy and uncontrolled results

              True, but this is also true of every single other hook and especially true of an old school approach like appending to $CFG->additionalhtmlhead eg a plugin could just set it to something instead of appending and wipe out other plugins additions, or do a regex replace on it or something else horrible. Making hooks extend an interface would cut down on a lot of potential issues.

              > if people wants to change standard_head_html they have a setting for it

              Yes but only via either overriding a renderer which in this case isn't fine grained enough to be usefully overridden and we can't do regardless because we are not in a theme, or by appending or pre-pending data to additionalhtmlhead which again isn't fine grained enough to get the order right. We need a generic method for multiple plugins to add the bits they need which is why hooks are a better solution BUT we also need to be able to prioritise the execution order of the plugins and/or add multiple hook points so that you can use the right hook to put things in the correct order.

              I'll add a second example data point of a hook that I think is needed (and I'll open as an issue at some point). We've written a bunch of different local plugins for various clients which force students to do various things before they can use the site, like fill out a form, or complete a course, watch a video, or whatever. And some have additional requirements like they need to redo it every year. Think of them as like $USER->policyagreed on steroids. We typically hard code a call roughly where the policyagreed check is done in require_login() but this is ripe for a hook instead. If this was done then policyagreed could get refactored out and just use the same hook. But similar to above there could be multiple active plugins using the same hook, and order could be important. A realistic example might be "Login > Accept EULA > Provide AVETMISS data > Watch / dismiss intro help video > Dashboard". But different to the pre_head_content hook above, we want the order specified in config by the site admin, not by the plugin developer(s), so we'd want an admin interface where we could see all the hook receivers and override its default priority.

              Show
              brendanheywood Brendan Heywood added a comment - Apologies if this is a bit long and a little out of context as this is a partial continuation of a discussion started in MDL-28030 and is just another couple data points for any hook API proposal. I'll add a quick summary so you don't have to trawl through the other issue. In MDL-28030 I've proposed adding a hook called pre_head_content using get_plugins_with_function which enables any plugin to add content to the start of the head tag. On the surface this is very similar to simply appending stuff to $CFG->additionalhtmlhead but the critical different is in that particular use case it needed to render first, and from a local plugin and fairly late in the overall execution ie as the final page view is being generated. David Monllaó replied: > I understand why we need pre_head_content, this is fine for me, the problem is that you are justifying the callback approach because "pre_head_content is a generic useful hook that I could see being used simultaneously by other plugin types completely unrelated to clean urls or url rewriting" and this is not right; as Dan mentioned in the previous review, multiple plugins hooking that process might lead to crazy and uncontrolled results, if people wants to change standard_head_html they have a setting for it, if they need PHP they can override the renderer, adding an extra new way to do it will only confuse people, it is not 100% the same result, but the result is more concise and controlled, proper hooks would require many considerations and should be discussed in detail, MDL-44078 seems the correct place to do it IMO and its solution would be consistent across components, would define a strict interface, would be maintained (no surprises in hooked functions)... Lets break this down: > multiple plugins hooking that process might lead to crazy and uncontrolled results True, but this is also true of every single other hook and especially true of an old school approach like appending to $CFG->additionalhtmlhead eg a plugin could just set it to something instead of appending and wipe out other plugins additions, or do a regex replace on it or something else horrible. Making hooks extend an interface would cut down on a lot of potential issues. > if people wants to change standard_head_html they have a setting for it Yes but only via either overriding a renderer which in this case isn't fine grained enough to be usefully overridden and we can't do regardless because we are not in a theme, or by appending or pre-pending data to additionalhtmlhead which again isn't fine grained enough to get the order right. We need a generic method for multiple plugins to add the bits they need which is why hooks are a better solution BUT we also need to be able to prioritise the execution order of the plugins and/or add multiple hook points so that you can use the right hook to put things in the correct order. I'll add a second example data point of a hook that I think is needed (and I'll open as an issue at some point). We've written a bunch of different local plugins for various clients which force students to do various things before they can use the site, like fill out a form, or complete a course, watch a video, or whatever. And some have additional requirements like they need to redo it every year. Think of them as like $USER->policyagreed on steroids. We typically hard code a call roughly where the policyagreed check is done in require_login() but this is ripe for a hook instead. If this was done then policyagreed could get refactored out and just use the same hook. But similar to above there could be multiple active plugins using the same hook, and order could be important. A realistic example might be "Login > Accept EULA > Provide AVETMISS data > Watch / dismiss intro help video > Dashboard". But different to the pre_head_content hook above, we want the order specified in config by the site admin, not by the plugin developer(s), so we'd want an admin interface where we could see all the hook receivers and override its default priority.
              Hide
              poltawski Dan Poltawski added a comment -

              I reincarnated this topic on the GDF https://moodle.org/mod/forum/discuss.php?d=327349

              Show
              poltawski Dan Poltawski added a comment - I reincarnated this topic on the GDF https://moodle.org/mod/forum/discuss.php?d=327349
              Hide
              marina Marina Glancy added a comment -

              Hello, we have spent a long time today discussing this in HQ scrum. I'll try to summarise our discussion here in the list of points:

              • Nobody likes the current implementation with functions that must be located in lib.php
              • Some want to make a clear distinction between callbacks and hooks, however the majority voted to have them all together in one pile and to be called "hooks"
              • Again, majority of people agreed that implementations of hooks should be defined in db/hooks.php for both performance and readability
              • Implementing interfaces or abstract base classes for the hooks implementation has both its advantages and disadvantages:
                • Clear hints on what method(s) are expected from the callback
                • Self-contained documentation in phpdocs
                • abstract classes force us to have one method per class in the most cases which is not convenient
                • interfaces force us to use them only in the classes without constructors, it is not possible to define static functions in the interface
                • it is extremely inflexible for future evolution of the hooks - we can not even add arguments with default values to the end of the arguments list
              • There was a suggestion to define the hooks as an array of strings (hooks names) in the same db/hooks.php where each element would have a phpdocs with textual description of what is expected as arguments and return value for this hook
              • We decided that it could only work if we always pass the specific object to the hook and expect all hooks (or a single callback) to get information from getter methods of this object and also "return" values by calling setter methods of this object
              • The idea to have a special class for each hook that is passed as argument to the hook implementations appealed to everybody since it ticks all boxes:
                • Hook is self-documented in phpdocs of this class
                • Evolution is possible by adding more getter/setter methods in the future without breaking backward compatibility, similarly deprecation can be done nicely without fatal errors
                • We do not need return values from the hook implementations, which means it is easier to call them
                • Actual hook execution can be a method inside this class and it can also call and process "legacy" (existing) callbacks
              • After this discussion we looked again at the https://docs.moodle.org/dev/Hooks_spec and decided that we came to exactly the same model
              • We have also remembered that this specification did not go forward not because it had any flaws but because we could not promise to transfer all existing callbacks and it seemed that there was not enough discussion

              At the moment HQ agrees that there was enough discussion and we can't think of anything better. Also we don't need to aim to convert all existing callbacks immediately - we simply don't have enough resources for it. We don't want to delay this change any longer because we are working in several directions on AJAX-fying moodle UI and we desperately need a hook/callback api.

              Show
              marina Marina Glancy added a comment - Hello, we have spent a long time today discussing this in HQ scrum. I'll try to summarise our discussion here in the list of points: Nobody likes the current implementation with functions that must be located in lib.php Some want to make a clear distinction between callbacks and hooks, however the majority voted to have them all together in one pile and to be called "hooks" Again, majority of people agreed that implementations of hooks should be defined in db/hooks.php for both performance and readability Implementing interfaces or abstract base classes for the hooks implementation has both its advantages and disadvantages: Clear hints on what method(s) are expected from the callback Self-contained documentation in phpdocs abstract classes force us to have one method per class in the most cases which is not convenient interfaces force us to use them only in the classes without constructors, it is not possible to define static functions in the interface it is extremely inflexible for future evolution of the hooks - we can not even add arguments with default values to the end of the arguments list There was a suggestion to define the hooks as an array of strings (hooks names) in the same db/hooks.php where each element would have a phpdocs with textual description of what is expected as arguments and return value for this hook We decided that it could only work if we always pass the specific object to the hook and expect all hooks (or a single callback) to get information from getter methods of this object and also "return" values by calling setter methods of this object The idea to have a special class for each hook that is passed as argument to the hook implementations appealed to everybody since it ticks all boxes: Hook is self-documented in phpdocs of this class Evolution is possible by adding more getter/setter methods in the future without breaking backward compatibility, similarly deprecation can be done nicely without fatal errors We do not need return values from the hook implementations, which means it is easier to call them Actual hook execution can be a method inside this class and it can also call and process "legacy" (existing) callbacks After this discussion we looked again at the https://docs.moodle.org/dev/Hooks_spec and decided that we came to exactly the same model We have also remembered that this specification did not go forward not because it had any flaws but because we could not promise to transfer all existing callbacks and it seemed that there was not enough discussion At the moment HQ agrees that there was enough discussion and we can't think of anything better. Also we don't need to aim to convert all existing callbacks immediately - we simply don't have enough resources for it. We don't want to delay this change any longer because we are working in several directions on AJAX-fying moodle UI and we desperately need a hook/callback api.
              Hide
              fred Frédéric Massart added a comment -

              I don't know who the "majority" or "HQ" is, so I'll consider it is everybody but me, and as there is no record of some of the details of the discussion we had, let me summarise a few things that were in disagreement with some of the points above.

              Those in the office will laugh as I've said it countless times but hooks are not callbacks. Hooks are broadcasted to all plugins, and whichever want to do something does. A callback is used when it must exist, and there is only one. It is not broadcasted, it is calling what needs to be called. A callback may or may not return a value depending on the API. A hook does not return any value, it modifies something that exists. E.g. hook: pre_create_user($data), callback: $success = do_create_user($data). Or callback vs. events in Javascript.

              More importantly, if we have "hooks" which are only callbacks, that means that when a 3rd party developer will be looking at all the hooks in Moodle they will find some that are never broadcasted, as their usage is specific. How will they know? Will we add a "notahook" property to the hooks?

              Regarding the older spec, I don't think we have agreed on the final coding details, but we did agree on the hook registration (hooks.php), and the specific class instance passed to the registered callables.

              Show
              fred Frédéric Massart added a comment - I don't know who the "majority" or "HQ" is, so I'll consider it is everybody but me, and as there is no record of some of the details of the discussion we had, let me summarise a few things that were in disagreement with some of the points above. Those in the office will laugh as I've said it countless times but hooks are not callbacks. Hooks are broadcasted to all plugins, and whichever want to do something does. A callback is used when it must exist, and there is only one. It is not broadcasted, it is calling what needs to be called. A callback may or may not return a value depending on the API. A hook does not return any value, it modifies something that exists. E.g. hook: pre_create_user($data), callback: $success = do_create_user($data). Or callback vs. events in Javascript. More importantly, if we have "hooks" which are only callbacks, that means that when a 3rd party developer will be looking at all the hooks in Moodle they will find some that are never broadcasted, as their usage is specific. How will they know? Will we add a "notahook" property to the hooks? Regarding the older spec, I don't think we have agreed on the final coding details, but we did agree on the hook registration (hooks.php), and the specific class instance passed to the registered callables.
              Hide
              mudrd8mz David Mudrák added a comment -

              I am not sure I fully understand the distinction that Fred is trying to express (actually, I am sure I don't fully understand it). From certain signs (broadcasting etc) it looks like a description of thrown events.

              A callback is used when it must exist, and there is only one.

              Can you illustrate this by some examples? I am thinking of things like giving every single installed plugin to give a chance to extend the user sign up form. Is this a hook or a callback in your mind? And why is there the difference?

              The idea of having a thing like a "token" that travels around all the registered hook subscribers that can modify it is interesting ("token" as in token ring network topology). But again, how would it look in particular examples? Say the extension of the signup form. Would the form instance be the thing that travels around? Or what?

              In either case, I am wondering about the order in which the hook subscribers are ... called. If two plugins both register to modify HTML footer before it is printed, what will be the order they are given such a chance? Where would be the order defined? Will the admin have a chance to alter it? Etc.

              Show
              mudrd8mz David Mudrák added a comment - I am not sure I fully understand the distinction that Fred is trying to express (actually, I am sure I don't fully understand it). From certain signs (broadcasting etc) it looks like a description of thrown events. A callback is used when it must exist, and there is only one. Can you illustrate this by some examples? I am thinking of things like giving every single installed plugin to give a chance to extend the user sign up form. Is this a hook or a callback in your mind? And why is there the difference? The idea of having a thing like a "token" that travels around all the registered hook subscribers that can modify it is interesting ("token" as in token ring network topology). But again, how would it look in particular examples? Say the extension of the signup form. Would the form instance be the thing that travels around? Or what? In either case, I am wondering about the order in which the hook subscribers are ... called. If two plugins both register to modify HTML footer before it is printed, what will be the order they are given such a chance? Where would be the order defined? Will the admin have a chance to alter it? Etc.
              Hide
              mudrd8mz David Mudrák added a comment -

              I think that the existing Hook spec partially answers my question regarding the essence / nature of the token object. In the example there it is an object extending \core\hook\base that provides access to the form. I think that is what confused me last time as I thought that somehow represents the hook itself, not the data container between the core and the subscriber.

              Show
              mudrd8mz David Mudrák added a comment - I think that the existing Hook spec partially answers my question regarding the essence / nature of the token object. In the example there it is an object extending \core\hook\base that provides access to the form. I think that is what confused me last time as I thought that somehow represents the hook itself, not the data container between the core and the subscriber.
              Hide
              mudrd8mz David Mudrák added a comment -

              Just a quick note. It took me a minute to read the first two paragraphs of https://www.mediawiki.org/wiki/Manual:Hooks and intuitively understand the behaviour of their system. It took me almost two years to understand some essential ideas in https://docs.moodle.org/dev/Hooks_spec

              Let us KISS

              Show
              mudrd8mz David Mudrák added a comment - Just a quick note. It took me a minute to read the first two paragraphs of https://www.mediawiki.org/wiki/Manual:Hooks and intuitively understand the behaviour of their system. It took me almost two years to understand some essential ideas in https://docs.moodle.org/dev/Hooks_spec Let us KISS
              Hide
              poltawski Dan Poltawski added a comment -

              working in several directions on AJAX-fying moodle UI and we desperately need a hook/callback api.

              Sorry, but I don't buy this argument at all. Please come up with concrete examples where you can't achieve what you need with existing approaches.

              Show
              poltawski Dan Poltawski added a comment - working in several directions on AJAX-fying moodle UI and we desperately need a hook/callback api. Sorry, but I don't buy this argument at all. Please come up with concrete examples where you can't achieve what you need with existing approaches.
              Hide
              mudrd8mz David Mudrák added a comment -

              come up with concrete examples

              Yes. I would really like to see a list of actually required (not theoretical) use cases for how hooks will be used. I believe that will put more light into things.

              From me, one common example from the plugins world is recently discussed need to inject custom AMD module (or generally any JS) into $PAGE->require on every page. We seem to lack a general "before HTML header" and "before HTML footer" hookable points. Guys from the community come with better or worse hacks to achieve that, such as

              • putting the code into the body of their plugin's lib.php
              • appending their code to $CFG->additionalhtmlfooter upon the plugin installation
              • and other tricks not making us happy when reviewing such plugins - but then we generally have no good alternative to offer
              Show
              mudrd8mz David Mudrák added a comment - come up with concrete examples Yes. I would really like to see a list of actually required (not theoretical) use cases for how hooks will be used. I believe that will put more light into things. From me, one common example from the plugins world is recently discussed need to inject custom AMD module (or generally any JS) into $PAGE->require on every page. We seem to lack a general "before HTML header" and "before HTML footer" hookable points. Guys from the community come with better or worse hacks to achieve that, such as putting the code into the body of their plugin's lib.php appending their code to $CFG->additionalhtmlfooter upon the plugin installation and other tricks not making us happy when reviewing such plugins - but then we generally have no good alternative to offer
              Hide
              mudrd8mz David Mudrák added a comment -

              Other example comes from MDL-28030 where Brendan Heywood needs a clean way to add the <base> attribute to every page from his plugin.

              Show
              mudrd8mz David Mudrák added a comment - Other example comes from MDL-28030 where Brendan Heywood needs a clean way to add the <base> attribute to every page from his plugin.
              Hide
              mudrd8mz David Mudrák added a comment -

              For the record, I had a good chat with Fred regarding my questions above at https://moodle.org/local/chatlogs/index.php?conversationid=18893 (just in case you had impression there is not enough said in these comments).

              Show
              mudrd8mz David Mudrák added a comment - For the record, I had a good chat with Fred regarding my questions above at https://moodle.org/local/chatlogs/index.php?conversationid=18893 (just in case you had impression there is not enough said in these comments).
              Hide
              marina Marina Glancy added a comment -

              Re voting on hooks/callbacks distinction: Frédéric Massart - for it, Marina Glancy - undecided/for, everybody else present in the meeting - against. At least I remember that Damyon Wiese, Andrew Nicols, Mark Nelson, Cameron Ball said that they did not want to make a distinction. I'm not sure about other participants - Adrian Greeve, Simey Lameze, Jun Pataleta, John Okely but they definitely did not give any arguments for this idea.

              There will still be a distinction inside the execute() function of the individual hooks, for example, callbacks will throw an exception if componentname is not specified and/or implementation in this callback is not present. But all of them will be called "hooks", and the implementations will be listed in db/hooks.php regardless of whether it is a hook or callback.

              Regarding this conversation in devchat that I missed because I went to the beach:

              What is, I think important, is that we don't mistake hooks for callbacks. You could pass a callback (or callback name) via Ajax to be executed, you wouldn't pass a hook. The same way you wouldn't pass an event as an argument.

              Let me point out here that you can NEVER pass a callback to the Ajax. If a callback(hook) needs to be accessible from Ajax there will be a separate web service for it. We can only pass component(plugin) name and some additional arguments but never a callback. When this hook is executed it will validate the component name and call a single hook implementation from the given component.

              Show
              marina Marina Glancy added a comment - Re voting on hooks/callbacks distinction: Frédéric Massart - for it, Marina Glancy - undecided/for, everybody else present in the meeting - against. At least I remember that Damyon Wiese , Andrew Nicols , Mark Nelson , Cameron Ball said that they did not want to make a distinction. I'm not sure about other participants - Adrian Greeve , Simey Lameze , Jun Pataleta , John Okely but they definitely did not give any arguments for this idea. There will still be a distinction inside the execute() function of the individual hooks, for example, callbacks will throw an exception if componentname is not specified and/or implementation in this callback is not present. But all of them will be called "hooks", and the implementations will be listed in db/hooks.php regardless of whether it is a hook or callback. Regarding this conversation in devchat that I missed because I went to the beach: What is, I think important, is that we don't mistake hooks for callbacks. You could pass a callback (or callback name) via Ajax to be executed, you wouldn't pass a hook. The same way you wouldn't pass an event as an argument. Let me point out here that you can NEVER pass a callback to the Ajax. If a callback(hook) needs to be accessible from Ajax there will be a separate web service for it. We can only pass component(plugin) name and some additional arguments but never a callback. When this hook is executed it will validate the component name and call a single hook implementation from the given component.
              Hide
              cameron1729 Cameron Ball added a comment - - edited

              I thought what we voted on was not whether there should be a distinction between the term "hook" and the term "callback" (there most certainly is), but whether or not we should use the spec laid out by Petr. I actually share a lot of Fred's views on the matter.

              Personally, I don't have much of an opinion on how we should do hooks or callbacks in moode, because I don't know enough about moode. What I am very concerned about is abuse of terminology. Hooks and callbacks are existing, well established concepts and I think it would be a bad idea to use those terms to describe something different to what a software engineer might expect.

              From the discussion on Friday I felt like a big part of the problem we have is documentation. So whatever we do, I think that we need to be very clear from the beginning.

              Show
              cameron1729 Cameron Ball added a comment - - edited I thought what we voted on was not whether there should be a distinction between the term "hook" and the term "callback" (there most certainly is), but whether or not we should use the spec laid out by Petr. I actually share a lot of Fred's views on the matter. Personally, I don't have much of an opinion on how we should do hooks or callbacks in moode, because I don't know enough about moode. What I am very concerned about is abuse of terminology. Hooks and callbacks are existing, well established concepts and I think it would be a bad idea to use those terms to describe something different to what a software engineer might expect. From the discussion on Friday I felt like a big part of the problem we have is documentation. So whatever we do, I think that we need to be very clear from the beginning.
              Hide
              marina Marina Glancy added a comment -

              I thought what we voted on was not whether there should be a distinction between the term "hook" and the term "callback" (there most certainly is), but whether or not we should use the spec laid out by Petr. I actually share a lot of Fred's views on the matter.

              We had two votes, one on the distinction hooks/callbacks (in the beginning) and one on the approach (in the end). However in my comment above I have only mentioned the first vote so I don't understand what do you disagree with. We did not examine or discuss Petr's spec in details, all I said above is that we came to the same model. It may take another two years to agree on details and by that time we will have twice more pluginname_callbackname() functions in various lib.php files.

              Show
              marina Marina Glancy added a comment - I thought what we voted on was not whether there should be a distinction between the term "hook" and the term "callback" (there most certainly is), but whether or not we should use the spec laid out by Petr. I actually share a lot of Fred's views on the matter. We had two votes, one on the distinction hooks/callbacks (in the beginning) and one on the approach (in the end). However in my comment above I have only mentioned the first vote so I don't understand what do you disagree with. We did not examine or discuss Petr's spec in details, all I said above is that we came to the same model . It may take another two years to agree on details and by that time we will have twice more pluginname_callbackname() functions in various lib.php files.
              Hide
              cameron1729 Cameron Ball added a comment -

              Ah OK, I remember the first one now. Sorry, I think I got confused by the wording again. From memory I think the first vote was more to do with if we should have two different systems (for which we are loosely using the terms "hooks" and "callbacks"). In that case I agree that there should be one consistent system for extending functionality.

              Show
              cameron1729 Cameron Ball added a comment - Ah OK, I remember the first one now. Sorry, I think I got confused by the wording again. From memory I think the first vote was more to do with if we should have two different systems (for which we are loosely using the terms "hooks" and "callbacks"). In that case I agree that there should be one consistent system for extending functionality.
              Hide
              marina Marina Glancy added a comment -

              We had another discussion today again and had another vote. Now majority has decided to have separate systems for hooks and callbacks. We have not agreed on anything regarding callbacks implementation. Everything I said above still applies for hooks

              Show
              marina Marina Glancy added a comment - We had another discussion today again and had another vote. Now majority has decided to have separate systems for hooks and callbacks. We have not agreed on anything regarding callbacks implementation. Everything I said above still applies for hooks
              Hide
              stronk7 Eloy Lafuente (stronk7) added a comment -

              Hi, just a "little" comment, that I've shared with HQ dev leaders because I've seen it repeated a number of times (here, in the dev chat, apparently also happening in SCRUMs...) is that I don't "buy" (understand) why all this discussion about separate systems (hooks vs callbacks) is happening.

              Neither I understand why we are trying to classify "things" being callbacks OR hooks.

              In my brain, they are not alternatives, they are the 2 (complementary and required) sides of the same thing: A hooking system.

              And any hooking system has 2 parts:

              • The hooks: Or points where "interception" is allowed. Call them managers, dispatchers, publishers, mediators.. whatever you want (not adhering to any pattern here, just the idea). Surely "callers" is a way to see them.
              • The callbacks: Or which code is "executed" at hook points. Call them listeners, observers, subscribers... whatever you want. Surely "executors" is a way to see them.

              And any executed code from a hook is, by definition, a callback. It doesn't matter if single or multiple executions are allowed. It doesn't matter if they can change status, neither if they do return information or no. At least that's the way I learned about hooking systems in general (again, without going to the implementation details at all) and how those 2 required parts of the system are in my brain. Hence my surprise about all the discussion trying to differentiate hooks and callbacks. No comparison, differentiation is possible. They are the driver and the routes, the socket and the cables. 100% complimentary, not comparable/differentiable.

              Said that, I'd suggest to unify the terminology at step #1. Maybe I'm the only geek in Moodleland having that hook/callback opinion (after all, I'm an old-programmer) and maybe nowadays they are really comparable terms and the discussion above is legit. I just ask for a clarification about that. Only then we'll be able to discuss any implementation.

              My 2 cents, surely sounding like a loose bull in a china shop at this stage, I'm afraid, but I've not been able to follow all the ongoing stuff lately.

              Ciao

              Show
              stronk7 Eloy Lafuente (stronk7) added a comment - Hi, just a "little" comment, that I've shared with HQ dev leaders because I've seen it repeated a number of times (here, in the dev chat, apparently also happening in SCRUMs...) is that I don't "buy" (understand) why all this discussion about separate systems (hooks vs callbacks) is happening. Neither I understand why we are trying to classify "things" being callbacks OR hooks. In my brain, they are not alternatives, they are the 2 (complementary and required) sides of the same thing: A hooking system. And any hooking system has 2 parts: The hooks: Or points where "interception" is allowed. Call them managers, dispatchers, publishers, mediators.. whatever you want (not adhering to any pattern here, just the idea). Surely "callers" is a way to see them. The callbacks: Or which code is "executed" at hook points. Call them listeners, observers, subscribers... whatever you want. Surely "executors" is a way to see them. And any executed code from a hook is, by definition, a callback. It doesn't matter if single or multiple executions are allowed. It doesn't matter if they can change status, neither if they do return information or no. At least that's the way I learned about hooking systems in general (again, without going to the implementation details at all) and how those 2 required parts of the system are in my brain. Hence my surprise about all the discussion trying to differentiate hooks and callbacks. No comparison, differentiation is possible. They are the driver and the routes, the socket and the cables. 100% complimentary, not comparable/differentiable. Said that, I'd suggest to unify the terminology at step #1. Maybe I'm the only geek in Moodleland having that hook/callback opinion (after all, I'm an old-programmer) and maybe nowadays they are really comparable terms and the discussion above is legit. I just ask for a clarification about that. Only then we'll be able to discuss any implementation. My 2 cents, surely sounding like a loose bull in a china shop at this stage, I'm afraid, but I've not been able to follow all the ongoing stuff lately. Ciao
              Hide
              marina Marina Glancy added a comment -

              I hope my comments here are not confusing because sometimes I summarise the scrum discussions, sometimes may mention the "dev leaders" discussions and sometimes share my own opinion. I try to prefix my comments with the source.

              So, here is my personal one:

              As I said above I was undecided/for separation of one-to-one and one-to-many callbacks (just alternative naming now taking in consideration Eloy's comment above). Now I tend to NOT MAKING distinction. For example, I've been looking yesterday as two issues that are currently in integration and could have really benefited of the new hooks api:

              1. Global search (MDL-31989) is one of the most typical hook, which is also being implemented with its own custom api atm. The way it works:

              • search for all plugins/components implementing global search
              • filter them by the ones that are accessible to the current user (I did not look in details why it's needed but this is how it is now)
              • call each of the filtered list of callbacks

              So while being a typical hook when everything can hook into core process, it still calls each callback separately. This would not work well if we had clear separation of calling-all vs. calling-only-one.

              2. Inplace editable (MDL-51802) is a typical one-to-one callback - we always request a callback from one specific component/plugin. But it could be really useful if the same plugin could define several listeners to the same hook and expect all of them to be called.

              For example: core_cohort component implements inplace-editable for cohort names and cohort idnumbers. Here is the implementation with the current callback system - there is a function in cohort/lib.php that looks at the itemtype and calls one update function or another.
              Here is another commit on top of the previous one that creates two callback methods for the same hook and each of them checks if itemtype is what it can process. This would allow us to remove the dispatcher function like what core_cohort_inplace_editable was doing before.

              Some may argue that such approach is needed in this particular case but this is a real use case.

              So, as a summary I (myself, personally) want to join the camp of no distinction between one-to-one or one-to-many hooks/callbacks

              Show
              marina Marina Glancy added a comment - I hope my comments here are not confusing because sometimes I summarise the scrum discussions, sometimes may mention the "dev leaders" discussions and sometimes share my own opinion. I try to prefix my comments with the source. So, here is my personal one: As I said above I was undecided/for separation of one-to-one and one-to-many callbacks (just alternative naming now taking in consideration Eloy's comment above). Now I tend to NOT MAKING distinction. For example, I've been looking yesterday as two issues that are currently in integration and could have really benefited of the new hooks api: 1. Global search ( MDL-31989 ) is one of the most typical hook, which is also being implemented with its own custom api atm. The way it works: search for all plugins/components implementing global search filter them by the ones that are accessible to the current user (I did not look in details why it's needed but this is how it is now) call each of the filtered list of callbacks So while being a typical hook when everything can hook into core process, it still calls each callback separately. This would not work well if we had clear separation of calling-all vs. calling-only-one. 2. Inplace editable ( MDL-51802 ) is a typical one-to-one callback - we always request a callback from one specific component/plugin. But it could be really useful if the same plugin could define several listeners to the same hook and expect all of them to be called. For example: core_cohort component implements inplace-editable for cohort names and cohort idnumbers. Here is the implementation with the current callback system - there is a function in cohort/lib.php that looks at the itemtype and calls one update function or another. Here is another commit on top of the previous one that creates two callback methods for the same hook and each of them checks if itemtype is what it can process. This would allow us to remove the dispatcher function like what core_cohort_inplace_editable was doing before. Some may argue that such approach is needed in this particular case but this is a real use case. So, as a summary I (myself, personally) want to join the camp of no distinction between one-to-one or one-to-many hooks/callbacks
              Hide
              damyon Damyon Wiese added a comment -

              Just noting that I am in the same "no-distinction" camp and have voted that way from the beginning - but the scrum vote was a roughly 8-2 against at the time.

              Show
              damyon Damyon Wiese added a comment - Just noting that I am in the same "no-distinction" camp and have voted that way from the beginning - but the scrum vote was a roughly 8-2 against at the time.
              Hide
              fred Frédéric Massart added a comment -

              In computer programming, a callback is a piece of executable code that is passed as an argument to other code, which is expected to call back (execute) the argument at some convenient time. The invocation may be immediate as in a synchronous callback, or it might happen at later time as in an asynchronous callback. In all cases, the intention is to specify a function or subroutine as an entity that is, depending on the language, more or less similar to a variable. In simpler terms, a callback function is a function that is passed as an argument to another function and is invoked after some kind of event.

              https://en.wikipedia.org/wiki/Callback_%28computer_programming%29

              Maybe it'd be clearer for everyone is if we used the term callback properly... we do not have callbacks, we do have APIs which all define where the callables should be, often in lib.php with a conventional name of componentname_mypublicapifunction. When you are adding a new instance of a module, we really are far from the callback, the plugin never asked to be called in the first place, why would it ask to be called back? mod_add_instance is a definition of an API, only it's old and looks procedural, if we had that modclass::add_instance() I doubt we'd call it a callback.

              In the case of the comment API which expects many functions to be created in lib.php we could talk of callback because the plugin itself prints the comment form, and requests to be sent back the comment entered by the user facing the UI. Though, it's pretty clear that the comment API, is an API, it's not a set of callbacks.

              I agree with Eloy that there is a caller, and a callee, and typically the caller will never be a callback, though we very often use the wrong terms in our live/tracker discussions to refer to a caller. That said, we can't just decide to have a standardised API for both hooks and callbacks because we have a caller and a callee...

              The questions we need to answer is:

              • How do we implement hooks in core?
              • How do we implement a core API delegating to plugins?

              That is, what are the rules for doing this:

              // Hooks.
              function update_user($user) {
                // Calls all registered 'callables'.
                // Does not return anything.
                pre_update_user_hook($user);  
                ...
              }
               
              // Delegate, API, "callback".
              function core_validate_comment_text($commentobj);
                // Finds the 'callable' of the originating plugin, and delegate validation.
                // Returns value depending on how API was designed
                if (!core_validate_comment_at_origin($commentobj->origin, $comment)) {  
                  die();
                }
              }
              

              Yes, both use 'callables'.
              No, both have a different purpose.

              Yes, a hook dispatches to 'registered' callables, function, callbacks, code, whatever...
              No, a callback does not dispatch anything, it is using an API, an interface, a public thing, the one and only "callee" must exist.

              A good example of callback in PHP is array_map().

              Cheers,
              Fred

              PS: Yes, that is contradicting some of the things I posted on the forum. It became clearer to me what callbacks are through those discussions, and how we do not have a callbacks but APIs.

              Show
              fred Frédéric Massart added a comment - In computer programming, a callback is a piece of executable code that is passed as an argument to other code, which is expected to call back (execute) the argument at some convenient time. The invocation may be immediate as in a synchronous callback, or it might happen at later time as in an asynchronous callback. In all cases, the intention is to specify a function or subroutine as an entity that is, depending on the language, more or less similar to a variable. In simpler terms, a callback function is a function that is passed as an argument to another function and is invoked after some kind of event. https://en.wikipedia.org/wiki/Callback_%28computer_programming%29 Maybe it'd be clearer for everyone is if we used the term callback properly... we do not have callbacks , we do have APIs which all define where the callables should be, often in lib.php with a conventional name of componentname_mypublicapifunction . When you are adding a new instance of a module, we really are far from the callback , the plugin never asked to be called in the first place, why would it ask to be called back? mod_add_instance is a definition of an API, only it's old and looks procedural, if we had that modclass::add_instance() I doubt we'd call it a callback. In the case of the comment API which expects many functions to be created in lib.php we could talk of callback because the plugin itself prints the comment form, and requests to be sent back the comment entered by the user facing the UI. Though, it's pretty clear that the comment API, is an API, it's not a set of callbacks. I agree with Eloy that there is a caller , and a callee , and typically the caller will never be a callback , though we very often use the wrong terms in our live/tracker discussions to refer to a caller . That said, we can't just decide to have a standardised API for both hooks and callbacks because we have a caller and a callee... The questions we need to answer is: How do we implement hooks in core? How do we implement a core API delegating to plugins? That is, what are the rules for doing this: // Hooks. function update_user($user) { // Calls all registered 'callables'. // Does not return anything. pre_update_user_hook($user); ... }   // Delegate, API, "callback". function core_validate_comment_text($commentobj); // Finds the 'callable' of the originating plugin, and delegate validation. // Returns value depending on how API was designed if (!core_validate_comment_at_origin($commentobj->origin, $comment)) { die(); } } Yes, both use 'callables'. No, both have a different purpose. Yes, a hook dispatches to 'registered' callables, function, callbacks, code, whatever... No, a callback does not dispatch anything, it is using an API, an interface, a public thing, the one and only "callee" must exist. A good example of callback in PHP is array_map(). Cheers, Fred PS: Yes, that is contradicting some of the things I posted on the forum. It became clearer to me what callbacks are through those discussions, and how we do not have a callbacks but APIs.
              Hide
              dougiamas Martin Dougiamas added a comment -

              Terms are important in this discussion. I grappled with all this last night and it took a detailed explanation from Marina and then a discussion from Eloy to really nail it in my mind. Sure, I'm an old fogey now but I am guessing that others are also having some difficulty, which means votes may not be valid.

              I suspect the terminology in systems like Drupal has coloured things, since they refer to the callables in plugins as "hooks" which is clearly unusual.

              I am on board Eloy's characterisations of things as callers/hooks and callables/executors.

              Callers decide what they want to call, either:

              1. I would like to send this object to a particular plugin to get something back or for it to just execute something.
              2. I would like to make this object available to ALL plugins that are interested in it.

              The Callers/Hooks manager probably works similar to Events, in that it parses the whole system for callables and caches them. As such I think we just need one file in every plugin like db/callbacks.php (db/callables.php or db/executors.php or db/pluginapi.php) to define them all.

              Existing callers can be modified to look for these new ones first and fall back to old functions if not found - allowing us to migrate over time.

              Show
              dougiamas Martin Dougiamas added a comment - Terms are important in this discussion. I grappled with all this last night and it took a detailed explanation from Marina and then a discussion from Eloy to really nail it in my mind. Sure, I'm an old fogey now but I am guessing that others are also having some difficulty, which means votes may not be valid. I suspect the terminology in systems like Drupal has coloured things, since they refer to the callables in plugins as "hooks" which is clearly unusual. I am on board Eloy's characterisations of things as callers/hooks and callables/executors. Callers decide what they want to call, either: I would like to send this object to a particular plugin to get something back or for it to just execute something. I would like to make this object available to ALL plugins that are interested in it. The Callers/Hooks manager probably works similar to Events, in that it parses the whole system for callables and caches them. As such I think we just need one file in every plugin like db/callbacks.php (db/callables.php or db/executors.php or db/pluginapi.php) to define them all. Existing callers can be modified to look for these new ones first and fall back to old functions if not found - allowing us to migrate over time.
              Hide
              cameron1729 Cameron Ball added a comment - - edited

              In general (i.e., not in the context of moodle):

              Hooking

              • A technique to modify the behaviour of software by interception.
              • The thing that handles the interception is called the hook. So usually you would put these in predefined spots in your application, but not necessarily (see next point).
              • Can be achieved using callbacks, but it isn't necessary. For example you could override function definitions (bit tricky in PHP though).

              Callback

              • Something passed as an argument to another function to be run at some later point (could be a function pointer, could be an anonymous function, whatever). In PHP we can use the fact that we have first-class functions.

              I would argue that a hooking system where you give functions a specific name (maybe you prefix them with a plugin/component name or something), in order to have them called at some point in the application's execution is not using callbacks. The functions themselves are not callbacks, they were never passed anywhere. They're just functions. I'm talking generally. Not about moodle, not even about PHP.

              Fred I'm not sure I would call array_map a callback, but certainly it is used with callbacks. array_map does "call back" the function provided to it as an argument, but it is the provided function that we refer to as the callback. Edit: Now I understand you were saying array_map is an example of using a callback.

              It would be sad to me if we abused these terms to mean something moodle specific.

              Show
              cameron1729 Cameron Ball added a comment - - edited In general (i.e., not in the context of moodle): Hooking A technique to modify the behaviour of software by interception. The thing that handles the interception is called the hook. So usually you would put these in predefined spots in your application, but not necessarily (see next point). Can be achieved using callbacks, but it isn't necessary. For example you could override function definitions (bit tricky in PHP though). Callback Something passed as an argument to another function to be run at some later point (could be a function pointer, could be an anonymous function, whatever ). In PHP we can use the fact that we have first-class functions. I would argue that a hooking system where you give functions a specific name (maybe you prefix them with a plugin/component name or something), in order to have them called at some point in the application's execution is not using callbacks. The functions themselves are not callbacks, they were never passed anywhere. They're just functions. I'm talking generally. Not about moodle, not even about PHP. Fred I'm not sure I would call array_map a callback, but certainly it is used with callbacks. array_map does "call back" the function provided to it as an argument, but it is the provided function that we refer to as the callback. Edit: Now I understand you were saying array_map is an example of using a callback. It would be sad to me if we abused these terms to mean something moodle specific.
              Hide
              marina Marina Glancy added a comment -

              Sending for peer review, the branch includes hooks needed for MDL-48012

              Show
              marina Marina Glancy added a comment - Sending for peer review, the branch includes hooks needed for MDL-48012
              Show
              cibot CiBoT added a comment - Fails against automated checks. Checked MDL-44078 using repository: https://github.com/marinaglancy/moodle master (34 errors / 2 warnings) [branch: wip-MDL-44078-master | CI Job ] phplint (0/0) , phpcs (1/1) , js (0/0) , css (0/0) , phpdoc (33/0) , commit (0/1) , savepoint (0/0) , thirdparty (0/0) , grunt (0/0) , shifter (0/0) , travis (0/0) , More information about this report
              Hide
              damyon Damyon Wiese added a comment - - edited

              Thanks Marina for pushing this forward.

              Some questions about the code.

              QA) Why are we not using constructors for the hooks? The end result seems identical to this invented static create() thing.

              QB) Why is there an "executing" variable in the hook base class? - it is never set anywhere.

              QC) Is there a recommended way to convert an existing component_callback into a hook and maintain backwards compatibility ? I guess you would convert the existing component_callback into a hook, then register a callback for the hook that calls the legacy API. I think we should have an Epic at least for listing existing callbacks to be converted.

              List of tiny problems:

              1. typo in phpdocs for * @param string $componenetname (execute)
              2. phpdocs for function add_callbacks missing $componentname
              3. 2 blank lines in lib/tests/fixtures/hook_fixtures.php
              4. phpdocs for function add_component_callbacks are wrong
              5. travis found a missing string

              Show
              damyon Damyon Wiese added a comment - - edited Thanks Marina for pushing this forward. Some questions about the code. QA) Why are we not using constructors for the hooks? The end result seems identical to this invented static create() thing. QB) Why is there an "executing" variable in the hook base class? - it is never set anywhere. QC) Is there a recommended way to convert an existing component_callback into a hook and maintain backwards compatibility ? I guess you would convert the existing component_callback into a hook, then register a callback for the hook that calls the legacy API. I think we should have an Epic at least for listing existing callbacks to be converted. List of tiny problems: 1. typo in phpdocs for * @param string $componenetname (execute) 2. phpdocs for function add_callbacks missing $componentname 3. 2 blank lines in lib/tests/fixtures/hook_fixtures.php 4. phpdocs for function add_component_callbacks are wrong 5. travis found a missing string
              Hide
              skodak Petr Skoda added a comment - - edited

              Hi guys, just for your information here 0001-TL-8208-add-general-hooks.patch is my current patch for hooks in Totara that we were planning to include in our next release prior to this issue being revived. Feel free to reuse the code or ideas. I am heading off for a spearfishing trip now, if you have any questions I can answer them later next week. Ciao

              Show
              skodak Petr Skoda added a comment - - edited Hi guys, just for your information here 0001-TL-8208-add-general-hooks.patch is my current patch for hooks in Totara that we were planning to include in our next release prior to this issue being revived. Feel free to reuse the code or ideas. I am heading off for a spearfishing trip now, if you have any questions I can answer them later next week. Ciao
              Hide
              marina Marina Glancy added a comment -

              Hi Damyon,
              QA) - there is no restriction on how individual hooks are created. I personally find static create() better because it is chainable (I can call ::create()->execute() )
              QB) - it is supposed to prevent recursive executing - I missed that it is not set (note to myself: always write unittests!)
              QC) - yes but.... I could not find a single "proper" callback in moodle that could be converted without re-thinking it's API. Half of existing callbacks should be converted to the activity plugin base class methods. Lots of others return html instead of templateable/renderable objects (such as recent activity). Course reset consists of three callbacks and it is also not obvious how to convert them. I'll try to find something and sketch an example of conversion.

              I'll look at your other comments after I finish testing. Also I wanted to write more phpdocs to the new hooks and cover the rest of them with unittests

              Petr Skoda (hello!) I have already took your commit as the base as you can see in my branch. However I changed it a little because we also need to be able to execute callback for one component only and also allow core components to define callbacks.

              Show
              marina Marina Glancy added a comment - Hi Damyon, QA) - there is no restriction on how individual hooks are created. I personally find static create() better because it is chainable (I can call ::create()->execute() ) QB) - it is supposed to prevent recursive executing - I missed that it is not set (note to myself: always write unittests!) QC) - yes but.... I could not find a single "proper" callback in moodle that could be converted without re-thinking it's API. Half of existing callbacks should be converted to the activity plugin base class methods. Lots of others return html instead of templateable/renderable objects (such as recent activity). Course reset consists of three callbacks and it is also not obvious how to convert them. I'll try to find something and sketch an example of conversion. I'll look at your other comments after I finish testing. Also I wanted to write more phpdocs to the new hooks and cover the rest of them with unittests Petr Skoda (hello!) I have already took your commit as the base as you can see in my branch. However I changed it a little because we also need to be able to execute callback for one component only and also allow core components to define callbacks.
              Show
              cibot CiBoT added a comment - Fails against automated checks. Checked MDL-44078 using repository: https://github.com/marinaglancy/moodle master (9 errors / 2 warnings) [branch: wip-MDL-44078-master | CI Job ] phplint (0/0) , phpcs (1/1) , js (0/0) , css (0/0) , phpdoc (8/0) , commit (0/1) , savepoint (0/0) , thirdparty (0/0) , grunt (0/0) , shifter (0/0) , travis (0/0) , More information about this report
              Show
              cibot CiBoT added a comment - Fails against automated checks. Checked MDL-44078 using repository: https://github.com/marinaglancy/moodle master (3 errors / 0 warnings) [branch: wip-MDL-44078-master | CI Job ] phplint (0/0) , phpcs (1/0) , js (0/0) , css (0/0) , phpdoc (0/0) , commit (2/0) , savepoint (0/0) , thirdparty (0/0) , grunt (0/0) , shifter (0/0) , travis (0/0) , More information about this report
              Hide
              poltawski Dan Poltawski added a comment -

              Hi,

              I'm repeating myself, but it seeems add a lot of discussion work has gone into this and continue to leave the original 'objections' unanswered.

              Let me be clear, a lot of people want to see Moodle add more 'hooking points', that is indepdent of this 'hook system', so please keep the two seperate.

              This would be a different discussion if we did not have existing approaches, we do have existing approches and from what has been seen so far, we are not being blocked by the lack of the system. In fact, you use some issues as examples as the 'need' for this issue, which are currently being implemented with the callbacks we already have. That undermines the use case and it makes me question why we continue to burn time on this.

              The concern is yet another system and yet another Moodle inconsistency and new approach for developers.

              Please clearly express "We need this system for [BLANK] reason because the existing approaches XYZ do not allow us to achieve this for reason [BLANK]". That continues to be unanswered to some of us. If you can answer it then the case for this landing will be freer.

              Additionally:

              • When we start adding more hooking points, will they be as strong in API terms as module callbacks? If we change some internals and the hook no longer makes sense? Or if we change the data manupulated by the hook do we provide backwards compatibility? What is compatbility story going forward.
              • Do admins get any control, or do we allow any plugin to manipulate anything across the system? Should they? Same for ordering of execution.
              Show
              poltawski Dan Poltawski added a comment - Hi, I'm repeating myself, but it seeems add a lot of discussion work has gone into this and continue to leave the original 'objections' unanswered. Let me be clear, a lot of people want to see Moodle add more 'hooking points', that is indepdent of this 'hook system', so please keep the two seperate. This would be a different discussion if we did not have existing approaches, we do have existing approches and from what has been seen so far, we are not being blocked by the lack of the system. In fact, you use some issues as examples as the 'need' for this issue, which are currently being implemented with the callbacks we already have. That undermines the use case and it makes me question why we continue to burn time on this. The concern is yet another system and yet another Moodle inconsistency and new approach for developers. Please clearly express "We need this system for [BLANK] reason because the existing approaches XYZ do not allow us to achieve this for reason [BLANK]". That continues to be unanswered to some of us. If you can answer it then the case for this landing will be freer. Additionally: When we start adding more hooking points, will they be as strong in API terms as module callbacks? If we change some internals and the hook no longer makes sense? Or if we change the data manupulated by the hook do we provide backwards compatibility? What is compatbility story going forward. Do admins get any control, or do we allow any plugin to manipulate anything across the system? Should they? Same for ordering of execution.
              Hide
              marina Marina Glancy added a comment -

              Hi Dan,
              the problems with the current callbacks approach are listed in this issue description and on the page http://docs.moodle.org/dev/Hooks_spec
              We HAVE been blocked with the lack of system too, some of existing issues are linked to this one.

              Not all existing callbacks should/could be converted to hooks, many of them should become part of activity module OOP design. Similar way all course formats callbacks were once deprecated.

              Callbacks can set priority and they will be ordered based on it. There are no current plans to implement the admin control, HOWEVER each hook implementation can have it's own settings page.

              I feel sad that we are going into another endless round of disucssions

              Show
              marina Marina Glancy added a comment - Hi Dan, the problems with the current callbacks approach are listed in this issue description and on the page http://docs.moodle.org/dev/Hooks_spec We HAVE been blocked with the lack of system too, some of existing issues are linked to this one. Not all existing callbacks should/could be converted to hooks, many of them should become part of activity module OOP design. Similar way all course formats callbacks were once deprecated. Callbacks can set priority and they will be ordered based on it. There are no current plans to implement the admin control, HOWEVER each hook implementation can have it's own settings page. I feel sad that we are going into another endless round of disucssions
              Hide
              damyon Damyon Wiese added a comment -

              Just documenting the discussion I just had with Marina. One of the original goals of this new system was to bring order to the chaos of the many, many, many callbacks that exist. It took Marina a long time to find all the existing callbacks listed on this issue, and more have been added since, and there may be more that were not found. So - we should ensure that with this new API - we can look at a single file and see all of the hooks that a component is defining. Since we already have db/hooks.php in this patch - this seems like a perfect place to list both the registered handlers AND the definitions. The definitions do not need to be cached etc - but when developer mode is enabled and we execute a hook - we should check that each callback has been registered properly in the hooks.php file and generate some debugging messages if they have not.

              Show
              damyon Damyon Wiese added a comment - Just documenting the discussion I just had with Marina. One of the original goals of this new system was to bring order to the chaos of the many, many, many callbacks that exist. It took Marina a long time to find all the existing callbacks listed on this issue, and more have been added since, and there may be more that were not found. So - we should ensure that with this new API - we can look at a single file and see all of the hooks that a component is defining. Since we already have db/hooks.php in this patch - this seems like a perfect place to list both the registered handlers AND the definitions. The definitions do not need to be cached etc - but when developer mode is enabled and we execute a hook - we should check that each callback has been registered properly in the hooks.php file and generate some debugging messages if they have not.
              Hide
              marina Marina Glancy added a comment -

              I have added commit that adds debugging message if hook name is not listed in $hooks variable in the lib/db/hooks.php

              Show
              marina Marina Glancy added a comment - I have added commit that adds debugging message if hook name is not listed in $hooks variable in the lib/db/hooks.php
              Hide
              cibot CiBoT added a comment -

              Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!

              Show
              cibot CiBoT added a comment - Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!
              Show
              cibot CiBoT added a comment - Fails against automated checks. Checked MDL-44078 using repository: https://github.com/marinaglancy/moodle master (3 errors / 0 warnings) [branch: wip-MDL-44078-master | CI Job ] phplint (0/0) , phpcs (1/0) , js (0/0) , css (0/0) , phpdoc (0/0) , commit (2/0) , savepoint (0/0) , thirdparty (0/0) , grunt (0/0) , shifter (0/0) , travis (0/0) , More information about this report
              Hide
              damyon Damyon Wiese added a comment -

              Thanks for fixing the points I raised above Marina.

              Re: QA - Since php 5.4 (all supported versions) you can do (new Thing())->chained(). So - I still see 0 benefits of the static create() to a real constructor.

              Show
              damyon Damyon Wiese added a comment - Thanks for fixing the points I raised above Marina. Re: QA - Since php 5.4 (all supported versions) you can do (new Thing())->chained(). So - I still see 0 benefits of the static create() to a real constructor.
              Hide
              mudrd8mz David Mudrák added a comment -

              Well, even if none of the common reasons for having a static factory method applies here (no logic for selecting a subclass, no delegation etc), I am personally finding

              \core\hook\something::create($params)->execute();
              

              more readable than the alternative

              (new \core\hook\something($params))->execute();
              

              In other words, statement lines starting with opening bracket look unnatural to my eyes.

              Show
              mudrd8mz David Mudrák added a comment - Well, even if none of the common reasons for having a static factory method applies here (no logic for selecting a subclass, no delegation etc), I am personally finding \core\hook\something::create($params)->execute(); more readable than the alternative (new \core\hook\something($params))->execute(); In other words, statement lines starting with opening bracket look unnatural to my eyes.
              Hide
              dobedobedoh Andrew Nicols added a comment -

              All,

              I've been reviewing this issue for integration, however I currently have some concern about its direction.
              It appears that some of the concerns raised throughout this issue have been ignored, and that other issues are being used to force this issue in when it does not seem to be ready.
              These feelings have been echoed by others in the discussion above.

              Firstly the issue here seems a little confused. There's much talk of different terminology - hooks, callbacks, etc. but little discussion on what these should actually do within a Moodle context.
              The solution provided currently seems to be a version of hooks which takes a lot from our renderables and event 1 APIs, but this results in an extremely top-heavy and clunky implementation.
              There doesn't seem to be any true specifcation - sure we have a wiki page called "Hooks spec", but it's a collection of goals, notes on existing code, and a code example. This does not a specification make.

              I'm also concerned about the sheer amount of things that this implemention of hooks will 'solve':

              1. MDL-51867
              2. MDL-24359
              3. MDL-43742
              4. MDL-43808
              5. MDL-48012
              6. MDL-46611
              7. MDL-39709
              8. Many others I'm sure
              9. Numerous references in developer chat, meetings, etc.

              Of these, I feel that many have very different requirements, and some of them feel like an abuse of a Hooking system.
              As one example, the usage demonstrated in MDL-51867 is what Hooks should specifically not be – as Frédéric Massart so eloquently puts it, "it should not be an API hidden in the hook system".
              I see exactly the same in the example provided in the branch - inplace_editable. This is currently implemented as a component_callback() and it is being switched to a hook targetted at one specific usage. Furthermore the execute() function for inplace_editable() performs enough magic that we have essentially created a halt mechanism in the execution code. What is the benefit of this being called as a Hook over the current functionality? Not including the lib.php? That isn't enough IMO.

              Another concern I have here is the provided ability to target specific components or plugins. From my understanding of this issue to date, and from my presence in numerous lenghty meetings to discuss this functionality, I thought we were intending to separate out hooks, and what we have been (incorrectly) calling callbacks, but we seem to be doing them both here in one confused implementation. Who is to say that a local plugin doesn't want to listen to the hooks fired against a block for example? I think that we have to be very careful about filtering the hooks in this fashion as it is detrimental to the concept of the hooks as a whole.

              However, my main concern with the current design of this issue is that it is entirely top-heavy, which I suspect means that it will be sparsely implemented.
              The example hook classes that you give are great examples of this - 140 lines of comment + code for inplace_editable.
              Even just writing the boilerplate here is time consuming, repetitive, and wasteful.
              The benefits claimed here are the creation of consistent and documented API, but at the expense of an extremely
              time-consuming process to add hook calls. I think that there are other ways of documenting hooks and achieving these
              goals.

              Having a brief look at how other frameworks handle the definition and invocation of hooks enforces my feelings here.
              In the case of Wordpress, a hook is invoked:

              do_action( 'wp_login', $user->user_login, $user );
              

              This will call any implementation of the hook, and pass it the username, and the user object.

              Some of these hooks have inline documentation, but most don't have anything. Discovery is via a couple of places, for example:

              1. https://codex.wordpress.org/Plugin_API/Action_Reference/wp_login
              2. http://adambrown.info/p/wp_hooks
              3. grep "(do_action|apply_filters)"

              And in the case of Drupal:

              \Drupal::moduleHandler()->alter('file_url', $uri);
              

              This will call the function hook_file_url_alter and pass it the $uri which can be defined as pass-by-reference in the function signature.
              There is no documentation for this hook at the point at which it is invoked, but each hook has an example implementation defined.
              This example defition is then used to generate the documentation:

              1. https://github.com/drupal/drupal/blob/c6c691343f5a6bd00bf32cbd3199017e65a4a5be/core/lib/Drupal/Core/File/file.api.php#L42
              2. https://api.drupal.org/api/drupal/modules%21system%21system.api.php/function/hook_file_url_alter/7

              I also feel that there should be few (if any) subscribers to the hooks in core, with some being allowed in core plugins.
              We should not be using hooks as first-class API.
              This seems to be applied in drupal too (https://api.drupal.org/api/drupal/includes!module.inc/group/hooks/7):

              The example functions included are not part of the Drupal core, they are just models that you can modify. Only the hooks implemented within modules are executed when running Drupal.

              Moving forward

              For this issue to move forward I believe we need to flesh out the specification, and to think about what we are actually trying to achieve.

              I have been discussing this issue with Frédéric Massart amongst other and we have independently come to very similar alternative conclusions. Putting our heads together we both feel that hooks should be defined and invoked at the same time, and much more simply:

              // Invocation and definition.
              \core\hook::fire('post_course_edit', $course);
              

              We both felt that only accepting a single argument to the fire function would be clearest and that only accepting a stdClass would also allow for clearer use at implementation.

              Where a hook wishes to pass multiple objects:

              // Invocation and definition.
              $someothervalue = 1;
              \core\hook::fire('post_course_edit', (object) ['course' => $course, 'someothervalue' => &$someothervalue]);
              

              On the implementation side:

              // Example implementation.
              namespace \local\foo;
               
              class hooks {
                public static function post_course_edit(\core\hook\facade $hook) {
                  // Update the summary of the course.
                  if (strpos($hook->data->summary, '@copy;') === false) {
                    $hook->data->summary .= '<p>@copy; 2016 Cheese Makers Incorporated.</p>';
                  }
                }
              }
              

              We also felt that having the fire() function create a new \core\hook\facade and pass that into the function would also allow for future expansion - e.g. functions such as halt().

              When it comes to discoverability I feel we need to look around at what others are doing, and what does and does not work for them.

              I do not feel that we need to create entire classes and infrastuctures to provide discoverability and documentation, and I think that if we require this then we will be signing hook's own death warrant before it has even been born.

              As a side note we should remove the ability to specify an includefile in the hooks.php - there is no reason not to autoload.

              Summary

              In its present state I do not feel that this issue is ready for integration, or the right direction for Moodle in its current form.

              We have yet to properly define the problem, and we have to properly define the goals we wish to achieve.
              We need to say what the functionality should specifically not be trying to achieve just as much as we should be defining what it should be trying to achieve.

              Show
              dobedobedoh Andrew Nicols added a comment - All, I've been reviewing this issue for integration, however I currently have some concern about its direction. It appears that some of the concerns raised throughout this issue have been ignored, and that other issues are being used to force this issue in when it does not seem to be ready. These feelings have been echoed by others in the discussion above. Firstly the issue here seems a little confused. There's much talk of different terminology - hooks, callbacks, etc. but little discussion on what these should actually do within a Moodle context. The solution provided currently seems to be a version of hooks which takes a lot from our renderables and event 1 APIs, but this results in an extremely top-heavy and clunky implementation. There doesn't seem to be any true specifcation - sure we have a wiki page called "Hooks spec", but it's a collection of goals, notes on existing code, and a code example. This does not a specification make. I'm also concerned about the sheer amount of things that this implemention of hooks will 'solve': MDL-51867 MDL-24359 MDL-43742 MDL-43808 MDL-48012 MDL-46611 MDL-39709 Many others I'm sure Numerous references in developer chat, meetings, etc. Of these, I feel that many have very different requirements, and some of them feel like an abuse of a Hooking system. As one example, the usage demonstrated in MDL-51867 is what Hooks should specifically not be – as Frédéric Massart so eloquently puts it, "it should not be an API hidden in the hook system". I see exactly the same in the example provided in the branch - inplace_editable. This is currently implemented as a component_callback() and it is being switched to a hook targetted at one specific usage. Furthermore the execute() function for inplace_editable() performs enough magic that we have essentially created a halt mechanism in the execution code. What is the benefit of this being called as a Hook over the current functionality? Not including the lib.php? That isn't enough IMO. Another concern I have here is the provided ability to target specific components or plugins. From my understanding of this issue to date, and from my presence in numerous lenghty meetings to discuss this functionality, I thought we were intending to separate out hooks, and what we have been (incorrectly) calling callbacks, but we seem to be doing them both here in one confused implementation. Who is to say that a local plugin doesn't want to listen to the hooks fired against a block for example? I think that we have to be very careful about filtering the hooks in this fashion as it is detrimental to the concept of the hooks as a whole. However, my main concern with the current design of this issue is that it is entirely top-heavy, which I suspect means that it will be sparsely implemented. The example hook classes that you give are great examples of this - 140 lines of comment + code for inplace_editable. Even just writing the boilerplate here is time consuming, repetitive, and wasteful. The benefits claimed here are the creation of consistent and documented API, but at the expense of an extremely time-consuming process to add hook calls. I think that there are other ways of documenting hooks and achieving these goals. Having a brief look at how other frameworks handle the definition and invocation of hooks enforces my feelings here. In the case of Wordpress, a hook is invoked: do_action( 'wp_login', $user->user_login, $user ); This will call any implementation of the hook, and pass it the username, and the user object. Some of these hooks have inline documentation, but most don't have anything. Discovery is via a couple of places, for example: https://codex.wordpress.org/Plugin_API/Action_Reference/wp_login http://adambrown.info/p/wp_hooks grep "(do_action|apply_filters)" And in the case of Drupal: \Drupal::moduleHandler()->alter('file_url', $uri); This will call the function hook_file_url_alter and pass it the $uri which can be defined as pass-by-reference in the function signature. There is no documentation for this hook at the point at which it is invoked, but each hook has an example implementation defined. This example defition is then used to generate the documentation: https://github.com/drupal/drupal/blob/c6c691343f5a6bd00bf32cbd3199017e65a4a5be/core/lib/Drupal/Core/File/file.api.php#L42 https://api.drupal.org/api/drupal/modules%21system%21system.api.php/function/hook_file_url_alter/7 I also feel that there should be few (if any) subscribers to the hooks in core, with some being allowed in core plugins. We should not be using hooks as first-class API. This seems to be applied in drupal too ( https://api.drupal.org/api/drupal/includes!module.inc/group/hooks/7): The example functions included are not part of the Drupal core, they are just models that you can modify. Only the hooks implemented within modules are executed when running Drupal. Moving forward For this issue to move forward I believe we need to flesh out the specification, and to think about what we are actually trying to achieve. I have been discussing this issue with Frédéric Massart amongst other and we have independently come to very similar alternative conclusions. Putting our heads together we both feel that hooks should be defined and invoked at the same time, and much more simply: // Invocation and definition. \core\hook::fire('post_course_edit', $course); We both felt that only accepting a single argument to the fire function would be clearest and that only accepting a stdClass would also allow for clearer use at implementation. Where a hook wishes to pass multiple objects: // Invocation and definition. $someothervalue = 1; \core\hook::fire('post_course_edit', (object) ['course' => $course, 'someothervalue' => &$someothervalue]); On the implementation side: // Example implementation. namespace \local\foo;   class hooks { public static function post_course_edit(\core\hook\facade $hook) { // Update the summary of the course. if (strpos($hook->data->summary, '@copy;') === false) { $hook->data->summary .= '<p>@copy; 2016 Cheese Makers Incorporated.</p>'; } } } We also felt that having the fire() function create a new \core\hook\facade and pass that into the function would also allow for future expansion - e.g. functions such as halt(). When it comes to discoverability I feel we need to look around at what others are doing, and what does and does not work for them. I do not feel that we need to create entire classes and infrastuctures to provide discoverability and documentation, and I think that if we require this then we will be signing hook's own death warrant before it has even been born. As a side note we should remove the ability to specify an includefile in the hooks.php - there is no reason not to autoload. Summary In its present state I do not feel that this issue is ready for integration, or the right direction for Moodle in its current form. We have yet to properly define the problem, and we have to properly define the goals we wish to achieve. We need to say what the functionality should specifically not be trying to achieve just as much as we should be defining what it should be trying to achieve.
              Hide
              cibot CiBoT added a comment -

              Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

              Show
              cibot CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
              Hide
              bushido Mark Nielsen added a comment -

              Since Andrew was citing some other examples, I thought I might as well throw in my usual Symfony example, http://symfony.com/doc/current/components/event_dispatcher/introduction.html - Drupal uses this as well, but I haven't checked under the hood to see if they specifically use it for their hook system. Some nice things about Symfony's implementation:

              • Anything can listen to anything else, as it should be.
              • The definition of an event class is entirely optional as you can use just the base event class. This is very nice because for simple events, you can just reuse existing event definitions. If you are getting more complicated, then you can define a your own event class to handle the complexity. An event class is good to have as it is a contract between the dispatching code and the listeners. This does not mean that every event needs to be unique, you can mix and match event classes to reduce boilerplate.
              • I do like the idea of defining event names as constants as it helps to find usages, though grepping probably works almost as well.

              I'm necessarily advocating using Symfony's Event Dispatcher under the hood, but it is an excellent implementation.

              Completely switching topics: I did notice in the pull request that the defined hook had private data members and only getters. IMO the listeners should have full access and should be able to write to the data members. This allows you to use hooks to modify data prior to some action, like saving. Either way, you need a way to to return modifications otherwise you are missing out on a large use case for hooks (Note: I haven't looked at the whole PR, so there might be a way to do this already).

              I am looking forward to a hook API, thanks for working on this, cheers!

              Show
              bushido Mark Nielsen added a comment - Since Andrew was citing some other examples, I thought I might as well throw in my usual Symfony example, http://symfony.com/doc/current/components/event_dispatcher/introduction.html - Drupal uses this as well, but I haven't checked under the hood to see if they specifically use it for their hook system. Some nice things about Symfony's implementation: Anything can listen to anything else, as it should be. The definition of an event class is entirely optional as you can use just the base event class. This is very nice because for simple events, you can just reuse existing event definitions. If you are getting more complicated, then you can define a your own event class to handle the complexity. An event class is good to have as it is a contract between the dispatching code and the listeners. This does not mean that every event needs to be unique, you can mix and match event classes to reduce boilerplate. I do like the idea of defining event names as constants as it helps to find usages, though grepping probably works almost as well. I'm necessarily advocating using Symfony's Event Dispatcher under the hood, but it is an excellent implementation. Completely switching topics: I did notice in the pull request that the defined hook had private data members and only getters. IMO the listeners should have full access and should be able to write to the data members. This allows you to use hooks to modify data prior to some action, like saving. Either way, you need a way to to return modifications otherwise you are missing out on a large use case for hooks (Note: I haven't looked at the whole PR, so there might be a way to do this already). I am looking forward to a hook API, thanks for working on this, cheers!
              Hide
              marina Marina Glancy added a comment -
              Show
              marina Marina Glancy added a comment - My current branch for the history: https://github.com/marinaglancy/moodle/compare/master...wip-MDL-44078-master
              Hide
              damyon Damyon Wiese added a comment -

              Just commenting to say that this issue was discussed heavily between the dev leaders and we have an agreement on the way forward. This will be shared in detail ASAP on this issue and the linked spec but please hold off on further work/comments/discussions until then.

              Show
              damyon Damyon Wiese added a comment - Just commenting to say that this issue was discussed heavily between the dev leaders and we have an agreement on the way forward. This will be shared in detail ASAP on this issue and the linked spec but please hold off on further work/comments/discussions until then.
              Hide
              dobedobedoh Andrew Nicols added a comment -

              Just commenting that, after my integration review of this issue, I had a quick go at putting together a proof of concept for the style of system that Frédéric Massart and I had discussed.

              I've pushed that to https://github.com/andrewnicols/moodle/commits/MDL-44078-master and it is a fully working prototype. Currently missing unit tests.

              I've implemented the functionality that I described before. The basic invocation is:

              \core\hook::fire('pre_page_heading');
              

              Arguments can be specified and must be in the format of a single stdClass:

              \core\hook::fire('pre_course_delete', (object) ['course' => $course]);
              

              Since complex objects are always passed by reference this fits well with the design of hooks. Any callable implementing a hook can modify the passed object and it will be updated.

              Where a simple object must be passed, it must be passed-by-reference:

              \core\hook::fire('pre_message_send', (object) ['author' => $user, 'messagebody' => &$message, 'recipient' => $userto]);
              

              It is possible to deprecate a hook to mark it for retirement in line with the standard deprecation policies:

              $hook = \core\hook::create('pre_course_foo');
              $hook->deprecate('3.1', '3.5');
              $hook->fire();
              

              This will print a debugging notice at DEBUG_DEVELOPER level to inform developers that the 'pre_course_foo' hook has been deprecated in version 3.1 and will be removed in 3.5.

              Similarly it is possible to deprecate argument keys:

              $hook = \core\hook::create('pre_course_delete', (object) ['course' => $course, 'courseid' => $course->id]);
              $hook->deprecate_key('courseid', '3.1', '3.5');
              $hook->fire();
              

              Similarly it is possible to use the create incantation without deprecating if desired:

              $hook = \core\hook::create('pre_course_delete', (object) ['course' => $course]);
              $hook->fire();
              

              Discoverability of hooks is through grep:

              git grep -E '\\core\\hook::(create|fire)'
              

              This is common with wordpress, drupal, symfony, and others.

              Documentation could be provided via example implementations, similar to the way in which drupal does so.

              Listening to hooks is via the same kind of specification in db/hooks.php:

              $callables = [
                  [
                      'hookname' => 'pre_course_delete',
                      'callable' => '\local_foo\hooks::pre_course_delete',
                      'priority' => 100,
                  ],
              ];
              

              All callables must be autoloadable or functions already globally accessible in some fashion.

              When a hook is fired, and it has callables matched against it, a hook\facade is created. This contains the data passed in the second argument, and implements functions halt(), to prevent execution of subsequent callables, and is_stopped(); to determine whether halt has already been called.

              Where one of the argument keys has been deprecated the argument stdClss is transformed into a hook\data object which additionally report the deprecation upon get/set only.

              An example implementation may look like:

              namespace \local_foo;
              class hooks {
                public static function pre_course_edit(\core\hook\facade $hook) {
                  if (strpos($hook->data->course->summary, '&copy;') === false) {
                    $hook->data->course->summary .= '<br><p>&copy; ' . date('Y') . ' Moodle Pty Ltd';
                  }
                  $hook->halt();
                }
              }
              

              In my testing I was able to fire 1,000,000 different hooks in 2.4 seconds when they have no callables. Obviously each callable adds additional time dependant upon what it does. I haven't yet benchmarked the performance for each callable but I anticipate it to be pretty darn fast. Display of deprecation notices is similarly low cost.

              Show
              dobedobedoh Andrew Nicols added a comment - Just commenting that, after my integration review of this issue, I had a quick go at putting together a proof of concept for the style of system that Frédéric Massart and I had discussed. I've pushed that to https://github.com/andrewnicols/moodle/commits/MDL-44078-master and it is a fully working prototype. Currently missing unit tests. I've implemented the functionality that I described before. The basic invocation is: \core\hook::fire('pre_page_heading'); Arguments can be specified and must be in the format of a single stdClass: \core\hook::fire('pre_course_delete', (object) ['course' => $course]); Since complex objects are always passed by reference this fits well with the design of hooks. Any callable implementing a hook can modify the passed object and it will be updated. Where a simple object must be passed, it must be passed-by-reference: \core\hook::fire('pre_message_send', (object) ['author' => $user, 'messagebody' => &$message, 'recipient' => $userto]); It is possible to deprecate a hook to mark it for retirement in line with the standard deprecation policies: $hook = \core\hook::create('pre_course_foo'); $hook->deprecate('3.1', '3.5'); $hook->fire(); This will print a debugging notice at DEBUG_DEVELOPER level to inform developers that the 'pre_course_foo' hook has been deprecated in version 3.1 and will be removed in 3.5. Similarly it is possible to deprecate argument keys: $hook = \core\hook::create('pre_course_delete', (object) ['course' => $course, 'courseid' => $course->id]); $hook->deprecate_key('courseid', '3.1', '3.5'); $hook->fire(); Similarly it is possible to use the create incantation without deprecating if desired: $hook = \core\hook::create('pre_course_delete', (object) ['course' => $course]); $hook->fire(); Discoverability of hooks is through grep: git grep -E '\\core\\hook::(create|fire)' This is common with wordpress, drupal, symfony, and others. Documentation could be provided via example implementations, similar to the way in which drupal does so. Listening to hooks is via the same kind of specification in db/hooks.php: $callables = [ [ 'hookname' => 'pre_course_delete', 'callable' => '\local_foo\hooks::pre_course_delete', 'priority' => 100, ], ]; All callables must be autoloadable or functions already globally accessible in some fashion. When a hook is fired, and it has callables matched against it, a hook\facade is created. This contains the data passed in the second argument, and implements functions halt() , to prevent execution of subsequent callables, and is_stopped() ; to determine whether halt has already been called. Where one of the argument keys has been deprecated the argument stdClss is transformed into a hook\data object which additionally report the deprecation upon get/set only. An example implementation may look like: namespace \local_foo; class hooks { public static function pre_course_edit(\core\hook\facade $hook) { if (strpos($hook->data->course->summary, '&copy;') === false) { $hook->data->course->summary .= '<br><p>&copy; ' . date('Y') . ' Moodle Pty Ltd'; } $hook->halt(); } } In my testing I was able to fire 1,000,000 different hooks in 2.4 seconds when they have no callables. Obviously each callable adds additional time dependant upon what it does. I haven't yet benchmarked the performance for each callable but I anticipate it to be pretty darn fast. Display of deprecation notices is similarly low cost.
              Hide
              damyon Damyon Wiese added a comment -

              Ok - here is the summary of the dev leader discussions and the agreed way forward.

              The reason that we have groups of people debating different solutions is because we have different developers envisioning different use cases - and the best solution appears to be different based on the intended use case.

              Use A) simple extension points that can be littered throughout Moodle in order to create points of the code at which any plugin can execute some code.

              Use B) replacement for the existing component_callback mechanism for core-plugin communication through a well defined and documented API.

              Desirable properties of A)

              • Performant
              • Discoverable
              • Quick to implement
              • Always broadcast to all plugins

              Desirable properties of B)

              • Performant
              • Discoverable
              • Well documented
              • Unit Testable
              • Able to handle deprecations properly (not just say something was removed, but provide info on how to update code)
              • Able to broadcast to either all plugins of a single type, all plugin types or a single specified component
              • Able to convert existing component_callback uses

              Hence we have 2 proposed solutions - Andrews one which is only suitable for A) and Marinas one (based on Petrs proposal) which is only suitable for B). The main important difference between the 2 seems to be that B requires a new class per hook point, because the hook point is actually part of a real API, and so the properties and types should be validated, the proper use of the class documented etc.

              So the dev leader decision is to add 2 new apis to handle the different uses. Originally they seemed so similar that this was not necessary - but it has emerged that there are irreconcilable differences in the way they should be implemented. We also do not want to add the simple hooks without the replacement for component_callback - because that would lead to devs implementing real APIs through hooks (which is no improvement from the callback mess we have now).

              One new API will be hooks similar to Andrews proposed patch, and the other will be Marinas patch for core->subsystem->plugin communication under a new namespace (maybe built into core_component). I will assign this issue to myself and incorporate these patches into this new solution.

              So we will have a way to add hooks wherever they are requested by external devs - and a way to properly communicate between plugins and core/subsystems. However - core code will be very heavily discouraged from using hooks because hooks != well defined and documented apis - and core should be communicating only through well defined and documented apis.

              Expect to see some updates to the spec page and a new patch on this issue ASAP.

              Show
              damyon Damyon Wiese added a comment - Ok - here is the summary of the dev leader discussions and the agreed way forward. The reason that we have groups of people debating different solutions is because we have different developers envisioning different use cases - and the best solution appears to be different based on the intended use case. Use A) simple extension points that can be littered throughout Moodle in order to create points of the code at which any plugin can execute some code. Use B) replacement for the existing component_callback mechanism for core-plugin communication through a well defined and documented API. Desirable properties of A) Performant Discoverable Quick to implement Always broadcast to all plugins Desirable properties of B) Performant Discoverable Well documented Unit Testable Able to handle deprecations properly (not just say something was removed, but provide info on how to update code) Able to broadcast to either all plugins of a single type, all plugin types or a single specified component Able to convert existing component_callback uses Hence we have 2 proposed solutions - Andrews one which is only suitable for A) and Marinas one (based on Petrs proposal) which is only suitable for B). The main important difference between the 2 seems to be that B requires a new class per hook point, because the hook point is actually part of a real API, and so the properties and types should be validated, the proper use of the class documented etc. So the dev leader decision is to add 2 new apis to handle the different uses. Originally they seemed so similar that this was not necessary - but it has emerged that there are irreconcilable differences in the way they should be implemented. We also do not want to add the simple hooks without the replacement for component_callback - because that would lead to devs implementing real APIs through hooks (which is no improvement from the callback mess we have now). One new API will be hooks similar to Andrews proposed patch, and the other will be Marinas patch for core->subsystem->plugin communication under a new namespace (maybe built into core_component). I will assign this issue to myself and incorporate these patches into this new solution. So we will have a way to add hooks wherever they are requested by external devs - and a way to properly communicate between plugins and core/subsystems. However - core code will be very heavily discouraged from using hooks because hooks != well defined and documented apis - and core should be communicating only through well defined and documented apis. Expect to see some updates to the spec page and a new patch on this issue ASAP.
              Hide
              mudrd8mz David Mudrák added a comment -

              Thanks everybody for great comments here. It is very interesting to watch this issue evolution.

              Damyon, please let me say (again I guess) that it will really help to see these new APIs with actual implementation of places where they are used. Maybe we could follow up on the list of all existing "callbacks" that Marina summarised at the beginning of this saga and we could see how each of them would be converted to these new APIs? I must admit I still have troubles with fully understanding the needs and reasoning for the scope and complexity of the B) case.

              Secondly, are we going to distinguish the two types (Andrew's / Marina's) by their name somehow? Or will both called just hooks? Or simple hooks versus advanced hooks etc?

              Show
              mudrd8mz David Mudrák added a comment - Thanks everybody for great comments here. It is very interesting to watch this issue evolution. Damyon, please let me say (again I guess) that it will really help to see these new APIs with actual implementation of places where they are used. Maybe we could follow up on the list of all existing "callbacks" that Marina summarised at the beginning of this saga and we could see how each of them would be converted to these new APIs? I must admit I still have troubles with fully understanding the needs and reasoning for the scope and complexity of the B) case. Secondly, are we going to distinguish the two types (Andrew's / Marina's) by their name somehow? Or will both called just hooks? Or simple hooks versus advanced hooks etc?
              Hide
              jleyva Juan Leyva added a comment -

              +1 to David comments, and also, please, it would be great to have a document or something listing the use cases... where to use A or B or the current events API, I'm thinking in new developers coming to Moodle (or even existing ones)

              Show
              jleyva Juan Leyva added a comment - +1 to David comments, and also, please, it would be great to have a document or something listing the use cases... where to use A or B or the current events API, I'm thinking in new developers coming to Moodle (or even existing ones)
              Hide
              damyon Damyon Wiese added a comment -

              Sending this dev-leader agreed solution for peer review.

              Some notes about the new solution.

              Hooks are supported with the same API as proposed by Andrew (and his tests).

              Callbacks are supported with the same API as proposed by Marina/Petr (and their tests).

              I converted 98% of the existing uses of component_callback to avoid just creating another way to do things inconsistently. The things I didn't convert I will explain below.

              • get_types (this component_callback is already deprecated)
              • grade_item_update (this component callback is used both as a callback and directly by plugins API)
              • pluginfile variants - huge job to convert - needs to be done all at once
              • ltisource variants - these are doable - I just ran out of steam and want to get this up for peer review

              Still to do before 3.2:

              Convert more APIs to callbacks (git grep plugin_callback, git grep plugins_with_function, ltisource).

              Add hooks to the most obvious places (stages of page rendering, navigation tree building).

              Add callback for modform editing (building, validation and processing the submission via a single callback).

              Edit/add all the docs pages for all the updated APIs

              Show
              damyon Damyon Wiese added a comment - Sending this dev-leader agreed solution for peer review. Some notes about the new solution. Hooks are supported with the same API as proposed by Andrew (and his tests). Callbacks are supported with the same API as proposed by Marina/Petr (and their tests). I converted 98% of the existing uses of component_callback to avoid just creating another way to do things inconsistently. The things I didn't convert I will explain below. get_types (this component_callback is already deprecated) grade_item_update (this component callback is used both as a callback and directly by plugins API) pluginfile variants - huge job to convert - needs to be done all at once ltisource variants - these are doable - I just ran out of steam and want to get this up for peer review Still to do before 3.2: Convert more APIs to callbacks (git grep plugin_callback, git grep plugins_with_function, ltisource). Add hooks to the most obvious places (stages of page rendering, navigation tree building). Add callback for modform editing (building, validation and processing the submission via a single callback). Edit/add all the docs pages for all the updated APIs
              Hide
              cibot CiBoT added a comment -

              Fails against automated checks.

              Checked MDL-44078 using repository: git://github.com/damyon/moodle.git

              • master [branch: MDL-44078-master | CI Job]
                • Warn: The MDL-44078-master branch at git://github.com/damyon/moodle.git has not been rebased recently (>20 days ago).
                • Error: The MDL-44078-master branch at git://github.com/damyon/moodle.git does not apply clean to origin/master

              More information about this report

              Show
              cibot CiBoT added a comment - Fails against automated checks. Checked MDL-44078 using repository: git://github.com/damyon/moodle.git master [branch: MDL-44078-master | CI Job ] Warn: The MDL-44078 -master branch at git://github.com/damyon/moodle.git has not been rebased recently (>20 days ago). Error: The MDL-44078 -master branch at git://github.com/damyon/moodle.git does not apply clean to origin/master More information about this report
              Hide
              cibot CiBoT added a comment -

              Fails against automated checks.

              Checked MDL-44078 using repository: git://github.com/damyon/moodle.git

              More information about this report

              Show
              cibot CiBoT added a comment - Fails against automated checks. Checked MDL-44078 using repository: git://github.com/damyon/moodle.git master (93 errors / 86 warnings) [branch: MDL-44078-master | CI Job ] phplint (0/0) , phpcs (37/84) , js (0/0) , css (0/0) , phpdoc (49/0) , commit (7/2) , savepoint (0/0) , thirdparty (0/0) , grunt (0/0) , shifter (0/0) , travis (0/0) , More information about this report
              Hide
              damyon Damyon Wiese added a comment -

              Here is an important doc to go with this - to be promoted to a central obvious place on dev docs. It describes many general concepts in Moodle about inter-component communication.

              https://docs.google.com/document/d/1Z-vRWztT05bsb9b5KbBpLRP26oa3KTx3thB52_BW5VY/edit?usp=sharing

              Show
              damyon Damyon Wiese added a comment - Here is an important doc to go with this - to be promoted to a central obvious place on dev docs. It describes many general concepts in Moodle about inter-component communication. https://docs.google.com/document/d/1Z-vRWztT05bsb9b5KbBpLRP26oa3KTx3thB52_BW5VY/edit?usp=sharing
              Hide
              damyon Damyon Wiese added a comment -

              When converting things I did fix some things that were never working

              like this: https://github.com/damyon/moodle/compare/a0a6367...MDL-44078-master#diff-dc341ed527d978c9673824a4e871d98dL772

              (it's a component_callback - but it's declared inside a class so it never worked)

              Show
              damyon Damyon Wiese added a comment - When converting things I did fix some things that were never working like this: https://github.com/damyon/moodle/compare/a0a6367...MDL-44078-master#diff-dc341ed527d978c9673824a4e871d98dL772 (it's a component_callback - but it's declared inside a class so it never worked)
              Hide
              cibot CiBoT added a comment -

              Code verified against automated checks with warnings.

              Checked MDL-44078 using repository: git://github.com/damyon/moodle.git

              More information about this report

              Show
              cibot CiBoT added a comment - Code verified against automated checks with warnings. Checked MDL-44078 using repository: git://github.com/damyon/moodle.git master (0 errors / 73 warnings) [branch: MDL-44078-master | CI Job ] phplint (0/0) , phpcs (0/72) , js (0/0) , css (0/0) , phpdoc (0/0) , commit (0/1) , savepoint (0/0) , thirdparty (0/0) , grunt (0/0) , shifter (0/0) , travis (0/0) , More information about this report
              Hide
              damyon Damyon Wiese added a comment -

              Cheating the process a bit and sending this for integration. It is an important patch that has been waiting for peer review for a month - it is rebased and passing unit tests.

              I don't want new component_callbacks sneaking in while this patch sits in limbo and I don't want rebase conflicts.

              Show
              damyon Damyon Wiese added a comment - Cheating the process a bit and sending this for integration. It is an important patch that has been waiting for peer review for a month - it is rebased and passing unit tests. I don't want new component_callbacks sneaking in while this patch sits in limbo and I don't want rebase conflicts.
              Hide
              marina Marina Glancy added a comment -

              Just wanted to write here my observations from looking at the code. I do not see a need in creating two new apis when one of them is not used anywhere. There are no "db/hooks.php" files in this branch and I don't see why we can't use "callbacks" (db/callbacks.php) for anything that "hooks" can do.

              (I put "hooks" and "callbacks" in quotes because I refer to terms from Damyon's patch, not the terms used in the above comments)

              Show
              marina Marina Glancy added a comment - Just wanted to write here my observations from looking at the code. I do not see a need in creating two new apis when one of them is not used anywhere. There are no "db/hooks.php" files in this branch and I don't see why we can't use "callbacks" (db/callbacks.php) for anything that "hooks" can do. (I put "hooks" and "callbacks" in quotes because I refer to terms from Damyon's patch, not the terms used in the above comments)
              Hide
              damyon Damyon Wiese added a comment -

              I tried to explain the use cases for hooks and callbacks in the above google doc which will go in dev docs with this issue.

              Quoting myself:

              When should I create a hook and when should I create a callback?
              A: Creating a hook is like saying “I don’t know what this could be used for, but right here would be a good place for others to extend this code”. A hook comes with very little documentation in the code, just a listing in the relevant db/hooks.php file. Hooks are easy and quick to implement. It doesn’t matter if no plugins receive the hook. Hooks are not part of a multi-stage process.

              Good candidates for hooks are:
              Pre/post page header
              Pre/post page content
              Pre/post page footer
              Pre/post user menu
              Pre/post navigation tree

              Callbacks are more well defined. It takes more work to add a new callback than to add a hook because you must implement the callback base class. This class itself is a great place to document your API and define which fields can be modified and which ones cannot. Callbacks can evolve, supporting backwards compatibility and deprecation. Callbacks can be used for a multi-stage process (e.g. form definition, validation and submission).

              Good candidates for callbacks are:
              inplace editable
              Activity grade rescaling
              Fragments API
              Comments API permission check callbacks
              Extending forms

              Generally I think callbacks are very useful and hooks are just for a few random places that plugins/themes want to hook into and won't be used much.

              Show
              damyon Damyon Wiese added a comment - I tried to explain the use cases for hooks and callbacks in the above google doc which will go in dev docs with this issue. Quoting myself: When should I create a hook and when should I create a callback? A: Creating a hook is like saying “I don’t know what this could be used for, but right here would be a good place for others to extend this code”. A hook comes with very little documentation in the code, just a listing in the relevant db/hooks.php file. Hooks are easy and quick to implement. It doesn’t matter if no plugins receive the hook. Hooks are not part of a multi-stage process. Good candidates for hooks are: Pre/post page header Pre/post page content Pre/post page footer Pre/post user menu Pre/post navigation tree Callbacks are more well defined. It takes more work to add a new callback than to add a hook because you must implement the callback base class. This class itself is a great place to document your API and define which fields can be modified and which ones cannot. Callbacks can evolve, supporting backwards compatibility and deprecation. Callbacks can be used for a multi-stage process (e.g. form definition, validation and submission). Good candidates for callbacks are: inplace editable Activity grade rescaling Fragments API Comments API permission check callbacks Extending forms Generally I think callbacks are very useful and hooks are just for a few random places that plugins/themes want to hook into and won't be used much.
              Hide
              sbourget Stephen Bourget added a comment - - edited

              Just one question about the proposed patch...

              In https://github.com/damyon/moodle/commit/1c9b848169fa28718335117f18931967a638f424 you converted one of the rating API callbacks (mod_data_rating_can_see_item_ratings) to use this new callback method, but the other two rating callbacks (data_rating_permissions and data_rating_validate) (https://github.com/damyon/moodle/blob/4155e0fb7d030c1113a33ee903758777203d86b1/mod/data/lib.php#L1409) were not converted. Is this intentional? It seems odd that any developer using the ratings API would need to implement part of the API using this new approach and part of the API using the old approach.

              Show
              sbourget Stephen Bourget added a comment - - edited Just one question about the proposed patch... In https://github.com/damyon/moodle/commit/1c9b848169fa28718335117f18931967a638f424 you converted one of the rating API callbacks (mod_data_rating_can_see_item_ratings) to use this new callback method, but the other two rating callbacks (data_rating_permissions and data_rating_validate) ( https://github.com/damyon/moodle/blob/4155e0fb7d030c1113a33ee903758777203d86b1/mod/data/lib.php#L1409 ) were not converted. Is this intentional? It seems odd that any developer using the ratings API would need to implement part of the API using this new approach and part of the API using the old approach.
              Hide
              damyon Damyon Wiese added a comment -

              Thanks Stephen - I should have looked closer for uses of the deprecated plugin_callback - there are only 11 spread across comments, ratings, dnd and grading so I guess it would make sense to convert them all in the same patch. I'll add this to the existing patch tomorrow.

              Show
              damyon Damyon Wiese added a comment - Thanks Stephen - I should have looked closer for uses of the deprecated plugin_callback - there are only 11 spread across comments, ratings, dnd and grading so I guess it would make sense to convert them all in the same patch. I'll add this to the existing patch tomorrow.
              Hide
              damyon Damyon Wiese added a comment -

              The ratings api has now been fully converted to new callbacks.

              Show
              damyon Damyon Wiese added a comment - The ratings api has now been fully converted to new callbacks.
              Hide
              dmonllao David Monllaó added a comment -

              The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

              TIA and ciao

              Show
              dmonllao David Monllaó added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
              Hide
              cibot CiBoT added a comment -

              Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!

              Show
              cibot CiBoT added a comment - Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!
              Hide
              cibot CiBoT added a comment -

              Fails against automated checks.

              Checked MDL-44078 using repository: git://github.com/damyon/moodle.git

              • master [branch: MDL-44078-master | CI Job]
                • Error: The MDL-44078-master branch at git://github.com/damyon/moodle.git does not apply clean to origin/master

              More information about this report

              Show
              cibot CiBoT added a comment - Fails against automated checks. Checked MDL-44078 using repository: git://github.com/damyon/moodle.git master [branch: MDL-44078-master | CI Job ] Error: The MDL-44078 -master branch at git://github.com/damyon/moodle.git does not apply clean to origin/master More information about this report
              Hide
              poltawski Dan Poltawski added a comment -

              I haven't properly reviewed this yet, but quick comments:

              1. I really think this needs posting about on moodle.org (unless I missed it?)
              2. I have concerns about the amount of perceived boilerplate this change will bring. I noticed some copy and paste errors in the code around that.
              3. I don't know about the BC/support status of the two new entrypoints
              4. When we are talking about 'broadcast' things, it makes me think about ordering of executution and whether there needs to be control of that
              Show
              poltawski Dan Poltawski added a comment - I haven't properly reviewed this yet, but quick comments: I really think this needs posting about on moodle.org (unless I missed it?) I have concerns about the amount of perceived boilerplate this change will bring. I noticed some copy and paste errors in the code around that. I don't know about the BC/support status of the two new entrypoints When we are talking about 'broadcast' things, it makes me think about ordering of executution and whether there needs to be control of that
              Hide
              damyon Damyon Wiese added a comment -

              1. You don't think this was discussed enough ? https://moodle.org/mod/forum/discuss.php?d=327349
              2. Well - a lot of the boiler plate is just for getters and setters. This has been discussed before and I thought the preference was for us to type these out rather than use magic methods.
              3. Only callbacks can support BC - and the way I did this we can remove the deprecation just by changing the class the callback inherits from.
              4. You can control the ordering of execution. The receivers array allows you to set the priority - I just don't use it anywhere in this patch (but I think Petrs tests cover it).

              Show
              damyon Damyon Wiese added a comment - 1. You don't think this was discussed enough ? https://moodle.org/mod/forum/discuss.php?d=327349 2. Well - a lot of the boiler plate is just for getters and setters. This has been discussed before and I thought the preference was for us to type these out rather than use magic methods. 3. Only callbacks can support BC - and the way I did this we can remove the deprecation just by changing the class the callback inherits from. 4. You can control the ordering of execution. The receivers array allows you to set the priority - I just don't use it anywhere in this patch (but I think Petrs tests cover it).
              Hide
              poltawski Dan Poltawski added a comment -

              Re: discussion - given the impact this has on all developers, I do think its worth wider circulation of the proposed solution (I was aware of my own thread).

              Show
              poltawski Dan Poltawski added a comment - Re: discussion - given the impact this has on all developers, I do think its worth wider circulation of the proposed solution (I was aware of my own thread).
              Hide
              poltawski Dan Poltawski added a comment -

              I didn't do very well explaining myself yesterday, just to clarify my comments a little..

              2. I am not so concerned about callback implementers, more 'consumers'. To learn the lessons of plugin authors who have said fairly strongly that the amount of boilerplate necesssary to move from add_to_log to events is a burden for them. But I think in this case this is just a function of how you have chosen to do the conversion introduce new classes to support the autoloading.

              Maybe I am over thinking it, but would it be a terrible idea to support old-skool modules with less disruptive changes by allowing them to move to the new api by supporting use of a non-autoloaded function (and thus optional file inclusion requirement like webservice definitions) and then its not so radical moving to the new system?

              3. What I mean is - how will we support the evolution of callbacks and hooks over time? When one is added, do we guarantee to keep it? What about changing parameters in them?

              4. Cool, I didn't notice, there is a lot of code

              Show
              poltawski Dan Poltawski added a comment - I didn't do very well explaining myself yesterday, just to clarify my comments a little.. 2. I am not so concerned about callback implementers, more 'consumers'. To learn the lessons of plugin authors who have said fairly strongly that the amount of boilerplate necesssary to move from add_to_log to events is a burden for them. But I think in this case this is just a function of how you have chosen to do the conversion introduce new classes to support the autoloading. Maybe I am over thinking it, but would it be a terrible idea to support old-skool modules with less disruptive changes by allowing them to move to the new api by supporting use of a non-autoloaded function (and thus optional file inclusion requirement like webservice definitions) and then its not so radical moving to the new system? 3. What I mean is - how will we support the evolution of callbacks and hooks over time? When one is added, do we guarantee to keep it? What about changing parameters in them? 4. Cool, I didn't notice, there is a lot of code
              Hide
              damyon Damyon Wiese added a comment -

              2. OK - for implementors there is no requirement to use autoloading - they only need to implement a php "callable" and point to it from the hooks/callbacks files. I chose to put these in new files in auto loaded locations because I think it is tidy but there are no restrictions on this part at all.
              3. Hooks - no there is no mechanism for deprecations - this is meant to be a sloppy API and is intended to change (break) over time. It's just a fancy way of adding core hacks without hacking core. Callbacks - yes we can deprecate callbacks/parameters - because we have a true "class" we can add parameters, deprecate functions etc - all business as usual. The constructors I wrote take an array of arguments instead of naming individual arguments for exactly this reason.

              Show
              damyon Damyon Wiese added a comment - 2. OK - for implementors there is no requirement to use autoloading - they only need to implement a php "callable" and point to it from the hooks/callbacks files. I chose to put these in new files in auto loaded locations because I think it is tidy but there are no restrictions on this part at all. 3. Hooks - no there is no mechanism for deprecations - this is meant to be a sloppy API and is intended to change (break) over time. It's just a fancy way of adding core hacks without hacking core. Callbacks - yes we can deprecate callbacks/parameters - because we have a true "class" we can add parameters, deprecate functions etc - all business as usual. The constructors I wrote take an array of arguments instead of naming individual arguments for exactly this reason.
              Hide
              poltawski Dan Poltawski added a comment -

              Useful comment from Michael on forum: https://moodle.org/mod/forum/discuss.php?d=327349#p1357377

              Show
              poltawski Dan Poltawski added a comment - Useful comment from Michael on forum: https://moodle.org/mod/forum/discuss.php?d=327349#p1357377
              Hide
              dmonllao David Monllaó added a comment -

              Thanks for moving this issue despite all discussions around it during years. I think that this will make a lot of devs happy, I have some comments about the proposal.

              1. I don't love that we implement our stuff again having well tested solutions like symfony's event dispatcher or the zf alternative, I don't personally care if the moodle package contains 2 more MB, we have too many things that work the moodle way and this is a barrier for new devs, most of php devs work already know how to work with symfony and its events, I would prefer it to a moodle solution (my personal opinion) https://github.com/symfony/symfony/blob/master/src/Symfony/Component/EventDispatcher/EventDispatcher.php#L36 is passing the Event object to the listeners and the Event is returned back to the dispatch() caller (https://github.com/symfony/symfony/blob/master/src/Symfony/Component/EventDispatcher/EventDispatcherInterface.php#L32) wouldn't be very hard to extend Symfony\Component\EventDispatcher\Event to set there the data the caller allow the called functions to read and change.
              2. https://github.com/damyon/moodle/compare/5a1728d...MDL-44078-master#diff-7ddc3423e816672018962011841a651aR39 The data available to a hook is likely to change between releases with no deprecation warning This is not awesome, hooks arguments are part of the API, we can not sell a new cool API to hack moodle if we break it when we want, I agree that it is still better than hacking core codebase though but IMO we need to agree on a deprecation process for changes in the callbacks data. By the way, this backwards compatibility argument is the same I've heard against a dependency injection system in moodle
              3. Regarding the order of execution that Dan asked about. I haven't seen any execution order control in the patch, just hardcoded priority numbers, it would be enough for core callbacks but not for 3rd party plugins. I mean that 3rd party plugins are not supposed to be aware of all other 3rd party moodle plugins and set the priority according to it. I remember discussing about this with Dan a few months ago, let me know if I am missing something because I haven't read all the discussions along the years, but it looks to me that if we want to guarantee to admins that they can safely install plugins (safely = no unexpected conflicts) like they currently do things are not as straight forward as they look in the patch. Callers pass arguments by reference to the called functions, how can we know that a 3rd party plugin will not break what another 3rd party plugin is doing? A plugin dev must be able to rely on the changes they apply to the data they receive from the caller. I can't think of a fully automated solution for this, the best I can think of is to flag called functions that change the contents they receive (in most cases plugins just add data which shouldn't be a problem) and show a warning to the admin if they try to install a second plugin that is changing the same hook contents. This would require callbacks to be coded defensively and 3rd party devs to add a section in their README.md about how they change the data so admins can know when there is risk of conflict. It is a pity that we have to bother admins about it, any better alternative?
              Show
              dmonllao David Monllaó added a comment - Thanks for moving this issue despite all discussions around it during years. I think that this will make a lot of devs happy, I have some comments about the proposal. I don't love that we implement our stuff again having well tested solutions like symfony's event dispatcher or the zf alternative, I don't personally care if the moodle package contains 2 more MB, we have too many things that work the moodle way and this is a barrier for new devs, most of php devs work already know how to work with symfony and its events, I would prefer it to a moodle solution (my personal opinion) https://github.com/symfony/symfony/blob/master/src/Symfony/Component/EventDispatcher/EventDispatcher.php#L36 is passing the Event object to the listeners and the Event is returned back to the dispatch() caller ( https://github.com/symfony/symfony/blob/master/src/Symfony/Component/EventDispatcher/EventDispatcherInterface.php#L32 ) wouldn't be very hard to extend Symfony\Component\EventDispatcher\Event to set there the data the caller allow the called functions to read and change. https://github.com/damyon/moodle/compare/5a1728d...MDL-44078-master#diff-7ddc3423e816672018962011841a651aR39 The data available to a hook is likely to change between releases with no deprecation warning This is not awesome, hooks arguments are part of the API, we can not sell a new cool API to hack moodle if we break it when we want, I agree that it is still better than hacking core codebase though but IMO we need to agree on a deprecation process for changes in the callbacks data. By the way, this backwards compatibility argument is the same I've heard against a dependency injection system in moodle Regarding the order of execution that Dan asked about. I haven't seen any execution order control in the patch, just hardcoded priority numbers, it would be enough for core callbacks but not for 3rd party plugins. I mean that 3rd party plugins are not supposed to be aware of all other 3rd party moodle plugins and set the priority according to it. I remember discussing about this with Dan a few months ago, let me know if I am missing something because I haven't read all the discussions along the years, but it looks to me that if we want to guarantee to admins that they can safely install plugins (safely = no unexpected conflicts) like they currently do things are not as straight forward as they look in the patch. Callers pass arguments by reference to the called functions, how can we know that a 3rd party plugin will not break what another 3rd party plugin is doing? A plugin dev must be able to rely on the changes they apply to the data they receive from the caller. I can't think of a fully automated solution for this, the best I can think of is to flag called functions that change the contents they receive (in most cases plugins just add data which shouldn't be a problem) and show a warning to the admin if they try to install a second plugin that is changing the same hook contents. This would require callbacks to be coded defensively and 3rd party devs to add a section in their README.md about how they change the data so admins can know when there is risk of conflict. It is a pity that we have to bother admins about it, any better alternative?
              Hide
              damyon Damyon Wiese added a comment -

              1. It does not make sense to start using random pieces of symfony in Moodle without jumping all the way in using it properly. This would be Moodle X.
              2. When thinking about hooks - accept they are nasty hack points and we won't use them much because they don't provide a real API. The main candidates for the hooks are going to be "page rendering stages" and "navigation tree". Things in the $PAGE will change and stuff in the nav tree will change and we won't be considering how it will break plugins using the hooks.
              3. None of the things I have converted care at all about the order of execution - I expect this is a very rare requirement. There is no perfect solution - not all plugins can be "first" or "last". If you know of a plugin you are trying to get ahead of you can look up it's priority and beat it.

              Show
              damyon Damyon Wiese added a comment - 1. It does not make sense to start using random pieces of symfony in Moodle without jumping all the way in using it properly. This would be Moodle X. 2. When thinking about hooks - accept they are nasty hack points and we won't use them much because they don't provide a real API. The main candidates for the hooks are going to be "page rendering stages" and "navigation tree". Things in the $PAGE will change and stuff in the nav tree will change and we won't be considering how it will break plugins using the hooks. 3. None of the things I have converted care at all about the order of execution - I expect this is a very rare requirement. There is no perfect solution - not all plugins can be "first" or "last". If you know of a plugin you are trying to get ahead of you can look up it's priority and beat it.
              Hide
              poltawski Dan Poltawski added a comment - - edited

              2. For the record, I agree that this is 'OK'. It stays stable each major release only, we document it clearly that way. But, as Michael says here - using better terminology will really help with this. Basically the use case for this is a way to 'distribute' a hack (because you can't distribute a git patch with a plugin sanely)
              3. A good example of this not being that rare I can imagine is munging a course, or the url rewriting thing - we need to be really careful about the broadcast stuff

              Show
              poltawski Dan Poltawski added a comment - - edited 2. For the record, I agree that this is 'OK'. It stays stable each major release only, we document it clearly that way. But, as Michael says here - using better terminology will really help with this. Basically the use case for this is a way to 'distribute' a hack (because you can't distribute a git patch with a plugin sanely) 3. A good example of this not being that rare I can imagine is munging a course, or the url rewriting thing - we need to be really careful about the broadcast stuff
              Hide
              dmonllao David Monllaó added a comment -

              Moodle X release date 2040 sorry, I'm joking, what I mean is that I doubt that at short or mid term we will break with everything, and even less after what happened with moodle 2. If we just have a few hooks like the current ones + rendering stuff this approach may be enough, if we want to add a new long term solution for people to hack core massively then I would vote for a proper solution using components already known by devs that would make easier in future to move to another framework. If I would be a 3rd party hacking moodle what would excite me is something like this https://api.drupal.org/api/drupal/includes!module.inc/group/hooks/8.x.

              I'm aware that symfony's components approach don't have many fans in Moodle, I wouldn't be disappointed if we go with this solution, I just think that is is another step that moves us away from the PHP community.

              Show
              dmonllao David Monllaó added a comment - Moodle X release date 2040 sorry, I'm joking, what I mean is that I doubt that at short or mid term we will break with everything, and even less after what happened with moodle 2. If we just have a few hooks like the current ones + rendering stuff this approach may be enough, if we want to add a new long term solution for people to hack core massively then I would vote for a proper solution using components already known by devs that would make easier in future to move to another framework. If I would be a 3rd party hacking moodle what would excite me is something like this https://api.drupal.org/api/drupal/includes!module.inc/group/hooks/8.x . I'm aware that symfony's components approach don't have many fans in Moodle, I wouldn't be disappointed if we go with this solution, I just think that is is another step that moves us away from the PHP community.
              Hide
              dmonllao David Monllaó added a comment -

              The integration of this issue has been delayed to next week because the integration period is over (Monday, Tuesday) and testing must happen on Wednesday.

              This rigid timeframe on each integration/testing cycle aims to produce a better and clear separation and organization of tasks for everybody.

              This is a bulk-automated message, so if you want to blame somebody/thing/where, don't do it ehere (use git instead) :-D :-P

              Apologies for the inconvenience, this will be integrated next week. Thanks for your collaboration & ciao

              Show
              dmonllao David Monllaó added a comment - The integration of this issue has been delayed to next week because the integration period is over (Monday, Tuesday) and testing must happen on Wednesday. This rigid timeframe on each integration/testing cycle aims to produce a better and clear separation and organization of tasks for everybody. This is a bulk-automated message, so if you want to blame somebody/thing/where, don't do it ehere (use git instead) :-D :-P Apologies for the inconvenience, this will be integrated next week. Thanks for your collaboration & ciao
              Hide
              dmonllao David Monllaó added a comment -

              The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

              TIA and ciao

              Show
              dmonllao David Monllaó added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
              Hide
              cibot CiBoT added a comment -

              Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!

              Show
              cibot CiBoT added a comment - Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!
              Hide
              cibot CiBoT added a comment -

              Fails against automated checks.

              Checked MDL-44078 using repository: git://github.com/damyon/moodle.git

              • master [branch: MDL-44078-master | CI Job]
                • Warn: The MDL-44078-master branch at git://github.com/damyon/moodle.git has not been rebased recently (>20 days ago).
                • Error: The MDL-44078-master branch at git://github.com/damyon/moodle.git does not apply clean to origin/master

              More information about this report

              Show
              cibot CiBoT added a comment - Fails against automated checks. Checked MDL-44078 using repository: git://github.com/damyon/moodle.git master [branch: MDL-44078-master | CI Job ] Warn: The MDL-44078 -master branch at git://github.com/damyon/moodle.git has not been rebased recently (>20 days ago). Error: The MDL-44078 -master branch at git://github.com/damyon/moodle.git does not apply clean to origin/master More information about this report
              Hide
              markn Mark Nelson added a comment -

              There is a tried and tested way used by other frameworks to achieve this goal. Yet it seems we are re-inventing the wheel, again! Saying, 'we can do that in Moodle X' tells me that we don't believe this is a long-term solution. (I haven't looked at the code in much detail - merely commenting on design concept, feel free to ignore.)

              Show
              markn Mark Nelson added a comment - There is a tried and tested way used by other frameworks to achieve this goal. Yet it seems we are re-inventing the wheel, again! Saying, 'we can do that in Moodle X' tells me that we don't believe this is a long-term solution. (I haven't looked at the code in much detail - merely commenting on design concept, feel free to ignore.)
              Hide
              dobedobedoh Andrew Nicols added a comment -

              Hi all,

              The integration team has been discussing this issue in order to give a single consensus decision on the state of the issue as a whole.

              Our discussion is ongoing, but we wanted to keep everyone informed of where we are at this stage.

              We’ve discussed several aspects including naming of the concepts, whether implementations such as the Symfony EventDispatcher should be used instead, whether these two concepts should really be a single concept, and whether this implementation is possibly a little inside-out.

              There are several points we have not yet discussed or finalised and we hope to continue our discussions on these areas.

              Naming
              Summary

              The team unanimously decided that the term callback is incorrect and should not be used.
              It was felt that more discussion was required on the appropriate terms to use.
              The term hook seems appropriate as a basis, though the related terminology may need to be defined still.

              Rationale

              In discussing the naming of these concepts, we unanimously decided that the term callback is incorrectly used in the majority of the current patch. These approaches both resemble a traditional event-based system in almost every instance, however the term Event has already been over-used in Moodle and we felt that it would be best not to add additional versions of it here.
              The term hook seems appropriate though is only a part of the picture. The terminology around hooks, and their dispatch needs to be discussed further.
              We did briefly consider using the hook analogy an extending it to have terms such as fish, and for the arguments passed to be some form of bait. This was, however, felt to be too confusing.

              Symfony EventDispatcher
              Summary

              The team unanimously decided that the Symfony EventDispatcher is not well suited to Moodle, however the approaches used there are both valid and relevant, and should be used as inspiration for our own system.

              Rationale

              We wanted to look at other implementations to see whether there was any approach that we could incorporate directly.

              The obvious system to look into is the Symfony EventDispatcher, which is readily available as a Component which can be brought into Moodle wholesale and used.

              David and I (Andrew) were initially keen on this idea, both having used Symfony before.

              The Symfony EventDispatcher model has many benefits, including prior knowledge for many developers, the fact that it is already widely used and issues resolved, that it is well-tested, and well documented.

              However, there were two main concerns with its use in Moodle:
              The naming of the classes could become confusing for many developers. Although these are namespaced, having two systems both named Event, and having a vaguely similar purpose and a very different result for the end user would make documentation confusing and be a barrier to new developers and users of either system.
              In order to make these less confusing we would have to somehow rename the classes (either a class_alias approach, or an extension of the relevant classes);
              The EventDispatcher is designed to be run such that all subscribers are added beforehand, and the Dispatcher is responsible for sorting those and calling them.
              There is no mechanism to add caching, or to lazily load the listeners (either as required, or wholesale on the first dispatch). Moodle does not make use of a Dependency Injection model like Symfony, which would make this approach easier and adding one is beyond the scope of this issue.
              As a result, the EventDispatcher would have to be made aware of all hooks early on, and would need significant wrapping to make this work within the Moodle architecture.

              While neither of these issues are insurmountable, we felt that in order to use the Symfony EventDispatcher outright, it would need to be wrapped extensively.
              We also discussed the required backportability of such a solution and again this led to our decision.

              The small size of the library, combined with the issues above meant that we felt we would be better to continue writing our own approach to the problem in this instance, rather than wrapping it.

              We did feel that we should look to Symfony for guidance on how we can best implement the solution. Their approach has many benefits and there are many gains to be had by creating a system similar to theirs.

              Combining Hooks and Callbacks
              Summary

              The team unanimously decided that the two approaches should be combined into a single approach and that the approach should support both uses.
              It was felt that combining these would give the best experience for developers using these features.

              Rationale

              The team discussed whether the two approaches (callback, and hook) should be combined into a single API.

              We unanimously felt that this should be the case - there should be a single system which incorporates requirements of both systems.

              We considered the suggested rationale for having two methods - specifically that the hook concept is designed to be quick, easy, and ‘hackier’. It was considered to have less need for backwards-compatibility.

              It was felt that having two very similar but slightly different implementations would be bad for developers in general. Converting a hook into a callback, for example, would not be achievable in a backwards-compatible fashion, and the hook concept would end up as a second-class citizen.

              It was also felt that developers wishing to contribute their own hook or callback should not have to jump through hoops in order to get these into core.

              One major point in favour of combining the two approaches, and of changing the way in which the are hooked is that the arguments become more flexible and can still allow for backwards compatibility and future proofing to be considered. This can be achieved via naming of hooks, rather than using classes to name them, and provision of some generic data models.

              A key benefit of this is that it becomes more readily possible to dispatch a hook which has no data. There are many times in Moodle where the hook is just there to announce that something is about to happen, but the only relevant arguments would be a global variable such as $PAGE, or $OUTPUT.

              To give a specific example of this, we may wish to announce that the header is about to be printed, to allow some form of additional content to be added to the header. Since such a change should be made using the page API made available via the global variable $PAGE, it does not make sense to pass this global variable as an argument. This is likely to be a data-less call.

              Such calls may be made via code something like:

              \core\hook::fire('core-header-pre');
              

              With each the handler receiving a standard data model containing no data:

              public static function my_core_header_pre_handler(hook_data $data) {
              }
              

              Where data is present, this can either be coerced into a generic data object:

              $hookdata = \core\hook::fire('mod_forum-create_post-pre', $post);
               
              public static function my_mod_forum_create_post_pre(\core\hook\hook_data $data) {
                  $subject = $data->get_data()->subject;
              }
              

              Or a data object extending the generic data object can be provided:

              $hookdata = \core\hook::fire('mod_forum-create_post-pre', new hook\create_post_pre($post));
               
              public static function my_mod_forum_create_post_pre(\mod_forum\hook\create_post_pre $data) {
                  $subject = $data->get_subject();
              }
              

              Note: The above samples are still part of the area we are discussing.

              Summary

              We are still discussing this proposal and the intricacies of it and we will do our best to keep this issue up-to-date.

              I realise that some of the above may be a little contentious as some believe that there should be a single system, and some that there should be two. For me personally, one of my main concerns with the current split is that the split is in the wrong place. We have \core\hook which are hooks, and core
              callback
              , which are technically speaking both hooks and callbacks depending on whether the dispatch function is passed a first argument or not.

              Andrew

              Show
              dobedobedoh Andrew Nicols added a comment - Hi all, The integration team has been discussing this issue in order to give a single consensus decision on the state of the issue as a whole. Our discussion is ongoing, but we wanted to keep everyone informed of where we are at this stage. We’ve discussed several aspects including naming of the concepts, whether implementations such as the Symfony EventDispatcher should be used instead, whether these two concepts should really be a single concept, and whether this implementation is possibly a little inside-out. There are several points we have not yet discussed or finalised and we hope to continue our discussions on these areas. Naming Summary The team unanimously decided that the term callback is incorrect and should not be used. It was felt that more discussion was required on the appropriate terms to use. The term hook seems appropriate as a basis, though the related terminology may need to be defined still. Rationale In discussing the naming of these concepts, we unanimously decided that the term callback is incorrectly used in the majority of the current patch. These approaches both resemble a traditional event-based system in almost every instance, however the term Event has already been over-used in Moodle and we felt that it would be best not to add additional versions of it here. The term hook seems appropriate though is only a part of the picture. The terminology around hooks, and their dispatch needs to be discussed further. We did briefly consider using the hook analogy an extending it to have terms such as fish , and for the arguments passed to be some form of bait . This was, however, felt to be too confusing. Symfony EventDispatcher Summary The team unanimously decided that the Symfony EventDispatcher is not well suited to Moodle, however the approaches used there are both valid and relevant, and should be used as inspiration for our own system. Rationale We wanted to look at other implementations to see whether there was any approach that we could incorporate directly. The obvious system to look into is the Symfony EventDispatcher, which is readily available as a Component which can be brought into Moodle wholesale and used. David and I (Andrew) were initially keen on this idea, both having used Symfony before. The Symfony EventDispatcher model has many benefits, including prior knowledge for many developers, the fact that it is already widely used and issues resolved, that it is well-tested, and well documented. However, there were two main concerns with its use in Moodle: The naming of the classes could become confusing for many developers. Although these are namespaced, having two systems both named Event, and having a vaguely similar purpose and a very different result for the end user would make documentation confusing and be a barrier to new developers and users of either system. In order to make these less confusing we would have to somehow rename the classes (either a class_alias approach, or an extension of the relevant classes); The EventDispatcher is designed to be run such that all subscribers are added beforehand, and the Dispatcher is responsible for sorting those and calling them. There is no mechanism to add caching, or to lazily load the listeners (either as required, or wholesale on the first dispatch). Moodle does not make use of a Dependency Injection model like Symfony, which would make this approach easier and adding one is beyond the scope of this issue. As a result, the EventDispatcher would have to be made aware of all hooks early on, and would need significant wrapping to make this work within the Moodle architecture. While neither of these issues are insurmountable, we felt that in order to use the Symfony EventDispatcher outright, it would need to be wrapped extensively. We also discussed the required backportability of such a solution and again this led to our decision. The small size of the library, combined with the issues above meant that we felt we would be better to continue writing our own approach to the problem in this instance, rather than wrapping it. We did feel that we should look to Symfony for guidance on how we can best implement the solution. Their approach has many benefits and there are many gains to be had by creating a system similar to theirs. Combining Hooks and Callbacks Summary The team unanimously decided that the two approaches should be combined into a single approach and that the approach should support both uses. It was felt that combining these would give the best experience for developers using these features. Rationale The team discussed whether the two approaches ( callback , and hook ) should be combined into a single API. We unanimously felt that this should be the case - there should be a single system which incorporates requirements of both systems. We considered the suggested rationale for having two methods - specifically that the hook concept is designed to be quick, easy, and ‘hackier’. It was considered to have less need for backwards-compatibility. It was felt that having two very similar but slightly different implementations would be bad for developers in general. Converting a hook into a callback , for example, would not be achievable in a backwards-compatible fashion, and the hook concept would end up as a second-class citizen. It was also felt that developers wishing to contribute their own hook or callback should not have to jump through hoops in order to get these into core. One major point in favour of combining the two approaches, and of changing the way in which the are hooked is that the arguments become more flexible and can still allow for backwards compatibility and future proofing to be considered. This can be achieved via naming of hooks, rather than using classes to name them, and provision of some generic data models. A key benefit of this is that it becomes more readily possible to dispatch a hook which has no data. There are many times in Moodle where the hook is just there to announce that something is about to happen, but the only relevant arguments would be a global variable such as $PAGE , or $OUTPUT . To give a specific example of this, we may wish to announce that the header is about to be printed, to allow some form of additional content to be added to the header. Since such a change should be made using the page API made available via the global variable $PAGE , it does not make sense to pass this global variable as an argument. This is likely to be a data-less call. Such calls may be made via code something like: \core\hook::fire('core-header-pre'); With each the handler receiving a standard data model containing no data: public static function my_core_header_pre_handler(hook_data $data) { } Where data is present, this can either be coerced into a generic data object: $hookdata = \core\hook::fire('mod_forum-create_post-pre', $post);   public static function my_mod_forum_create_post_pre(\core\hook\hook_data $data) { $subject = $data->get_data()->subject; } Or a data object extending the generic data object can be provided: $hookdata = \core\hook::fire('mod_forum-create_post-pre', new hook\create_post_pre($post));   public static function my_mod_forum_create_post_pre(\mod_forum\hook\create_post_pre $data) { $subject = $data->get_subject(); } Note: The above samples are still part of the area we are discussing. Summary We are still discussing this proposal and the intricacies of it and we will do our best to keep this issue up-to-date. I realise that some of the above may be a little contentious as some believe that there should be a single system, and some that there should be two. For me personally, one of my main concerns with the current split is that the split is in the wrong place. We have \core\hook which are hooks, and core callback , which are technically speaking both hooks and callbacks depending on whether the dispatch function is passed a first argument or not. Andrew
              Hide
              dougiamas Martin Dougiamas added a comment -

              Two comments from me:

              1) I seriously can't believe we are still talking about starting this issue from scratch after all the discussions so far. A lot of discussions have happened and a lot of code has been written. This is not an academic argument ... we are wasting a lot of expensive time and need to get past this ASAP. Restarting will kill this for 3.2.

              2) I see a lot of references to what to "other developers" will think/feel. Has anyone actually bothered to survey them? My suspicion is that they don't care too much about this implementation or that, as long as it's well documented and not at odds with general Moodle architecture. And most of all, actually implemented.

              Show
              dougiamas Martin Dougiamas added a comment - Two comments from me: 1) I seriously can't believe we are still talking about starting this issue from scratch after all the discussions so far. A lot of discussions have happened and a lot of code has been written. This is not an academic argument ... we are wasting a lot of expensive time and need to get past this ASAP. Restarting will kill this for 3.2. 2) I see a lot of references to what to "other developers" will think/feel. Has anyone actually bothered to survey them? My suspicion is that they don't care too much about this implementation or that, as long as it's well documented and not at odds with general Moodle architecture. And most of all, actually implemented.
              Hide
              dobedobedoh Andrew Nicols added a comment -

              Taking myself off as reviewer of this issue as I'm not currently on the integration cycle.

              Show
              dobedobedoh Andrew Nicols added a comment - Taking myself off as reviewer of this issue as I'm not currently on the integration cycle.
              Hide
              poltawski Dan Poltawski added a comment - - edited

              I'm just setting the state of this issue to be reopened which is more accurate because its not being considered for integration. There are discussions on how this is moving forward, but they are stalling.

              If you want my opinon on why this issue keeps stalling, it's because of this.

              Edit: Just to note, I think that Google Doc breaking communication and structures down is immensely valuable to explain things to people - i've used it a few times already. I'd love to see that put somewhere proper.

              Show
              poltawski Dan Poltawski added a comment - - edited I'm just setting the state of this issue to be reopened which is more accurate because its not being considered for integration. There are discussions on how this is moving forward, but they are stalling. If you want my opinon on why this issue keeps stalling, it's because of this . Edit: Just to note, I think that Google Doc breaking communication and structures down is immensely valuable to explain things to people - i've used it a few times already. I'd love to see that put somewhere proper.
              Hide
              cibot CiBoT added a comment -

              Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

              Show
              cibot CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

                Dates

                • Created:
                  Updated: