To Dan https://tracker.moodle.org/browse/MDL-41266?focusedCommentId=271925&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-271925 :
1.a) get_events() methods is not very useful because it returns just list of events. In standard distribution we need it in live report and the other report that allows searching for and filtering of events. It is not useful for participation reports, statistics and pretty much any interesting report because we do not need ten other reports that show individual events, we need to get some more interesting information in reports. This means that most interesting reports are probably going to use the database table directly in custom queries. I do not see a reason for spending lots of time on this if it is not going to be used much.
1.b) Ankit already added description of the select restrictions into http://docs.moodle.org/dev/Logging_2#Reading_API , I believe this is not a show stopper due to 1.a) because this API is not useful for advanced reports.
2.a) the organisation of interfaces was done very carefully - the reading and writing of events to log storages is 100% separate. The core contains only the stuff that is used from standard reports, you can for example supply your own logging manager with completely different log writing implementation API and the standard reports are still going to work just fine.
2.b) This is not about "one log plugin" there are two completely separate APIs involved - first is log writing (defined in tool_log plugin) and log reading interface defined in the core. I wanted to make a simple example of external database log storage without any reading but in the end others implemented the reading support which pretty much spoiled my carefully designed example. I guess we need a new non-sql example here.
In anycase I do not expect developers start implementing their own log readers and writers unless they already have a custom logging/reporting solution already. We need new interesting reports, not a bazillion of different log storages which have no way of working with event basic reports such as participation and statistics.
3. Exactly! The idea is to use the new PHP language features consistently/properly in new subsystems because we cannot use them in exsting APIs...
3.a) Read the traits docs, I suppose you will see that the two independent reading/logging interfaces are the perfect example for trait usage. I was hoping we could introduce it in other areas such as DML drivers.
3.b) the requirement for insert_events is clearly documented in inline docs. Any way nobody forces you to use the traits, that is the huge difference between extends and uses keywords.
3.c) Traits are new, traits are different - the resulting code does not feel the same. Your comments seem too much subjective to me, I hope you will reconsider them after you get to use the traits yourself. Or if you really want to try to code it yourself the old way, I would be glad to point out the code constructs that smell silly to me.
4. The fallback to dummy manager is default intentionally. It allows you to completely disable the logging subsystem. I would consider it a bug if it was falling back to some random or unconfigured logging manager - will not fix
5. There is nothing wrong with that explanation imho, simply do not ever try to call it directly. Unfortunately PHP is not Java with awesome protected keyword - we cannot restrict access to any public method for specific classes only - will not fix
6. restore_legacy is very ugly indeed - it will get tons more hacks if you allow use to get this integrated, everything there is 100% self contained and can be fixed/improved later as necessary - this is good design imo
7. of course there is code duplication - first you design plugins and only after that you start looking for similarities. The point here is that we must make the public API as small as possible which allows addon developers to create some truly innovative log storage and log reader plugins.
8. Some lang strings suck, I agree. Unfortunately the native English speakers we have in the backend team did not participate for some reason - we need help with fixing of this
To Damyon https://tracker.moodle.org/browse/MDL-41266?focusedCommentId=271944&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-271944:
we have spent a lot of time discussion the APIs and I can assure you the way they are they work fine for the reports we have now in Moodle. I believe that your proposal would not allow us to implement them - not a bug imo