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

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

    Details

    • Type: Improvement
    • Status: Open
    • Priority: Minor
    • Resolution: Unresolved
    • Affects Version/s: 2.6.1
    • Fix Version/s: None
    • Component/s: Policy
    • Labels:
    • Affected Branches:
      MOODLE_26_STABLE

      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

          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 Mudrak 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 Mudrak 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 Mudrak 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 Mudrak 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 Mudrak 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 Mudrak 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 Mudrak 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 Mudrak 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 Mudrak 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 Mudrak 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 Mudrak 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 Mudrak 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 Mudrak 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 Mudrak 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.

              People

              • Votes:
                17 Vote for this issue
                Watchers:
                25 Start watching this issue

                Dates

                • Created:
                  Updated: