[RFC] Add class for add_log function

Note: We are moving the topics of this forum and it will be deleted at some point

Publish your own request for comments/change or patches for the next version of phpBB. Discuss the contributions and proposals of others. Upcoming releases are 3.2/Rhea and 3.3.
Post Reply
User avatar
nickvergessen
Former Team Member
Posts: 733
Joined: Sun Oct 07, 2007 11:54 am
Location: Stuttgart, Germany
Contact:

[RFC] Add class for add_log function

Post by nickvergessen » Sun Mar 18, 2012 11:52 am

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
Member of the Development-TeamNo Support via PM

User avatar
naderman
Product Manager
Product Manager
Posts: 1727
Joined: Sun Jan 11, 2004 2:11 am
Location: Karlsruhe, Germany
Contact:

Re: [RFC] Add class for add_log function

Post by naderman » Mon Mar 19, 2012 4:37 pm

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?

User avatar
nickvergessen
Former Team Member
Posts: 733
Joined: Sun Oct 07, 2007 11:54 am
Location: Stuttgart, Germany
Contact:

Re: [RFC] Add class for add_log function

Post by nickvergessen » Mon Mar 19, 2012 4:51 pm

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.
Member of the Development-TeamNo Support via PM

User avatar
naderman
Product Manager
Product Manager
Posts: 1727
Joined: Sun Jan 11, 2004 2:11 am
Location: Karlsruhe, Germany
Contact:

Re: [RFC] Add class for add_log function

Post by naderman » Mon Mar 19, 2012 8:57 pm

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?

User avatar
nickvergessen
Former Team Member
Posts: 733
Joined: Sun Oct 07, 2007 11:54 am
Location: Stuttgart, Germany
Contact:

Re: [RFC] Add class for add_log function

Post by nickvergessen » Mon Mar 19, 2012 11:49 pm

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.
Member of the Development-TeamNo Support via PM

User avatar
A_Jelly_Doughnut
Registered User
Posts: 1780
Joined: Wed Jun 04, 2003 4:23 pm

Re: [RFC] Add class for add_log function

Post by A_Jelly_Doughnut » Tue Mar 20, 2012 12:25 am

nickvergessen wrote: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.
A_Jelly_Doughnut

Oleg
Posts: 1150
Joined: Tue Feb 23, 2010 2:38 am
Contact:

Re: [RFC] Add class for add_log function

Post by Oleg » Tue Mar 20, 2012 12:53 am

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.

User avatar
igorw
Registered User
Posts: 500
Joined: Thu Jan 04, 2007 11:47 pm

Re: [RFC] Add class for add_log function

Post by igorw » Tue Mar 20, 2012 9:40 am

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

User avatar
naderman
Product Manager
Product Manager
Posts: 1727
Joined: Sun Jan 11, 2004 2:11 am
Location: Karlsruhe, Germany
Contact:

Re: [RFC] Add class for add_log function

Post by naderman » Tue Mar 20, 2012 1:36 pm

I agree, mostly because of Oleg's concern.

Post Reply