General developer forum

 
 
My ugly mug
Feedback requested on new Events system specification
Group DevelopersGroup Moodle Course Creator Certificate holdersGroup Moodle HQGroup Particularly helpful MoodlersGroup Testers

Hi, all devs.

The Moodle HQ Back-end team is specifying a new approach to logging. As an initial part of this, the team has been redesigning the Events system and has a draft specification for Events ready for public comment.

http://docs.moodle.org/dev/Event_2

Your feedback is welcomed.

To add comments, questions and suggestions, please respond to this forum post.

 
Average of ratings: -
Picture of Amanda Doughty
Re: Feedback requested on new Events system specification
Group Developers

This is great news and something we have been talking to Moodlerooms about, in connection with developments we are collaborating on. It will pave the way for improved communication/notifications.

 
Average of ratings: -
Tim at Lone Pine Koala Sanctuary
Re: Feedback requested on new Events system specification
Group DevelopersGroup Documentation writersGroup Particularly helpful Moodlers

I fear that I am struggling to get past the "Why is a new events system needed?" section. You have not sold this to me at all.

You may well have had extensive discussions about this within Moodle HQ, but if so you need to tell us about it if you want us to understand any comment intelligently on your proposal.

  • "The events need to be more strictly defined." - they do? why?
  • "It will be possible to subscribe to '*' event" - You could do that with a trivial change to the current system.
  • "Potential duplication of events and logging actions" - well, that would be a problem for the design of the logging system. You need to explain more if you want to use this as a motivation for re-implementing events in Moodle.

The Performance section is also pretty weak. Does "There is a general plan to complete pre- and post-implementation testing as development happens." mean that you will do performance and load-testing before any new code is integrated? If not, how can you be sure that you are not trashing Moodle 2.6 performance?

"Some basic profiling has been conducted" Sounds good, but we are meant to be (software) engineers. Please outline what you did and what the results were, otherwise it is just words.

In Events API the statement "Each plugin will define the events that it can report (trigger) by extending an abstract base class" seems to be rather at odds with the reason for change that events "need to contain a lot more information in a standardised way."

Maintainability & Self documenting seem to be properties of the current events system. If you claim that the new system is better, then you will have to explain in what way.

Naming: Do you think $int is a good variable name? Of course not. It should be called something like $priority. So, calling part of the API 'callable' is poor. I suggest 'handler'. 'internal' should be true/false, not 0/1.

"Developers of observers must make sure that execution does not end with a fatal error under any condition" - you can do that, in PHP?!

"A list of available observers is constructed on the fly directly from all available events.php files. It is no longer parsed during upgrade or install." - I hope this is one place where you did some preliminary performance-testing.

"Prevents abuse of cron events" what do you mean by 'abuse'? Cron events are used. 'abuse' is a rather emotive word. If you mean "our new system will not provide one feature of the old system" then please explain why that is not an issue. Simply describing the existing uses as abuse, without explanation, is the kind of thing a politician would do to justify their proposed change. It is unbecoming for a software developer to do so.

