Moodle
  1. Moodle
  2. MDL-39846

Define new event dispatcher behaviour and implement it

    Details

    • Type: Task Task
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 2.5
    • Fix Version/s: 2.6
    • Component/s: Events API
    • Labels:
    • Story Points:
      20
    • Rank:
      171
    • Sprint:
      BACKEND Sprint 2

      Description

      1/ Describe new event dispatcher
      http://docs.moodle.org/dev/Event_2#Stage_1

      2/ Implement new event dispatching

      3/ Add phpunit support for event observation

        Issue Links

          Activity

          Hide
          Rajesh Taneja added a comment -

          Adding patch created by Petr.

          @Petr: Patch looks Great, should we also support core_event_manager::registerobserver and core_event_manager_unregisterobsever for more flexibility and when plugin is installed/uninstalled, it can take responsibility to register/remove event observers.

          This might help:

          1. Avoid maintaining old events.php
          2. Access to observers during install
          3. Avoid use of caching
          4. Easy-of-use to observe events.

          We can also provide hook api in mod/plugin/lib.php register_observer() to keep things simple.

          Show
          Rajesh Taneja added a comment - Adding patch created by Petr. @Petr: Patch looks Great, should we also support core_event_manager::registerobserver and core_event_manager_unregisterobsever for more flexibility and when plugin is installed/uninstalled, it can take responsibility to register/remove event observers. This might help: Avoid maintaining old events.php Access to observers during install Avoid use of caching Easy-of-use to observe events. We can also provide hook api in mod/plugin/lib.php register_observer() to keep things simple.
          Hide
          Petr Škoda added a comment -

          This would significantly change the design, my idea is to maintain the observers automatically as the db/events.php files are added or removed, no install/upgrade at all. The only drawback is that the observers must not fail with fatal error before the install or upgrade is executed.

          Show
          Petr Škoda added a comment - This would significantly change the design, my idea is to maintain the observers automatically as the db/events.php files are added or removed, no install/upgrade at all. The only drawback is that the observers must not fail with fatal error before the install or upgrade is executed.
          Hide
          Rajesh Taneja added a comment -

          Thanks for quick reply Petr,

          Developers who wants to observe events, will probably don't mind events.php or register_event().
          But it will be nice to consider api registration, as this is the only time we can change things to make it simple and easy.

          Show
          Rajesh Taneja added a comment - Thanks for quick reply Petr, Developers who wants to observe events, will probably don't mind events.php or register_event(). But it will be nice to consider api registration, as this is the only time we can change things to make it simple and easy.
          Hide
          Petr Škoda added a comment -

          Submitting for peer review: please ignore the fields and logging related methods in the base class, I am going to work on that later.

          Show
          Petr Škoda added a comment - Submitting for peer review: please ignore the fields and logging related methods in the base class, I am going to work on that later.
          Hide
          Petr Škoda added a comment - - edited

          Submitting for new peer review - dispatching manager and base class is now implemented and open for discussion, there is also a sample conversion of role (un)assigning events and enrol_category plugin.

          There are a few TODOs:

          • event level is mostly undefined
          • event language strings are missing
          • I decided to remove the affectedobject because it seemed confusing to me
          • the spec is not updated to match the implementation details (such as code examples, class names, etc.)
          • get_description() and other informative methods are still missing

          We need to decide if this is the right direction or not.

          Show
          Petr Škoda added a comment - - edited Submitting for new peer review - dispatching manager and base class is now implemented and open for discussion, there is also a sample conversion of role (un)assigning events and enrol_category plugin. There are a few TODOs: event level is mostly undefined event language strings are missing I decided to remove the affectedobject because it seemed confusing to me the spec is not updated to match the implementation details (such as code examples, class names, etc.) get_description() and other informative methods are still missing We need to decide if this is the right direction or not.
          Hide
          Rajesh Taneja added a comment -

          Thanks for the patch Petr,

          I tried to get my head around this and IMO direction seems fine.

          Few things to consider
          1. manager can be a singleton, it won't make much difference, but make it easy to override it (if need be)
          2. not sure if event trigger should internally call manager::dispatch(), seems like control is with event and not with manager.
          3. #L6R158 while evaluating context, should it also, check if cm or categoryid is set and evaluate context.
          Typos at few places
          1. #L6R121 "developers to something" => "developers to do something"
          2. #L6R126 and #L6R132, "is no" => "is not"
          3. #L6R398 "evetn" => "event"
          Show
          Rajesh Taneja added a comment - Thanks for the patch Petr, I tried to get my head around this and IMO direction seems fine. Few things to consider manager can be a singleton, it won't make much difference, but make it easy to override it (if need be) not sure if event trigger should internally call manager::dispatch(), seems like control is with event and not with manager. #L6R158 while evaluating context, should it also, check if cm or categoryid is set and evaluate context. Typos at few places #L6R121 "developers to something" => "developers to do something" #L6R126 and #L6R132, "is no" => "is not" #L6R398 "evetn" => "event"
          Hide
          Petr Škoda added a comment -

          1. no, manager is not a singleton intentionally, if you really really want to replace it use custom classloader
          2. event calling the manager::dispatch($this) method is fundamental part of the design that hides implementation from developers - nobody is allow to interfere with event dispatching
          3. category and cm are not accepted, context is the required event parameter that will get stored in event

          Thanks for the typo reports!

          Show
          Petr Škoda added a comment - 1. no, manager is not a singleton intentionally, if you really really want to replace it use custom classloader 2. event calling the manager::dispatch($this) method is fundamental part of the design that hides implementation from developers - nobody is allow to interfere with event dispatching 3. category and cm are not accepted, context is the required event parameter that will get stored in event Thanks for the typo reports!
          Hide
          Petr Škoda added a comment -

          Resubmitting for the last review (hopefully) before integration request. I have updated the spec at http://docs.moodle.org/dev/Event_2, everything except logging specific stuff (such as levels) should be in place. Ciao.

          Show
          Petr Škoda added a comment - Resubmitting for the last review (hopefully) before integration request. I have updated the spec at http://docs.moodle.org/dev/Event_2 , everything except logging specific stuff (such as levels) should be in place. Ciao.
          Hide
          Dan Poltawski added a comment -

          Petr, did you see Tim's comments about naming here: https://moodle.org/mod/forum/discuss.php?d=229425#p1006156

          Seems sensible to me.

          Show
          Dan Poltawski added a comment - Petr, did you see Tim's comments about naming here: https://moodle.org/mod/forum/discuss.php?d=229425#p1006156 Seems sensible to me.
          Hide
          Petr Škoda added a comment -

          I did not invent the crud, but I like it because it tells you exactly what to put there - I have no problem remembering all possible values. Ask Fred, he is the author.

          I disagree with the data['extradata'] because everything in data property is data, I do not see a need to repeat that word.

          Show
          Petr Škoda added a comment - I did not invent the crud, but I like it because it tells you exactly what to put there - I have no problem remembering all possible values. Ask Fred, he is the author. I disagree with the data ['extradata'] because everything in data property is data, I do not see a need to repeat that word.
          Hide
          Tim Hunt added a comment -

          I still have to give this a -1 because:

          1. it mixes up the dispatch logic, which should be in manager::dispatch with the event base class.

          2. we are still requiring developers to define a different class for every single even they raise, which is unnecessary. It should be one class per type of event.

          However, Petr and I were unable to have a meaningful discussion about this before, I so I am not hopeful now.

          If you are just going to carry on despite objections, one point you do need to address is

          3. the PHPdoc comments are woefully in adequate.

          some examples:

              /**
               * Override in subclass.
               *
               * Set all required data properties:
               *  1/ crud - letter [crud]
               *  2/ level - number 1...100
               *
               * Optionally validate $this->data['extra'].
               *
               * @return void
               */
              protected abstract function init();
          

          If I am a deveper trying to override this, how do I decide which integer to pick for level? Why might I want to validate extra?

              /**
               * Returns localised general event name.
               *
               * Override in subclass, we can not make it static and abstract at the same time.
               *
               * @return string|\lang_string
               */
              public static function get_name() {
          

          Is the 'static and abstract' information relevant to someone trying to use this API? What will the name I set here be used for? Does it been to be understandable to all users, or just technical people like admins?

          Show
          Tim Hunt added a comment - I still have to give this a -1 because: 1. it mixes up the dispatch logic, which should be in manager::dispatch with the event base class. 2. we are still requiring developers to define a different class for every single even they raise, which is unnecessary. It should be one class per type of event. However, Petr and I were unable to have a meaningful discussion about this before, I so I am not hopeful now. If you are just going to carry on despite objections, one point you do need to address is 3. the PHPdoc comments are woefully in adequate. some examples: /** * Override in subclass. * * Set all required data properties: * 1/ crud - letter [crud] * 2/ level - number 1...100 * * Optionally validate $ this ->data['extra']. * * @ return void */ protected abstract function init(); If I am a deveper trying to override this, how do I decide which integer to pick for level? Why might I want to validate extra? /** * Returns localised general event name. * * Override in subclass, we can not make it static and abstract at the same time. * * @ return string|\lang_string */ public static function get_name() { Is the 'static and abstract' information relevant to someone trying to use this API? What will the name I set here be used for? Does it been to be understandable to all users, or just technical people like admins?
          Hide
          Petr Škoda added a comment - - edited

          Tim:

          1/ How would you protect the internal state of each event instance if you were able to manually dispatch the event? You would have to significantly open the API which would allow developers to set invalid event state. We must protect the internal state of event during the dispatching process, note that the $event->trigger() may return before the observers are notified about its existence.

          2/ The one class per event is fundamental part of the design that was submitted for review, you would have to write a completely new spec and different implementation. You can not persuade me unless you show me a real code that clearly demonstrates your proposal is better.

          3/ Yes, the logging specific parts are not complete (such as level and crud) - I said that above, this information is not used in normal event observers. I suppose get_name() and get_description() will be used mostly in logging API too, is not finalised yet.

          Show
          Petr Škoda added a comment - - edited Tim: 1/ How would you protect the internal state of each event instance if you were able to manually dispatch the event? You would have to significantly open the API which would allow developers to set invalid event state. We must protect the internal state of event during the dispatching process, note that the $event->trigger() may return before the observers are notified about its existence. 2/ The one class per event is fundamental part of the design that was submitted for review, you would have to write a completely new spec and different implementation. You can not persuade me unless you show me a real code that clearly demonstrates your proposal is better. 3/ Yes, the logging specific parts are not complete (such as level and crud) - I said that above, this information is not used in normal event observers. I suppose get_name() and get_description() will be used mostly in logging API too, is not finalised yet.
          Hide
          Dan Poltawski added a comment -

          Hi Petr,

          It'd be nice to see an example of how to unit test one of these individual events in isolation (as a unit), e.g. you could test the result of the cached querie sand the description of those accesslib events.

          Show
          Dan Poltawski added a comment - Hi Petr, It'd be nice to see an example of how to unit test one of these individual events in isolation (as a unit), e.g. you could test the result of the cached querie sand the description of those accesslib events.
          Hide
          Petr Škoda added a comment - - edited

          Unit testing individual events? I would set my own handler that catches '*' and stored the result in some queue. The problem is that you need to prevent all other observers, but that borks other stuff. Maybe we could just test them indirectly in places where the events are actually used.

          Any better idea?

          Show
          Petr Škoda added a comment - - edited Unit testing individual events? I would set my own handler that catches '*' and stored the result in some queue. The problem is that you need to prevent all other observers, but that borks other stuff. Maybe we could just test them indirectly in places where the events are actually used. Any better idea?
          Hide
          Frédéric Massart added a comment -

          Hi Petr,

          I skimmed through the code, and this is what I noticed. Please ignore me if I failed to understand properly what happened in there.

          Event base

          • What if I want to name my event without an object. For example when someone has logged in, it should be core_event_loggedin. The subject of the action is implied to be the current user and should therefore not be part of the event name, but currently we're checking that an object is part of the class name. I don't think it should be the case. (L133)
          • You removed the associated object, but I think we should keep that. Many events rely on more than one object and an action. Some actions are performed on 2 things, being too vague kills the purpose. Example: User added to a cohort, it should be core_event_cohort_member_added. Where cohort is the associated object, otherwise how do you name the event? core_event_cohort_added? core_event_member_added? core_event_member_cohortadded? cohortadded not being a verb... http://docs.moodle.org/dev/Event_2#Structure_of_existing_events
          • Setting $data['courseid'] does not mean that we are specifically in a CONTEXT_COURSE. I would personally prefer using $PAGE first. (L150)
          • We are in a DEBUG_DEVELOPER only statement, but as I like readability, I'd prefer DEBUG_DEVELOPER to be set on each debugging() call. (~L178, ~L387, some other places) (Also useful for greps on specific DEBUG_DEVELOPER debug calls, if ever needed)
          • Why/when $level became an arbitrary number? Is 1 the most important, or the less? When will I decide to actually change that, will that even make sense if everyone decide of its own level on such a large range?
          • No underscores in variable names. (L251)
          • Debugging is not limited to developer (L288, L295, some other places)
          • I find *_cached_record() a bit obscure, unlike the other methods I find that one not very self documenting.
            • This method can actually set a cached record after triggering the event, alert data corruption by the observers...
            • About the get_cached_record, what happens if the entry does not exist any more? Wouldn't this let the developer believe that the information is safe and somewhere, where in fact it is not?
            • It seems pretty hard to make sure that I am using the right $id when calling get_cached_record. Wouldn't it make sense to only have 1 entry per table? An event is supposed to happen on one object, not a group afaik.
            • Can I expect having this information back when I restore the event? If not, then why not making it part of $data? Example: keeping track of the deleted record.
            • 'cached' is not really needed in the method name after all, it's the record that we want, whether it's cached or not... or get_ephemeral_record() if it can never be restored.

          Manager

          • Some methods are lacking PHP Doc
          • L107-112, from the look of it, array_shift() does the same.

          Unit tests

          • That would be nice to have some tests for the data guessing/resolving we're doing in create().

          Cheers!
          Fred

          Show
          Frédéric Massart added a comment - Hi Petr, I skimmed through the code, and this is what I noticed. Please ignore me if I failed to understand properly what happened in there. Event base What if I want to name my event without an object. For example when someone has logged in, it should be core_event_loggedin. The subject of the action is implied to be the current user and should therefore not be part of the event name, but currently we're checking that an object is part of the class name. I don't think it should be the case. (L133) You removed the associated object, but I think we should keep that. Many events rely on more than one object and an action. Some actions are performed on 2 things, being too vague kills the purpose. Example: User added to a cohort, it should be core_event_cohort_member_added. Where cohort is the associated object, otherwise how do you name the event? core_event_cohort_added? core_event_member_added? core_event_member_cohortadded? cohortadded not being a verb... http://docs.moodle.org/dev/Event_2#Structure_of_existing_events Setting $data ['courseid'] does not mean that we are specifically in a CONTEXT_COURSE. I would personally prefer using $PAGE first. (L150) We are in a DEBUG_DEVELOPER only statement, but as I like readability, I'd prefer DEBUG_DEVELOPER to be set on each debugging() call. (~L178, ~L387, some other places) (Also useful for greps on specific DEBUG_DEVELOPER debug calls, if ever needed) Why/when $level became an arbitrary number? Is 1 the most important, or the less? When will I decide to actually change that, will that even make sense if everyone decide of its own level on such a large range? No underscores in variable names. (L251) Debugging is not limited to developer (L288, L295, some other places) I find *_cached_record() a bit obscure, unlike the other methods I find that one not very self documenting. This method can actually set a cached record after triggering the event, alert data corruption by the observers... About the get_cached_record, what happens if the entry does not exist any more? Wouldn't this let the developer believe that the information is safe and somewhere, where in fact it is not? It seems pretty hard to make sure that I am using the right $id when calling get_cached_record. Wouldn't it make sense to only have 1 entry per table? An event is supposed to happen on one object, not a group afaik. Can I expect having this information back when I restore the event? If not, then why not making it part of $data? Example: keeping track of the deleted record. 'cached' is not really needed in the method name after all, it's the record that we want, whether it's cached or not... or get_ephemeral_record() if it can never be restored. Manager Some methods are lacking PHP Doc L107-112, from the look of it, array_shift() does the same. Unit tests That would be nice to have some tests for the data guessing/resolving we're doing in create(). Cheers! Fred
          Hide
          Petr Škoda added a comment -
          • I do not like the idea of events without object, nothing should be implied. For user login we could use \core\event\user_loggedin - it may sound a bit weird because the object would duplicate current user from $event->userid but I think it is better to not have any exceptions in the naming of classes.
          • Adding users to cohort would be \core\event\cohort_member_added where action is "added" and object "cohort_member" and objectid is id of the user that was added and the cohort id is in other['cohortid']
          • Do not rely on PAGE in low level APIs such as events, it is intended for things that deal with current page such as renderers.
          • We were discussing recently new $CFG->slowdebug that would replace if(debugging('', DEBUG_DEVELOPER)), my code there is already compatible with that.
          • Level is not defined yet, see the TODO? it needs to be discussed as part of the logging API, in any case numbers are better than varchars for storage.
          • whoeverinventedtheruleforvarswithoutunderscoresshouldbeshot
          • the record caching is general intentionally, you may have multiple user ids involved in one event, the point here is that you can not expect to get the data after restore, it is also stored by reference because it is object - I used the name "cache" because I am going to use it as caches in enrol/*
          • yep, the more docs the better, I sometimes skip phpdocs when overriding the classes because they should be taken from parent automatically
          • array shift is very slow if used repeatedly on large arrays, it is better to not use it sometimes
          • yep, the more tests the better

          Let's discuss the record caches with some real events tomorrow, thanks a lot.

          Show
          Petr Škoda added a comment - I do not like the idea of events without object, nothing should be implied. For user login we could use \core\event\user_loggedin - it may sound a bit weird because the object would duplicate current user from $event->userid but I think it is better to not have any exceptions in the naming of classes. Adding users to cohort would be \core\event\cohort_member_added where action is "added" and object "cohort_member" and objectid is id of the user that was added and the cohort id is in other ['cohortid'] Do not rely on PAGE in low level APIs such as events, it is intended for things that deal with current page such as renderers. We were discussing recently new $CFG->slowdebug that would replace if(debugging('', DEBUG_DEVELOPER)), my code there is already compatible with that. Level is not defined yet, see the TODO? it needs to be discussed as part of the logging API, in any case numbers are better than varchars for storage. whoeverinventedtheruleforvarswithoutunderscoresshouldbeshot the record caching is general intentionally, you may have multiple user ids involved in one event, the point here is that you can not expect to get the data after restore, it is also stored by reference because it is object - I used the name "cache" because I am going to use it as caches in enrol/* yep, the more docs the better, I sometimes skip phpdocs when overriding the classes because they should be taken from parent automatically array shift is very slow if used repeatedly on large arrays, it is better to not use it sometimes yep, the more tests the better Let's discuss the record caches with some real events tomorrow, thanks a lot.
          Hide
          Frédéric Massart added a comment -
          • "cohort_member" is ambiguous, I would really have to read the event's code to find out what objectid is. Though I guess the event could define get_cohortid(), get_userid(). Probably the better! Should that be a rule/recommendation for events with multiple objects?
          • If we should not rely on PAGE, let's not rely on it at all... the guessing might be too arbitrary here.
          • Understood about get_cached_record, but I'm surprised that we allow devs to do crazy stuff here where as we protected a lot from other places... At least, the setter should be disabled after the event is triggered IMO. I'm still not convinced with the name "cache", it is misleading... we are not caching, nor saving, we're just assigning a value to a variable, and if it does not exist we get it from the database. Would there be another name more general for all type of events (not only enrol/)? Also, what happens in the case of a deleted object? We would most likely use $other to store the original data, but we'd be tempted to use cached_record, wouldn't we?

          Thanks!
          Fred

          Show
          Frédéric Massart added a comment - "cohort_member" is ambiguous, I would really have to read the event's code to find out what objectid is. Though I guess the event could define get_cohortid(), get_userid(). Probably the better! Should that be a rule/recommendation for events with multiple objects? If we should not rely on PAGE, let's not rely on it at all... the guessing might be too arbitrary here. Understood about get_cached_record, but I'm surprised that we allow devs to do crazy stuff here where as we protected a lot from other places... At least, the setter should be disabled after the event is triggered IMO. I'm still not convinced with the name "cache", it is misleading... we are not caching, nor saving, we're just assigning a value to a variable, and if it does not exist we get it from the database. Would there be another name more general for all type of events (not only enrol/)? Also, what happens in the case of a deleted object? We would most likely use $other to store the original data, but we'd be tempted to use cached_record, wouldn't we? Thanks! Fred
          Hide
          Dan Poltawski added a comment -

          (You should remember to use numbers fred )

          D1. If we should not rely on PAGE, let's not rely on it at all... the guessing might be too arbitrary here.

          Agreed, good point. Why not enforce context here?

          Show
          Dan Poltawski added a comment - (You should remember to use numbers fred ) D1. If we should not rely on PAGE, let's not rely on it at all... the guessing might be too arbitrary here. Agreed, good point. Why not enforce context here?
          Hide
          Dan Poltawski added a comment -

          Just to document a discussion from scrum today. We suggested that the name 'snapshot' might be a better description than cached for the get_cached_record() thing. Since its really giving a snapshot of the record at the point of the event firing.

          Show
          Dan Poltawski added a comment - Just to document a discussion from scrum today. We suggested that the name 'snapshot' might be a better description than cached for the get_cached_record() thing. Since its really giving a snapshot of the record at the point of the event firing.
          Hide
          Dan Poltawski added a comment -

          So regarding that unit testing thing, i've already hit my head on it in my hacking on MDL-39956, i've added cohort_create/update/delete. But we are not using those events anywhere, so i've got no way to test it with existing handlers.

          Show
          Dan Poltawski added a comment - So regarding that unit testing thing, i've already hit my head on it in my hacking on MDL-39956 , i've added cohort_create/update/delete. But we are not using those events anywhere, so i've got no way to test it with existing handlers.
          Hide
          Petr Škoda added a comment - - edited

          I have rebased my branch on top of current master and added following (I used a new branch name with suffix 'b'):

          1. renamed record cache to *_record_snapshot() - hopefully it will be more intuitive
          2. new objecttable event property - you need to add it to your init() method if you set objectid, this should make clear what the object actually is
          3. new advanced_testcase::redirectEvents() which is similar to messaging redirection - you can use it in unit tests
          4. context or contextid is now required in ::create()
          5. property iterator support
          Show
          Petr Škoda added a comment - - edited I have rebased my branch on top of current master and added following (I used a new branch name with suffix 'b'): renamed record cache to *_record_snapshot() - hopefully it will be more intuitive new objecttable event property - you need to add it to your init() method if you set objectid, this should make clear what the object actually is new advanced_testcase::redirectEvents() which is similar to messaging redirection - you can use it in unit tests context or contextid is now required in ::create() property iterator support
          Hide
          Dan Poltawski added a comment -

          thanks

          Show
          Dan Poltawski added a comment - thanks
          Hide
          Petr Škoda added a comment - - edited

          Some more improvements:
          1. add_snapshot_record() may be used only before $event->trigger() - this should prevent hacks that try to pass information around or modify state
          2. get_snapshot_record() can not work for restored events - the data is not there any more
          3. it is now possible to hardcode system context in init() method
          4. you can hardcode contextlevel in init() method, it is then used to verify the given context has correct level
          5. there is a new validate_data() at the end of create if you want to validate stuff (or do some dirty hacking, but shhhh)

          Thanks Fred for proposing and discussion these improvements today!

          Show
          Petr Škoda added a comment - - edited Some more improvements: 1. add_snapshot_record() may be used only before $event->trigger() - this should prevent hacks that try to pass information around or modify state 2. get_snapshot_record() can not work for restored events - the data is not there any more 3. it is now possible to hardcode system context in init() method 4. you can hardcode contextlevel in init() method, it is then used to verify the given context has correct level 5. there is a new validate_data() at the end of create if you want to validate stuff (or do some dirty hacking, but shhhh) Thanks Fred for proposing and discussion these improvements today!
          Hide
          Frédéric Massart added a comment - - edited

          I think this should go for integration. At least so that it can be reviewed by someone outside the Backend team. But I think we went the right way, and the design is good enough to allow for changes in the future. Also, I'm pretty sure we've argued enough on the reasons why we need one class per event and not on generic one. Hereafter my perception of the pros vs. cons of the One class per event.

          Pros

          • Events are self documented
          • Events know what they contain and how to handle the data
          • Events can implement specific access control on the data
          • Events are stricter and can enforce accurate data to be used (requirement for logging)
          • Events can easily be identified using their unique names

          Cons

          • It is a bit less straightforward to define a new event
          • Events cannot be shared unless they are defined as core events (that would defeat the purpose of One class per event)

          Something very important that we should not forget is that the events are mainly designed to be used by the logging systems. Those systems should receive accurate data, which is really not the case of the current event_trigger()/add_to_log() function... if we do not put in place a strict structure, we increase the risk of falling back where we suffered serious lack of information.

          Show
          Frédéric Massart added a comment - - edited I think this should go for integration. At least so that it can be reviewed by someone outside the Backend team. But I think we went the right way, and the design is good enough to allow for changes in the future. Also, I'm pretty sure we've argued enough on the reasons why we need one class per event and not on generic one. Hereafter my perception of the pros vs. cons of the One class per event . Pros Events are self documented Events know what they contain and how to handle the data Events can implement specific access control on the data Events are stricter and can enforce accurate data to be used (requirement for logging) Events can easily be identified using their unique names Cons It is a bit less straightforward to define a new event Events cannot be shared unless they are defined as core events (that would defeat the purpose of One class per event ) Something very important that we should not forget is that the events are mainly designed to be used by the logging systems. Those systems should receive accurate data, which is really not the case of the current event_trigger()/add_to_log() function... if we do not put in place a strict structure, we increase the risk of falling back where we suffered serious lack of information.
          Hide
          Dan Poltawski added a comment -

          Thinking more about the suggestion of a requirement for a second queryable object because when i came to implement core_cohort_member_added..

          Using the queryable fields, this is the best I can do with the description:
          'User '.$this->relateduserid.' was added to a cohort by '.$this->userid.' with cohort_members record:'.$this->objectid

          Crucially, its missing the cohort id. Now to me that seems like something that logs will want to query frequently.

          For example - as a admin, I can imagine wanting to query the logs with 'who added 'big bully student' to cohort 'only nice students'.

          Show
          Dan Poltawski added a comment - Thinking more about the suggestion of a requirement for a second queryable object because when i came to implement core_cohort_member_added.. Using the queryable fields, this is the best I can do with the description: 'User '.$this->relateduserid.' was added to a cohort by '.$this->userid.' with cohort_members record:'.$this->objectid Crucially, its missing the cohort id. Now to me that seems like something that logs will want to query frequently. For example - as a admin, I can imagine wanting to query the logs with 'who added 'big bully student' to cohort 'only nice students'.
          Hide
          Petr Škoda added a comment - - edited

          We could add this easily to current code, all we need is a good name for the new field and some strict guidelines for its use - the new event properties would be 'xxx', 'xxxtable' and 'xxxid'. If we do that we should also probably get rid of the automatic filling of $event->object because that would not make much sense any more. The class name would become nearly arbitrary, only the last word would be automatically parsed as action, but then we could eliminate the action completely.

          Show
          Petr Škoda added a comment - - edited We could add this easily to current code, all we need is a good name for the new field and some strict guidelines for its use - the new event properties would be 'xxx', 'xxxtable' and 'xxxid'. If we do that we should also probably get rid of the automatic filling of $event->object because that would not make much sense any more. The class name would become nearly arbitrary, only the last word would be automatically parsed as action, but then we could eliminate the action completely.
          Hide
          Frédéric Massart added a comment -

          Dan, looking at your code a second time, there is something that you shouldn't do. (Petr Škoda, I knew that would happen ). You cannot use the objectid to represent the ID of the entry in a Many to Many table. Here, to bypass the problem you are facing, you should declare:

          • $objecttable = 'cohort'
          • $objectid = The ID of the cohort
          • $relateduserid = The member of the cohort

          The $objecttable property has been added so that you could ignore how the event was name, but still know what the object is referring to. In this case, the objecttable could have been user, but that made only little sense as the relateduserid already holds this information.

          Show
          Frédéric Massart added a comment - Dan, looking at your code a second time, there is something that you shouldn't do. ( Petr Škoda , I knew that would happen ). You cannot use the objectid to represent the ID of the entry in a Many to Many table. Here, to bypass the problem you are facing, you should declare: $objecttable = 'cohort' $objectid = The ID of the cohort $relateduserid = The member of the cohort The $objecttable property has been added so that you could ignore how the event was name, but still know what the object is referring to. In this case, the objecttable could have been user, but that made only little sense as the relateduserid already holds this information.
          Hide
          Dan Poltawski added a comment -

          Fred: thanks, that makes total sense. However, we really do need to find a good way to document this, because I made that mistake.

          Show
          Dan Poltawski added a comment - Fred: thanks, that makes total sense. However, we really do need to find a good way to document this, because I made that mistake.
          Hide
          Frédéric Massart added a comment - - edited

          Yes, I have just had a chat with Petr, and as yeah we should not use object and objecttable, those terms are really confusing!

          I suggest that we rename object to target, and keep objecttable and objectid.

          Examples

          • \core\event\cohort_member_added
            • $target: cohort_member
            • $action: added
            • $objecttable: cohort
            • $objectid: cohort ID
            • $relateduserid: user ID
          • \mod_assign\event\assignment_submitted
            • $target: assignment
            • $action: submitted
            • $objecttable: assign_submission
            • $objectid: ID for entry in assign_submission
            • $relateduserid: user posting the assignment

          target might not be the best word, but it doesn't need to fit all the situations. It needs to be different than object and represents something that is unique to events, more like an event entity.

          Show
          Frédéric Massart added a comment - - edited Yes, I have just had a chat with Petr, and as yeah we should not use object and objecttable , those terms are really confusing! I suggest that we rename object to target , and keep objecttable and objectid . Examples \core\event\cohort_member_added $target: cohort_member $action: added $objecttable: cohort $objectid: cohort ID $relateduserid: user ID \mod_assign\event\assignment_submitted $target: assignment $action: submitted $objecttable: assign_submission $objectid: ID for entry in assign_submission $relateduserid: user posting the assignment target might not be the best word, but it doesn't need to fit all the situations. It needs to be different than object and represents something that is unique to events, more like an event entity.
          Hide
          Petr Škoda added a comment -

          Thanks everybody for the review and feedback, let's move this forward and see if integrators like it. I have rebased it on top on current integration, there are some dependant issues, but the rebasing should not create significant problems for them because they will not be integrated together with this issue and collisions are not expected.

          To integrators: please note the TODOs will be resolved when we start working at new logging infrastructure, the current state should be sufficient for normal event triggering and migration to new event observers. All changes are expected only in the event classes.

          Show
          Petr Škoda added a comment - Thanks everybody for the review and feedback, let's move this forward and see if integrators like it. I have rebased it on top on current integration, there are some dependant issues, but the rebasing should not create significant problems for them because they will not be integrated together with this issue and collisions are not expected. To integrators: please note the TODOs will be resolved when we start working at new logging infrastructure, the current state should be sufficient for normal event triggering and migration to new event observers. All changes are expected only in the event classes.
          Hide
          Frédéric Massart added a comment -

          ^ \o/

          Show
          Frédéric Massart added a comment - ^ \o/
          Hide
          Frédéric Massart added a comment -

          Hi Petr,

          We should change the conditional test for adding to mdl_log. $CFG->loglifetime === 0 means "Save everything forever". As you said, we should add an option "-1" for "Log nothing to mdl_log", and update this code:

                  if (!empty($CFG->loglifetime)) {
                      if ($data = $this->get_legacy_logdata()) {
                          call_user_func_array('add_to_log', $data);
                      }
                  }
          
          Show
          Frédéric Massart added a comment - Hi Petr, We should change the conditional test for adding to mdl_log. $CFG->loglifetime === 0 means "Save everything forever". As you said, we should add an option "-1" for "Log nothing to mdl_log", and update this code: if (!empty($CFG->loglifetime)) { if ($data = $this->get_legacy_logdata()) { call_user_func_array('add_to_log', $data); } }
          Hide
          Petr Škoda added a comment -

          oh, I forgot the legacy logging, fixed now

          Show
          Petr Škoda added a comment - oh, I forgot the legacy logging, fixed now
          Hide
          Dan Poltawski added a comment - - edited

          Petr, I think you've left some legacy code in your tests.

          Debugging: Accessing non-existent event property 'object'
          * line 575 of /lib/classes/event/base.php: call to debugging()
          * line 505 of /lib/tests/accesslib_test.php: call to core\event\base->__get()
          * line ? of unknownfile: call to accesslib_testcase->test_role_assign()
          * line 976 of /vendor/phpunit/phpunit/PHPUnit/Framework/TestCase.php: call to ReflectionMethod->invokeArgs()
          * line 831 of /vendor/phpunit/phpunit/PHPUnit/Framework/TestCase.php: call to PHPUnit_Framework_TestCase->runTest()
          * line 76 of /lib/phpunit/classes/advanced_testcase.php: call to PHPUnit_Framework_TestCase->runBare()
          * line 648 of /vendor/phpunit/phpunit/PHPUnit/Framework/TestResult.php: call to advanced_testcase->runBare()
          * line 776 of /vendor/phpunit/phpunit/PHPUnit/Framework/TestCase.php: call to PHPUnit_Framework_TestResult->run()
          * line 775 of /vendor/phpunit/phpunit/PHPUnit/Framework/TestSuite.php: call to PHPUnit_Framework_TestCase->run()
          * line 745 of /vendor/phpunit/phpunit/PHPUnit/Framework/TestSuite.php: call to PHPUnit_Framework_TestSuite->runTest()
          * line 705 of /vendor/phpunit/phpunit/PHPUnit/Framework/TestSuite.php: call to PHPUnit_Framework_TestSuite->run()
          * line 705 of /vendor/phpunit/phpunit/PHPUnit/Framework/TestSuite.php: call to PHPUnit_Framework_TestSuite->run()
          * line 349 of /vendor/phpunit/phpunit/PHPUnit/TextUI/TestRunner.php: call to PHPUnit_Framework_TestSuite->run()
          * line 176 of /vendor/phpunit/phpunit/PHPUnit/TextUI/Command.php: call to PHPUnit_TextUI_TestRunner->doRun()
          * line 129 of /vendor/phpunit/phpunit/PHPUnit/TextUI/Command.php: call to PHPUnit_TextUI_Command->run()
          * line 63 of /vendor/phpunit/phpunit/composer/bin/phpunit: call to PHPUnit_TextUI_Command::main()
          FDebugging: Accessing non-existent event property 'object'
          * line 575 of /lib/classes/event/base.php: call to debugging()
          * line 553 of /lib/tests/accesslib_test.php: call to core\event\base->__get()
          * line ? of unknownfile: call to accesslib_testcase->test_role_unassign()
          * line 976 of /vendor/phpunit/phpunit/PHPUnit/Framework/TestCase.php: call to ReflectionMethod->invokeArgs()
          * line 831 of /vendor/phpunit/phpunit/PHPUnit/Framework/TestCase.php: call to PHPUnit_Framework_TestCase->runTest()
          * line 76 of /lib/phpunit/classes/advanced_testcase.php: call to PHPUnit_Framework_TestCase->runBare()
          * line 648 of /vendor/phpunit/phpunit/PHPUnit/Framework/TestResult.php: call to advanced_testcase->runBare()
          * line 776 of /vendor/phpunit/phpunit/PHPUnit/Framework/TestCase.php: call to PHPUnit_Framework_TestResult->run()
          * line 775 of /vendor/phpunit/phpunit/PHPUnit/Framework/TestSuite.php: call to PHPUnit_Framework_TestCase->run()
          * line 745 of /vendor/phpunit/phpunit/PHPUnit/Framework/TestSuite.php: call to PHPUnit_Framework_TestSuite->runTest()
          * line 705 of /vendor/phpunit/phpunit/PHPUnit/Framework/TestSuite.php: call to PHPUnit_Framework_TestSuite->run()
          * line 705 of /vendor/phpunit/phpunit/PHPUnit/Framework/TestSuite.php: call to PHPUnit_Framework_TestSuite->run()
          * line 349 of /vendor/phpunit/phpunit/PHPUnit/TextUI/TestRunner.php: call to PHPUnit_Framework_TestSuite->run()
          * line 176 of /vendor/phpunit/phpunit/PHPUnit/TextUI/Command.php: call to PHPUnit_TextUI_TestRunner->doRun()
          * line 129 of /vendor/phpunit/phpunit/PHPUnit/TextUI/Command.php: call to PHPUnit_TextUI_Command->run()
          * line 63 of /vendor/phpunit/phpunit/composer/bin/phpunit: call to PHPUnit_TextUI_Command::main()
          F....................  183 / 2021 (  9%)
          
          Show
          Dan Poltawski added a comment - - edited Petr, I think you've left some legacy code in your tests. Debugging: Accessing non-existent event property 'object' * line 575 of /lib/classes/event/base.php: call to debugging() * line 505 of /lib/tests/accesslib_test.php: call to core\event\base->__get() * line ? of unknownfile: call to accesslib_testcase->test_role_assign() * line 976 of /vendor/phpunit/phpunit/PHPUnit/Framework/TestCase.php: call to ReflectionMethod->invokeArgs() * line 831 of /vendor/phpunit/phpunit/PHPUnit/Framework/TestCase.php: call to PHPUnit_Framework_TestCase->runTest() * line 76 of /lib/phpunit/classes/advanced_testcase.php: call to PHPUnit_Framework_TestCase->runBare() * line 648 of /vendor/phpunit/phpunit/PHPUnit/Framework/TestResult.php: call to advanced_testcase->runBare() * line 776 of /vendor/phpunit/phpunit/PHPUnit/Framework/TestCase.php: call to PHPUnit_Framework_TestResult->run() * line 775 of /vendor/phpunit/phpunit/PHPUnit/Framework/TestSuite.php: call to PHPUnit_Framework_TestCase->run() * line 745 of /vendor/phpunit/phpunit/PHPUnit/Framework/TestSuite.php: call to PHPUnit_Framework_TestSuite->runTest() * line 705 of /vendor/phpunit/phpunit/PHPUnit/Framework/TestSuite.php: call to PHPUnit_Framework_TestSuite->run() * line 705 of /vendor/phpunit/phpunit/PHPUnit/Framework/TestSuite.php: call to PHPUnit_Framework_TestSuite->run() * line 349 of /vendor/phpunit/phpunit/PHPUnit/TextUI/TestRunner.php: call to PHPUnit_Framework_TestSuite->run() * line 176 of /vendor/phpunit/phpunit/PHPUnit/TextUI/Command.php: call to PHPUnit_TextUI_TestRunner->doRun() * line 129 of /vendor/phpunit/phpunit/PHPUnit/TextUI/Command.php: call to PHPUnit_TextUI_Command->run() * line 63 of /vendor/phpunit/phpunit/composer/bin/phpunit: call to PHPUnit_TextUI_Command::main() FDebugging: Accessing non-existent event property 'object' * line 575 of /lib/classes/event/base.php: call to debugging() * line 553 of /lib/tests/accesslib_test.php: call to core\event\base->__get() * line ? of unknownfile: call to accesslib_testcase->test_role_unassign() * line 976 of /vendor/phpunit/phpunit/PHPUnit/Framework/TestCase.php: call to ReflectionMethod->invokeArgs() * line 831 of /vendor/phpunit/phpunit/PHPUnit/Framework/TestCase.php: call to PHPUnit_Framework_TestCase->runTest() * line 76 of /lib/phpunit/classes/advanced_testcase.php: call to PHPUnit_Framework_TestCase->runBare() * line 648 of /vendor/phpunit/phpunit/PHPUnit/Framework/TestResult.php: call to advanced_testcase->runBare() * line 776 of /vendor/phpunit/phpunit/PHPUnit/Framework/TestCase.php: call to PHPUnit_Framework_TestResult->run() * line 775 of /vendor/phpunit/phpunit/PHPUnit/Framework/TestSuite.php: call to PHPUnit_Framework_TestCase->run() * line 745 of /vendor/phpunit/phpunit/PHPUnit/Framework/TestSuite.php: call to PHPUnit_Framework_TestSuite->runTest() * line 705 of /vendor/phpunit/phpunit/PHPUnit/Framework/TestSuite.php: call to PHPUnit_Framework_TestSuite->run() * line 705 of /vendor/phpunit/phpunit/PHPUnit/Framework/TestSuite.php: call to PHPUnit_Framework_TestSuite->run() * line 349 of /vendor/phpunit/phpunit/PHPUnit/TextUI/TestRunner.php: call to PHPUnit_Framework_TestSuite->run() * line 176 of /vendor/phpunit/phpunit/PHPUnit/TextUI/Command.php: call to PHPUnit_TextUI_TestRunner->doRun() * line 129 of /vendor/phpunit/phpunit/PHPUnit/TextUI/Command.php: call to PHPUnit_TextUI_Command->run() * line 63 of /vendor/phpunit/phpunit/composer/bin/phpunit: call to PHPUnit_TextUI_Command::main() F.................... 183 / 2021 ( 9%)
          Hide
          Petr Škoda added a comment -

          grrrr, fixed, sorrrry

          Show
          Petr Škoda added a comment - grrrr, fixed, sorrrry
          Hide
          Petr Škoda added a comment -

          Rebased and added:

          1. snapshot handling is completely locked down - no code changes allowed
          2. all legacy data is protected, this should prevent accidental use outside of trigger() - this might complicate things if we decide to add BC unit tests, but in the long term it could prevent major coding style problems, we want to get rid of all legacy stuff eventually, right?
          Show
          Petr Škoda added a comment - Rebased and added: snapshot handling is completely locked down - no code changes allowed all legacy data is protected, this should prevent accidental use outside of trigger() - this might complicate things if we decide to add BC unit tests, but in the long term it could prevent major coding style problems, we want to get rid of all legacy stuff eventually, right?
          Hide
          Petr Škoda added a comment -

          if necessary I could add $this->assertLegacyEventDataEquals($expected, $event) and hack around the protected stuff there

          Show
          Petr Škoda added a comment - if necessary I could add $this->assertLegacyEventDataEquals($expected, $event) and hack around the protected stuff there
          Hide
          Ankit Agarwal added a comment -

          While working on blog events, I came across a few more situations:-

          1. How should be can_view() defined for a delete action? Say for a course delete.
          2. For other than delete events, I guess we should ideally call a core API to get the value for can_view(). However that api might depend on other surrounding entities which might not be present during an event restore. Say for example there is a event for when a tag is added to blog. After that someone deletes the blog. Later on when the event is restored, and can_view() is triggered, it won't find the blog and throw a DB exception or something similar. A solution to this can be using try catch always inside can_view(). Not sure if that is the most ideal solution though.
          Show
          Ankit Agarwal added a comment - While working on blog events, I came across a few more situations:- How should be can_view() defined for a delete action? Say for a course delete. For other than delete events, I guess we should ideally call a core API to get the value for can_view(). However that api might depend on other surrounding entities which might not be present during an event restore. Say for example there is a event for when a tag is added to blog. After that someone deletes the blog. Later on when the event is restored, and can_view() is triggered, it won't find the blog and throw a DB exception or something similar. A solution to this can be using try catch always inside can_view(). Not sure if that is the most ideal solution though.
          Hide
          Frédéric Massart added a comment -

          Re can_view(), this is what I posted on the user_loggedin event issue:

          My personal opinion is to delay the can_view() implementation to later. Not for this issue only, but for all the new events until we decide on how we will use the restored event within the logging system, and what we are expecting from it. I appears that we might need to pass a context to can_view() to check if the user has capability in the context needed.

          A teacher might be able to see when the user last logged in, but a teacher would often have no permissions on a site context. What about a parent that wants to see when was the last login of his children? Should we have flexible reports like that? And as you pointed out, what happens when the object is gone, if the user is deleted. The event should still be valid and its data accessible to the right people. Also, ideally, if I had the capability to view something, but then the role is revoked, why shouldn't I be able to access the logs I was seeing before?

          Show
          Frédéric Massart added a comment - Re can_view(), this is what I posted on the user_loggedin event issue: My personal opinion is to delay the can_view() implementation to later. Not for this issue only, but for all the new events until we decide on how we will use the restored event within the logging system, and what we are expecting from it. I appears that we might need to pass a context to can_view() to check if the user has capability in the context needed. A teacher might be able to see when the user last logged in, but a teacher would often have no permissions on a site context. What about a parent that wants to see when was the last login of his children? Should we have flexible reports like that? And as you pointed out, what happens when the object is gone, if the user is deleted. The event should still be valid and its data accessible to the right people. Also, ideally, if I had the capability to view something, but then the role is revoked, why shouldn't I be able to access the logs I was seeing before?
          Hide
          Petr Škoda added a comment -

          course_deleted - we should store parent category in $data['other'] somewhere and use course:view capability in that context

          The general logic is to use parent context if it exists. We could also invent a new capability to "view all logs" for cases where we can not find out where the thing lived any more.

          Show
          Petr Škoda added a comment - course_deleted - we should store parent category in $data ['other'] somewhere and use course:view capability in that context The general logic is to use parent context if it exists. We could also invent a new capability to "view all logs" for cases where we can not find out where the thing lived any more.
          Hide
          Damyon Wiese added a comment -

          Thanks Petr,

          Some comments from looking at the code:

          In \core\event\base:

          get_name could be implemented to look for a lang string based on the classname/component and if it doesn't exist return the default non-human name. That would save 1000 implementations of get_name() with just "return get_string('blah', ...)"

          I tend to agree with the suggestions above that it would be cleaner to move all the code from "\core\event\base\trigger()" to \core\event\manager\dispatch(). However I don't see how to keep the the legacy data private in a nice way if we do that. It's a small improvement so this is probably just as good.

          Finally - I think trigger_event should be deprecated as soon as all uses of it have been converted to the new system in core (This is not listed in the design doc). We should not ship 2.6 with 2 event systems in place.

          Re: when to implement "can_view" - IMO this should be based on the permissions at the time someone is viewing the logs. E.g. if a parent is added to a class they should be able to see their child logs for as long as the child has been enrolled. Similarly, if someone has their role removed, it is not expected for them to be able to still see logs of things they don't have access to. If something has been deleted at the time the logs are viewed - they should see something like "User deleted (342)".

          I don't see any blockers here - so I've integrated this to master only.

          This is a big patch - thanks for the hard work everyone.

          Show
          Damyon Wiese added a comment - Thanks Petr, Some comments from looking at the code: In \core\event\base: get_name could be implemented to look for a lang string based on the classname/component and if it doesn't exist return the default non-human name. That would save 1000 implementations of get_name() with just "return get_string('blah', ...)" I tend to agree with the suggestions above that it would be cleaner to move all the code from "\core\event\base\trigger()" to \core\event\manager\dispatch(). However I don't see how to keep the the legacy data private in a nice way if we do that. It's a small improvement so this is probably just as good. Finally - I think trigger_event should be deprecated as soon as all uses of it have been converted to the new system in core (This is not listed in the design doc). We should not ship 2.6 with 2 event systems in place. Re: when to implement "can_view" - IMO this should be based on the permissions at the time someone is viewing the logs. E.g. if a parent is added to a class they should be able to see their child logs for as long as the child has been enrolled. Similarly, if someone has their role removed, it is not expected for them to be able to still see logs of things they don't have access to. If something has been deleted at the time the logs are viewed - they should see something like "User deleted (342)". I don't see any blockers here - so I've integrated this to master only. This is a big patch - thanks for the hard work everyone.
          Hide
          David Monllaó added a comment -

          Passing it, phpunit running in CI server

          Show
          David Monllaó added a comment - Passing it, phpunit running in CI server
          Hide
          Ankit Agarwal added a comment -

          +1 for "view all logs" cap.

          Show
          Ankit Agarwal added a comment - +1 for "view all logs" cap.
          Hide
          Damyon Wiese added a comment -

          Thanks again for another week of fixes, improvements and testing. These changes have been released to the world.

          Cheers, Damyon

          Show
          Damyon Wiese added a comment - Thanks again for another week of fixes, improvements and testing. These changes have been released to the world. Cheers, Damyon
          Hide
          Eloy Lafuente (stronk7) added a comment -

          The phpunit event redirection facilities need documenting for sure, adding the dev_docs_required label.

          Show
          Eloy Lafuente (stronk7) added a comment - The phpunit event redirection facilities need documenting for sure, adding the dev_docs_required label.

            People

            • Votes:
              0 Vote for this issue
              Watchers:
              9 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Agile