[RFC] Add class for add_log function

[nickvergessen]

includes/functions_user.php:

Code: Select all

			// In phpBB 3.1.x i want to have logging in a class to be able to control it
			// For now, we need a quite hakish approach to circumvent logging for some actions
			// @todo implement cleanly
			if (!empty($GLOBALS['skip_add_log']))

I made a small class which is used by add_log. The class has two functions enable() and disable() to control whether logs should be added to the database. I also added unit tests for add_log() which work for the old and new version, to ensure everything works like it did.

I also added potential event listener, which need to be un-commented when the event stuff is merged.

Ticket: http://tracker.phpbb.com/browse/PHPBB3-10714
PR: https://github.com/phpbb/phpbb3/pull/631

[naderman]

What are your thoughts regarding Igor's concern about the enable/disable function? Should the log really provide an enable/disable method? Anyone else with an opinion on this?

[nickvergessen]

I'd say it has almost 0 additional costs, and it might help in some cases. But of course we can add a 10th parameter which forces us to add all default values to our calls.

[naderman]

Well the question is just, is it bad that your code logs something but it never shows up in the log, because somewhere someone disabled logging? Or is the inconvenience worse that you need to pass around parameters that define whether or not to log?

[nickvergessen]

It is not stored in the log, so it is not logged. In this case false is returned, so you know something went wrong, if its unintended.

[A_Jelly_Doughnut]

In this case false is returned, so you know something went wrong, if its unintended.

Assuming you check return value, which people aren't used to doing ...

@naderman: The reason for the feature currently hacked into add_log is things like the "prune user" feature. user_delete is called once for every user deleted, and every call to user_delete writes an entry to a log. The spam in the log can be a little annoying (but whether that is better or worse than missing log entires is up for debate)

====

What if you could enable a kind of log buffering instead ... if the current log string is the same as the last log string, then merge the arguments until buffering is disabled or the log strings no longer match ... more complicated implementation, but it all gets hidden in the new class.

[Oleg]

Let's take the case of logging per user when pruning users.

What if in the process of pruning users there is an error elsewhere which normally is logged. If you turn off logging globally you won't get that error logged and something might fail with no indication that it failed.

[igorw]

I still think there should be an argument, imo that's the cleanest solution.

[naderman]

I agree, mostly because of Oleg's concern.