$event->trigger(). A better design for this would be event_manager()->trigger($event). (The purpose of $event is to store information about the event that happened. The purpose of core_event_manager (instance returned by event_manager()) is to manage the process of event dispatching. We don't want different event types to be dispatched in different ways.)

See above for my criticism of "Each event class name is a unique identifier of the event."

Decision: Use separate class for each event

The kind of API where every use of the API requies defining a new types of class really sucks. "Prefer aggregation over inheritance". I can see the need for a few different event classes, like the way JavaScript has key event and mouse event classes that extend the basic DOM event class, but those three classes between them cover tens of different actual events.

(An example of where having to define a class really sucks is in the admin_setting_configselect class. If you want to use the lazy-loading facilities of that class, the only option is to subclass it. There are better designs.)

At least you have tried to explain your thinking here, except that you have not really. You claim "Maintainability - It is much easier to review, debug, integrate." Well, that is your assertion. My assertion is that "Each plugin defines events in a list based on a generic object" is easier to review, debug, integrate. Which of us is right?

"extreme flexible for plugin developers" strikes me as something that

  • is hard to "review, debug, integrate" - for example, how can you verify that the system does not have security holes?
  • makes "implementation of an events infrastructure ... significantly more complex and error prone"

Conversely, "Each plugin defines events in a list based on a generic object" is flexible enough. (you say it is not, but I don't believe you. Give me one specific example to prove your point.) I think I disagree with all your "Cons" for this decision.

Why do you think "All data would have to be calculated even if it is not used."?

Why does it make it harder to integrate legacy logging?

Why does it make supprorting legacy events harder or slower?

Why do you even want to use get_class_name($event) for anything. $event->name() looks like a better API to me.

Upgrade/install would not be necessary to register event. Why do you think that? Ah, because you think it "Would need to be maintained through DB tables." Why? No it isn't. When someone tries to create a core_course_view event, then we check the name against the appropriate events.php file. No DB table involved.

Wow! I am only 1/4 of the way thorugh the proposal so far. I will have to continue later. (Note to self, pick up at Event properties heading.)

 
Average of ratings: -
Picture of Petr Škoda
Re: Feedback requested on new Events system specification
Group DevelopersGroup Documentation writersGroup Moodle HQGroup Particularly helpful Moodlers

Why is a new events system needed?

I have updated this section, thanks.

Performance

No measurements were done because there is no code yet. The new events would also contain more data/functionality, and a lot more events would be triggered. We would need to measure the total performance gain/loss for events and logging at the same time. I suppose it is going to be slower in the end, but there would be a lot more flexibility and predictability which may be itself worth something.

Events API

Base class would define mandatory list of properties and preset some values and do basic data validation. Plugin event classes implement backwards compatibility for old logging and old events, they set all required information and store one custom data field. They might also contain other functionality such as access control, rendering for reports, etc.

We have tried to explain/describe it in the spec, I agree it may not be clear. I suppose you could help us to explain the benefits there using your perfect English language skills and OOP knowledge.

Naming

"Callable" is a type hint in PHP 5.4. It is used in PHP manual when describing callbacks, so "callback" is probably better, changing it. Observer or handler is problematic because the whole array is observer, your suggestion is worse because observer->observer is poor design - it must indicate that devs need to put function name or class::method there. Let's not start beating the PHP type system, we all know that 1/0 or true/false makes no difference - fixed in examples.

The current event system uses only event name and arbitrary event data, the event data is not documented much, it is doing database queries - pretty much anything else must be better, right?

"Developers of observers must make sure that execution does not end with a fatal error under any condition" - Current event handling fails badly if handler triggers exception, which happens during upgrades already. Your question about fatal errors is strange. We had that problem already, the difference is when observer gets execute before plugin installation (which makes it actually a bit more usable). Missing DB tables cause exceptions, not fatal errors - so it should not be a problem.

The list of handlers was fetched from database or MUC - it is both slow and inaccurate at the moment. Yes, it will be faster if we fix&improve the current listing of all plugins and then store the results only in MUC or in simple php file in dataroot. It must be faster and more accurate.

Cron event processing must not be time confusing, we already have enough task there, so yes they are abused in portfolio. The data objects are often out of data, such as in file instances in assignment cron events. Cron events were not supposed to be used for moving of time consuming tasks form current page to cron, they were intended as a fallback when handlers for some reasons failed event processing - this whole concept did not work much and was causing performance issues.

You are wrong, event_manager()->trigger($event) would be far worse design because we would have to open public api in event base class, $event->trigger() allows us to lock down the event so that it is not triggered twice or altered after triggering - it must stay unchanged because of the new execution order and buffering, most of the things are protected and they are supposed to be modified from init() method only. This is actually the main reason for extending of base class.

Decision: Use separate class for each event

Please watch your language, porn stars suck, class design does not. Inheritance seems to be perfectly valid for events where we want to enforce the structure and let extending class only add extra behaviour and legacy data. Extending in this particular case is in my opinion less error-prone. Your objections are just a general OOP talk, show me some more if you want to persuade me.

Reviews - you look at 1 file only and you see everything related to one particular event, you do not need to grep the codebase and verify where it is used to make sure thad developers set legacy logging/events everywhere, that they filled the same levels and crud everywhere, etc. Integration - integrators would know that any changes in event classes must be closed watched for BC issues. Again your objections are too general, you do not give any valid reasons to do it some other way. You also did not describe how you would do it, how am I supposed to argue with "generic object" approach? Nobody explained how that could possibly work yet.

"All data would have to be calculated even if it is not used."?

Some event properties are optional, they may not be needed at all when triggering it - legacy logs, legacy events, affected users, access control, etc. These are exposed via methods that might have to event access database to get the values. Of course developers may decide to store them as property in their init method. The point here is that methods give us room to improve performance here, the general objects do not, sorry.

Using class names as event names prevents typos or hacking with event names, the more we validate in PHP itself the faster it will be (this validates file location, file name and class name for free if you ignore class loader cost). But yes, we would have $event->name() too, it would always return the class name.

The available events should be documented somewhere, somebody proposed to register the events somehow, the problem is when you get the raw data from logs and you want to create the original instance of the event, the class loader with frankenstyle makes this trivial.

I suppose once you stop opposing the "single frankenstyle class per event extending base" concept you might realise that it magically solves many problems we would face otherwise. My code demo linked from the spec hopefully proves that.

Thanks for your comments!!!

 
Average of ratings: -
Tim at Lone Pine Koala Sanctuary
Re: Feedback requested on new Events system specification
Group DevelopersGroup Documentation writersGroup Particularly helpful Moodlers

The key design question is one class per type of event raised, or not, so let us focus discussion there for now.

I thought of a good parallel: exceptions. They are not exactly the same as events, but they are something everyone is familiar with, and they tend to work the same way in many different computer languages, which is a strong indication that the design is good.

In Moodle, we have quite a lot of different exception classes, where that makes sense.

We do not require that every single place where you might want to throw an exception, you have to define a new class. Can you imaging how much of a pain that would be?

If you understand the basics of moodle_exception and coding_exception, your probably understand most of the throw commands in Moodle. It is much harder if each separate throw command required a different subclass. Much more code for other developers to read.

As I say, exceptions are not a perfect analogy, but this is worth considering.

There are other ways I could try to explain this, but not now.

 

I would like to note that the syntax is

throw new coding_exception(...);

not

coding_exception::create(...)->throw();

 

The point for me is that an event object should just be a package of data. You seem to thinking the same way when you say "New event data must not contain any PHP classes except stdClass.", and talk about serialising, and other good parts of the events spec.

But with the sub-classing, and having methods like ->trigger(), you mixing behaviour into the event types. As a results, the events system has to be built with the assumptions that different events can behave completely differently. As a result, it will be very hard to do systematic optimisations.

The alternative point of view is that we define an events system where all events behave in fundamentally the same way. This is much easier for developers to understand, and with all events behaving the same way, it is much easier to understand what optimisations of the events system might be possible to implement without breaking things.

 
Average of ratings: -
Picture of Petr Škoda
Re: Feedback requested on new Events system specification
Group DevelopersGroup Documentation writersGroup Moodle HQGroup Particularly helpful Moodlers
Events are not exceptions, your analogies are misleading, sorry.
 
Average of ratings: -
Tim at Lone Pine Koala Sanctuary
Re: Feedback requested on new Events system specification
Group DevelopersGroup Documentation writersGroup Particularly helpful Moodlers

Well, that dismisses one of my points. Are you going to address the others?

 

You are the person (people) who are proposing to to change Moodle in a certain way, why are you expecting me to do all the work explaining things? Shouldn't it be your responsibility to explain to us why you are right?

 

Please tell me at least one thing that Moodle events are like. (Or are you claiming that you are building something with no precedent? Are you trying to claim that would be a wise thing to do?)

 

I point I made before is DOM events. Would you say Moodle events have some things in common with DOM events? (If not, why not?) http://www.w3.org/TR/DOM-Level-2-Events/events.html For DOM events, there are many different types of event like click, mousedown, mouse up, ... that are all instances of the MouseEvent class. That seems to be a good design that works. Have you ever seen anyone arguing that design was a mistake?

 
Average of ratings:Useful (1)
Picture of Petr Škoda
Re: Feedback requested on new Events system specification
Group DevelopersGroup Documentation writersGroup Moodle HQGroup Particularly helpful Moodlers
I tried to address all your points above, I have also tried to improve the spec in the meantime. The proposed solution seems to address all potential and existing problems we could imagine while working on the spec. I agree that the core_event_base class needs some more work, but that should not hopefully prevent discussions about the general design of new events.

This events proposal is an evolution of current events infrastructure and current logging subsystem. I have implemented the current eventlib.php code and maintained it, the original spec was created by multiple authors. I have also used the events extensively in enrolment plugins. I was working on reports maintenance for some time and the migration to new DML API. I was involved in various logging performance improvementns including the first rewrite of stats SQL.

I hope this gives me enough credit to say that there are problems in current events and log design that can not be resolved in existing code/design. The proposal was created by the whole backend team at HQ, the migration of logging to events was proposed by Moodlerooms during the hackfest last year, I did not hear any valid concerns at that time.

My personal experience is that precedents may or may not help. Sometimes you need to roll your own solution that solves exactly the problems you are facing: logging, many new events for everybody, more useful events, 100 % backwards compatibility, less error-prone.

I am not going to discuss any analogies here, sorry.
 
Average of ratings: -
ME
Re: Feedback requested on new Events system specification
Group Particularly helpful Moodlers

I would love to see some use cases that the new api would cover and which ones it would duplicate from the old api. From the spec I understand the logging use case but I am unclear of other advanced use cases. I assume from the backwards compatibility section all existing core events will continue to be supported.

From Amanda's posts and discussions with her I know she wants improvements to the data provided to event consumers by the event creator. I can't tell from a use case stand point if all of the event consumers she is looking to create have been considered and are part of the design or not.

So for example is there a use case for a course update event being consumed by a user alert system compared to say course completion processing? In general what event consumers has Moodle HQ designed for and what data do they need? From the spec all I see is logging.

The need for a new events api is pretty clear to me from what we have tried to do with the existing one and where the lack of data fails.

We currently have two event consumers:

1. User alerts
2. Course automation beyond just course completion.

Any time we use an event to display information to a user the lack of data surrounding the event is what kills the usability.

 
Average of ratings: -
Picture of Petr Škoda
Re: Feedback requested on new Events system specification
Group DevelopersGroup Documentation writersGroup Moodle HQGroup Particularly helpful Moodlers

Use cases:

  • logs
  • anything similar to course completion - the new event would tell you what happens in a more general way - if there is some standard dictionary of event words, you might implement actions based on completion of general activity instead of adding hacks to each plugin separately
  • gradebook history - we could send all grade changes through events and implement a gradebook logger on top of current DB tables
  • custom logging system that intercepts all events
  • we could trigger events during installation and upgrades (plugin installed, updated, etc.)
  • you could filter events by affected users and base notification on that

The more defined data is there in events the more we can do with it, that is the reason why there are so many public properties and methods in the base event class. It makes it slower, yes, but a lot more useful for anything. Logging should be a proof that it works.

 
Average of ratings: -
Mark Nielsen in Prague, Czech Republic - 2006
Re: Feedback requested on new Events system specification
Group DevelopersGroup Particularly helpful Moodlers

First, love the move to "on the fly creation" with the option to cache it.

The specification does pose a few concerns though:

  1. I think this was already brought up, but I'd like to say it again anyways. I think that one event class per event is not the way to go. EG: I may fire off an event that has the exact same event data in multiple locations, but the only thing that changes is the event name. Defining a new class just to change the event name doesn't make sense to me.
  2. The proposed API appears to have the event dispatching (AKA triggering) itself. This seems like a lack of separation of concerns. The event should just be information about the event being triggered. The actual triggering of the event and passing the event to the observers should be done by an event dispatcher.
  3. The option for a observer to listen to all events (EG: that asterisk option) seems problematic and the only use case I can image is for logging, but even that breaks down. I think it breaks down because I don't see that every single event is worth logging. For example I may actually want to use the observer pattern to extend classes. If I do this, then I wouldn't want to log that I looped though a 1000 widgets in order to modify some widget property via an observer.
  4. The base event class appears to have far too much going on in it. Some of that has to do with that it is dispatching itself and some has to do with adding specific support for logging. I already mentioned that the dispatching code should be moved out and I think the logging code should move to an interface. If an event implements the "loggable event" interface, then the event dispatcher could technically wrap it in another event and re-fire it as a single log event. Then any logging observers can listen to the one event and log anything that comes through.

In the end, I think some of the problems I laid out above are because this new API is trying to be an event API and a logging API all in one. Really, it should be an event API that the logging API can listen to in order to automatically generate logs. Both, should be able to be used independently.

Lastly, I'd like to point out an alternative solution that addresses all of my above concerns and several from the specification: the Symfony Event Dispatcher component. This is NOT the full Symfony framework, it is one of its individual, decoupled, building blocks designed for one task. Interestingly enough, Symfony just released their first LTS release which means Symfony 2.3 and its components will be supported for the next 3 years. If anything, you can use this for inspiration or better, give it a try as I think it could be used without even making it a hard dependency for Moodle (EG: you could swap it out later if needed).

Cheers, Mark

 
Average of ratings:Useful (5)
Tim at Lone Pine Koala Sanctuary
Re: Feedback requested on new Events system specification
Group DevelopersGroup Documentation writersGroup Particularly helpful Moodlers

Re 3: Whatever event classes we end up with, presumably there will be some class hierarchy (or interface hierarchy). So, you let us suppose we have some interfaces / base classes like quiz_event extends activity_event extends course_related_event extends base_event. There might also be a grade_received_event type. Submitting a quiz would be both a mod_quiz_event and a grade_received_event (which is why these might be better as interfaces).

Anyway if that was the case, then it would make more sense to let people subscribe to an event type like all events (base_event), or all quiz events (mod_quiz_event) or all grade_received_events. Not sure that actually solves all our problems, but worth considering.

Also (and I remember that Petr does not like analogies, but I still think that they are a powerful explanatory and educational technique) I would point out that is how exception handling works. You do catch (Exception $e) {...} or catch (coding_exception $e) {...}.

+1 for Mark's other comments too.

 
Average of ratings: -
Picture of Petr Škoda
Re: Feedback requested on new Events system specification
Group DevelopersGroup Documentation writersGroup Moodle HQGroup Particularly helpful Moodlers
Hello, thanks for the feedback.

1. One event is not supposed to be triggered in different locations, the events are not general. Defining one class per event is the way to go. It is a bit more work for developers, but it helps to enforce new rules and it resolves many potential problems.

2. The triggering details are encapsulated inside the event because it helps with keeping everything under control - it is extremely important to prevent developers from interfering with event processing both in event classes and observers. Your proposal would require opening of public API which would lead to many potential problems (modification of event data after triggering, repeated triggering, etc.) .

3. Listening to all events is an important features. Any other group filtering would require more processing power. Observers are not intended for extending of any classes. This is a notification framework only - one way communication, observers must not modify the event data.

4. I agree we need to keep the base class as small as possible, we must make sure it performs well. Extra validation should be done in developer mode only.

 
Average of ratings: -
Tim at Lone Pine Koala Sanctuary
Re: Feedback requested on new Events system specification
Group DevelopersGroup Documentation writersGroup Particularly helpful Moodlers

I still don't understand 2.

"2. The triggering details are encapsulated inside the event because it helps with keeping everything under control - it is extremely important to prevent developers from interfering with event processing both in event classes and observers. Your proposal would require opening of public API which would lead to many potential problems (modification of event data after triggering, repeated triggering, etc.)"

If the triggering details are encapuslated in the event, then it does not "keep everything under control". It does the opposite. You have absolutely no controll over how different events are dispatched. If there is a single function event_dispatcher::trigger($event) then how event dispatching works is completely under control.

So, what you are propsing suffers from all the potential problems you wish to prevent "(modification of event data after triggering, repeated triggering, etc.)". What we are proposing, if implemented properly, suffers from none of those issues.

 
Average of ratings: -
Picture of Petr Škoda
Re: Feedback requested on new Events system specification
Group DevelopersGroup Documentation writersGroup Moodle HQGroup Particularly helpful Moodlers
Under control === private properties and final methods can do that. Developers can not modify it in any way. Your trigger($event) requires multiple public methods that can be used in wrong or unexpected ways.

In any case the $event->trigger() is fundamental for the legacy event triggering and legacy logging support (see the demo implementation I did). trigger($event) would have to call $event->trigger() anyway to get the legacy stuff done.

If anybody disagrees with $event->trigger() please show us the code and API that would do the trick, but I personally doubt it would be doing better job.

The proposed events+logging design consists of multiple parts, changing of one part (triggering, observers or 1 class per file) would probably break other parts or introduce new unexpected problems. If you want to change something there it is your responsibility to prove it does not break the rest of the proposal.

In any case thanks a lot for your feedback, I agree this needs more discussion.




 
Average of ratings: -
Tim at Lone Pine Koala Sanctuary
Re: Feedback requested on new Events system specification
Group DevelopersGroup Documentation writersGroup Particularly helpful Moodlers

I don't uderstand the "Under control === private properties". Another example is contexts. Of course, the internals of contexts need to be kept under control, and they are. We have public read-only properties $context->contextlevel and $context->instanceid, etc.

 
Average of ratings: -
Picture of Petr Škoda
Re: Feedback requested on new Events system specification
Group DevelopersGroup Documentation writersGroup Moodle HQGroup Particularly helpful Moodlers
Events are not contexts.

event->trigger() does a lot more than just triggering of the event (see my sample code), clearly external function or method trigger($event) can not do that because it does not have access to protected and private properties.
 
Average of ratings: -
Picture of Hubert Chathi
Re: Feedback requested on new Events system specification
 

re: control, you could have something like $event->setTriggered() (terrible name, can't think of anything better) that does validation and sets the triggered flag (and throws an error if the triggered flag is already set).  So essentially, $event->setTriggered() would be the first few lines of the proposed $event->trigger().  Then the event manager would call $event->setTriggered() before actually triggering the event.

Splitting $event->trigger() up like this would allow for more unit testing.  For example, the current proposal can't test validation without triggering the event.  You also can't test that the event is unchangeable after the trigger flag is set, without triggering the event.  Allowing for other event managers will also allow for testing event handlers individually, without calling the other event handlers.

re: logging/legacy events, why is that part of the event class and not the event manager?

 
Average of ratings: -
Picture of Petr Škoda
Re: Feedback requested on new Events system specification
Group DevelopersGroup Documentation writersGroup Moodle HQGroup Particularly helpful Moodlers
Hmm, there would be a need for $event->soon_to_be_triggered() and $event->event_was_triggered() and also you would need some internal properties to verify the methods are called in correct order and only from the event dispatcher. The event might be also "unserialised" from the log storage which need to block any triggering.

The $event->trigger() needs a lot less lines of code, the public API is much smaller and it would be less error-prone. I really do not see any benefit in trigger($event), there seem to be only more problems there.

Why all legacy support only in base class?
1/ I think it is better to have all legacy code in one class only instead of multiple parts with necessary public APIs
2/ we can make the api protected, invisible outside of events, that means we can remove it in future completely without breaking public API
3/ it may be faster
 
Average of ratings: -
Mark Nielsen in Prague, Czech Republic - 2006
Re: Feedback requested on new Events system specification
Group DevelopersGroup Particularly helpful Moodlers

Hey Petr, thanks for the quick reply.  I'm going to try to change your mind ;)

  1. I would say that events are general, in that the data associated to them could often be exactly the same.  The only difference is the name.  For example, assignment submission event would be the same data (I would think) but the name would be submission_updated, submission_created and submission_deleted (or w/e they are).  Maybe I'm not seeing something here, could you point in the spec where this would break down?
  2. The triggering does not need to be inside the event to protect properties because the event class itself can do this by only providing getters for readonly properties.  I think observers modifying event data is a highly valuable use case.  EG: you could provide inexpensive hooks into core processes that could change the behavior of Moodle.  This would be very valuable for partners as they could customize key parts of Moodle without having to patch it.  This makes upgrades much easier.  This is how modern frameworks stay thin yet flexible.
  3. I feel like at some point, there would be some filtering at one end or the other (EG: in the observer).  I was literally thinking of something like this in the event triggering code: if $event instanceof loggable_event_interface then create new log event and fire it.  A double hop, but not really an expensive double hop AFAIK.  I think Tim's idea is also interesting, but not sure how to do that without it being a double hop for every event fired.
  4. Cool!

In the end, I feel like the current design has some limitations for usage in other use cases.  It seems like what is being built is not a event system, but a logging system that we can subscribe to.  Perhaps that's the goal, but I feel like this is an opportunity to get a great event framework in place that has the ability to be used in a wide variety of use cases.

Thanks for listening, Cheers, Mark.

 
Average of ratings: -
Picture of Petr Škoda
Re: Feedback requested on new Events system specification
Group DevelopersGroup Documentation writersGroup Moodle HQGroup Particularly helpful Moodlers
1. No, events are unique in multiple ways, such as access control, string description, documentation, parameters, legacy log actions, legacy events, crud types. Setting everything when triggering the event is error-prone and requires extra work. Imagine integrators would have to grep codebase to see if developers set proper create/read/update/delete flag, proper legacy logging and events etc. The whole idea is to centralise all information about one event in one place and supply minimal information when triggering the event. Another huge benefit is that the classes serve as the source of documentation that can not get out of sync.

Also the legacy support is implemented as methods, not properties - to make it fast you need to override the methods that construct the legacy info from new data. It will be a perf waste to construct the old log and event data once we turn them off (which may be pretty soon because there should be settings for that).

2. Why make it simple when you can create something beautifully complex? I very strongly disagree that anybody should be able to alter low level details of event triggering or dispatching. It has to be reliable and predictable, there is very little code involved.

The only important public API is that event can be triggered and observers receive it, everything in-between can be completely hidden, there is no reason to open it up in my opinion.

The new class loader would allow you to inject your own event base class or event manager if you really wanted to without core modification, having $event->trigger() would actually simplify your hacking because you would have to replace only one of them.

3. I disagree, there will be always new creative ways to filter events, if we try to implement them all it might get slower and slower for everybody - something would have to make sure there is no filtering required. If you use '*' handler only your servers would be affected, but I suppose it would not be slower than shared standard filtering. Also we would have to invent some creative way to describe filters and then interpret it in event dispatcher, but the fastest way for general filtering will be always actual PHP code in your observer, so I guess sooner or later your observers would switch to '*' and custom PHP code anyway.


It all depends how you define event system, in Moodle it was always notification that something happened, so yes it is and always was a kind of notification system with pluggable destinations - logging means that the events are stored somewhere instead of using them and throwing away.

We were discussing a similar "hook system" which would be used BEFORE something happens, such as ask all plugins if they way to add anything to html form. The communication would be two-way there - hook data could be modified by observers, some hook could reject the action for example.

These two systems hooks & events could not use the same infrastructure and classes, but the API could be similar.
 
Average of ratings: -
Picture of Amanda Doughty
Re: Feedback requested on new Events system specification
Group Developers

As the new events api is proposed for 2.6, how should a developer go about adding new events in 2.4/2.5? I am proposing to add some new file upload events which could be handled by the assignment module to enable a vastly improved audit trail for the activity (see MDL-31220).

My suggestion is to add events for:

1. New file upload

2. File update

3. File deletion

in lib/filelib.php

and then handle them within mod/assign by creating additional log entries.

If this made it into core code then the handlers would have to be added to the list in MDL-39952.

Is their scope for adding new events to 2.4/2.5 with a view to adapting them later for 2.6? I'd rather avoid abortive work if you have an alternative suggestion for interim event development, or are unlikely to consider it at all?

 
Average of ratings: -
Tim at Lone Pine Koala Sanctuary
Re: Feedback requested on new Events system specification
Group DevelopersGroup Documentation writersGroup Particularly helpful Moodlers

Just looking at some recent edits in the spec: http://docs.moodle.org/dev/index.php?title=Event_2&curid=4629&diff=41055&oldid=41049 I think we can improve some of the column names. For example:

If we have to explain that the column called 'crud' stores the 'transaction type', why not call that column 'transactiontype'.

Similarly, renaming 'extradata' to 'extra' seems like a step in the wrong direction.

 
Average of ratings:Useful (2)
Martin in black and white
Re: Feedback requested on new Events system specification
Group DevelopersGroup Documentation writersGroup Moodle HQGroup Particularly helpful MoodlersGroup Testers

Seems like sensible renames.

 
Average of ratings: -
Dan at desk in Moodle HQ, Perth
Re: Feedback requested on new Events system specification
Group DevelopersGroup Moodle Course Creator Certificate holdersGroup Moodle HQGroup Particularly helpful MoodlersGroup Testers

(better late than never).

One class per event

I have to say, I am coming around to the one-class per event idea, particular for the sake of documentation of the events (have a look at lib/db/events.php comments to see how poor the documentation state is). Amazingly, you don't mention the term 'lazy loading' in the documentation once, which really sums up the whole approach..

Can anyone come up with a good real life example of shared event, where they really could be shared and there would be no differences with permissions and that type of thing? We need that to really disprove the one class per event approach.

Something which concerns me a little is the 'distance' between event logic (in its definition) and the calling code. I could well imagine someone making some assumptions in an event definition which don't apply to all cases its called. Just stating that, as you didn't list in the drawbacks.

Cron execution

There is no support for cron execution [...]

Ok, I agree with the decision, but the justification is weak:

this eliminates performance problems

Wow a magic bullet! I am guessing you mean that you don't need to touch the database. Or some other magic? It would be good to explain this.

prevents abuse of cron events.

It would be good to explain what you define as an abuse.

Auto-documentation

Would it be possible/useful/worthile to create a webservices-like admin tool to do a listing on all events? Perhaps it would be useful to have some documention methods for that cause.

get_cached_record

This isn't mentioned in the spec (that I can see), but seems to be in the proposed implementation. Seems fairly fundamental.

 
Average of ratings: -
Tim at Lone Pine Koala Sanctuary
Re: Feedback requested on new Events system specification
Group DevelopersGroup Documentation writersGroup Particularly helpful Moodlers

One class per event

Docs. I see your lib/db/events.php, and raise you a mod/quiz/db/events.php. Good docs relies on developers taking the time to write them, not on any particular type of code. (I guess the integrators could also help by enforcing the documentation standards, but I don't suppose you could do anything about that wink)

Sharing event classes. I already gave the example of DOM events. All the mouse events use one class. Similarly all the key events. The point is that a lot of the differences between events is the data stored in the event (e.g. the event name), not in how the events behave as they flow through the event handling system.

I think my other example, exceptions, is valid. Imagine if you have to define a new class every time where now you just through a moodle_exception.

For a specific, Moodle example, why should each activity have to define its own deleted event. Will there be any way in which mod_quiz_deleted_event is different from mod_forum_deleted_event?

 
Average of ratings: -
Dan at desk in Moodle HQ, Perth
Re: Feedback requested on new Events system specification
Group DevelopersGroup Moodle Course Creator Certificate holdersGroup Moodle HQGroup Particularly helpful MoodlersGroup Testers

I see your lib/db/events.php, and raise you a mod/quiz/db/events.php. Good docs relies on developers taking the time to write them, not on any particular type of code.

If I had more time, i'd try and disprove your example and find a difference between the docs in that file and the in the actual implementation. But I don't and its quite time/cognitively intensive to try and compare those disparate things and verify that they up to date. Even with the reward of disproving your point! Which is one of the tricky things about integrators enforcing the standards.

For a specific, Moodle example, why should each activity have to define its own deleted event. Will there be any way in which mod_quiz_deleted_event is different from mod_forum_deleted_event?

That was what I was after. But would that actually be mod_deleted?

 
Average of ratings: -
Tim at Lone Pine Koala Sanctuary
Re: Feedback requested on new Events system specification
Group DevelopersGroup Documentation writersGroup Particularly helpful Moodlers

But would that actually be mod_deleted?

Well, that is a decision about which events we use in core. Do we want more finer grained events, so you can ask the system to send you mod_quiz_deleted, but not mod_forum_deleted; or do we just have a single mod_deleted event type, and the subscriber then has to do if $event->activity_type == 'mod_quiz'? Clearly both work. The design decision should be based on the list of use-cases you have identified for the events API.

 
Average of ratings: -
Picture of Petr Škoda
Re: Feedback requested on new Events system specification
Group DevelopersGroup Documentation writersGroup Moodle HQGroup Particularly helpful Moodlers
Activities are actually instances stored in course_modules table, the deleting is done by core, not activity itself - this leads to \core\event\course_module_deleted event, right?
 
Average of ratings: -
Dan at desk in Moodle HQ, Perth
Re: Feedback requested on new Events system specification
Group DevelopersGroup Moodle Course Creator Certificate holdersGroup Moodle HQGroup Particularly helpful MoodlersGroup Testers

(Arguing on both sides of the fence here)

Activities are actually instances stored in course_modules table, the deleting is done by core

Is that how we will define the rules? How about adding a new course module, mod_assign. In that case we might also want to know what submission plugins are enabled. Does that generate two events? One \core\event\course_module_deleted and one \mod_assign\event\instance_added?

(Please excuse my sloppy naming).

 
Average of ratings: -
Picture of Petr Škoda
Re: Feedback requested on new Events system specification
Group DevelopersGroup Documentation writersGroup Moodle HQGroup Particularly helpful Moodlers
Cron, external in transaction or failed events are passed to cron via DB table, we also store execution status in database table, the event data may be pretty big, it needs to be revalidated after reading in cron, db writes are anything but fast. Cron processing is a bottleneck already, we can not add more there. The current events were kind of designed by a comittee and overnegineered in my opinion, I have put everything I learned from it into the new spec.

I will add a new section explaining the record caching.
 
Average of ratings: -
Martin Langhoff - Sailing
Re: Feedback requested on new Events system specification
Group DevelopersGroup Particularly helpful Moodlers

Heh, I am late to the events party. RLers pointed this out. Interesting!

Questions, after reading this thread and the wikipage:

  • I don't fully understand "Current execution of external handlers during DB transactions blocks other handlers." -- there is no discussion of locks, forking or threading. How does this block?
  • One of the downsides of events triggered on GET/POST requests is that every pageload potentially needs to load the entry points to all of the Moodle plugins. Are we doing anything to offset that?
  • Anything preventing accidental "event bombs" where code/configuration leads to an event branching out aggressively? (think fork bombs)
  • Similarly... anything preventing circular events?

I personally like event dispatches somewhat restricted, to force some discipline smile . One of the big things that made me abandon Elgg was its horrible use of events instead of function calls. It made debugging a complete nightmare.

 
Average of ratings: -
Dan at desk in Moodle HQ, Perth
Re: Feedback requested on new Events system specification
Group DevelopersGroup Moodle Course Creator Certificate holdersGroup Moodle HQGroup Particularly helpful MoodlersGroup Testers

Since nobody else has replied, i'll try my best guess:

I don't fully understand "Current execution of external handlers during DB transactions blocks other handlers." -- there is no discussion of locks, forking or threading. How does this block?

I think it buffers up dispatching them until the transaction is committed rather than locking. See lib/classes/event/manager.php::process_buffers.

Off topic, but in the direction of your question and also might be of interest to you is a locking framework and cron scheduling proposal/development which Damyon is tackling in MDL-25500 and MDL-25499 subtasks.

One of the downsides of events triggered on GET/POST requests is that every pageload potentially needs to load the entry points to all of the Moodle plugins. Are we doing anything to offset that?

PHP autoloading - there is a new component cache see MDL-39854.

Anything preventing accidental "event bombs" where code/configuration leads to an event branching out aggressively? (think fork bombs) Similarly... anything preventing circular events

Not that i'm aware of and seems very sensible suggestions so I hope Petr or others can respond to that.

 
Average of ratings: -
Tim at Lone Pine Koala Sanctuary
Re: Feedback requested on new Events system specification
Group DevelopersGroup Documentation writersGroup Particularly helpful Moodlers

I have just seen MDL-40063, which is an attempt to replace add_to_log calls in the quiz with events. The net result is +1,250 lines of code for absolutely no gain in functionality.

Does anyone else things that is the sign of an appallingly bad API? Is it too late to improve the API in Moodle 2.7?

 
Average of ratings:Useful (1)
Picture of Tomasz Muras
Re: Feedback requested on new Events system specification
Group DevelopersGroup Particularly helpful MoodlersGroup Translators

Hi there,

I may be missing something obvious - how one (using any API) can get a full list of currently implemented events for current Moodle installation?

cheers,
Tomek

 
Average of ratings: -
Picture of Tomasz Muras
Re: Feedback requested on new Events system specification
Group DevelopersGroup Particularly helpful MoodlersGroup Translators

Hello,

I am not sure if current implementation accomplishes this point from wiki page:

Self-validating data structure (by PHP).

We have a separate class for every event but yet we use arrays to store all data ($this->data[]) that provide no validation from PHP at all.

The validation of the data is done explicitly in function validate_data(), which seems to contain a lot of duplicated code like:

if(!isset($this->data[...])) {
            throw new \coding_exception(...);

Maybe it's a better idea to define somewhere what the required extra data is and have a method in base class checking for it.

cheers,
Tomek

 
Average of ratings: -