Skip to content

Commit

Permalink
Update log_event to use sprintf to apply additional parameters to str…
Browse files Browse the repository at this point in the history 
…ing arguments
  • Loading branch information
@mantis
mantis committed Jan 5, 2014
1 parent 7244db9 commit 2d0df25
Show file tree
Hide file tree
Showing 4 changed files with 35 additions and 36 deletions.
4 changes: 2 additions & 2 deletions core/database_api.php
View file
Expand Up @@ -302,7 +302,7 @@ function db_query( $p_query, $p_limit = -1, $p_offset = -1 ) {
$t_elapsed = number_format( microtime(true) - $t_start, 4 );

if( ON == $g_db_log_queries ) {
log_event( LOG_DATABASE, array( $p_query, $t_elapsed), debug_backtrace() );
log_event( LOG_DATABASE, array( $p_query, $t_elapsed) );
array_push( $g_queries_array, array( $p_query, $t_elapsed ) );
} else {
array_push( $g_queries_array, array( '', $t_elapsed ) );
Expand Down Expand Up @@ -407,7 +407,7 @@ function db_query_bound( $p_query, $arr_parms = null, $p_limit = -1, $p_offset =
}
}
$t_log_msg = array( $p_query, $t_elapsed );
log_event( LOG_DATABASE, $t_log_msg, debug_backtrace() );
log_event( LOG_DATABASE, $t_log_msg );
array_push( $g_queries_array, $t_log_msg );
} else {
array_push( $g_queries_array, array( '', $t_elapsed ) );
Expand Down
42 changes: 21 additions & 21 deletions core/email_api.php
View file
Expand Up @@ -233,15 +233,15 @@ function email_collect_recipients( $p_bug_id, $p_notify_type, $p_extra_user_ids_
if ( ON == email_notify_flag( $p_notify_type, 'explicit' ) ) {
foreach ( $p_extra_user_ids_to_email as $t_user_id ) {
$t_recipients[$t_user_id] = true;
log_event( LOG_EMAIL_RECIPIENT, sprintf( 'Issue = #%d, add explicitly specified user = @U%d', $p_bug_id, $t_user_id ) );
log_event( LOG_EMAIL_RECIPIENT, 'Issue = #%d, add explicitly specified user = @U%d', $p_bug_id, $t_user_id );
}
}

# add Reporter
if( ON == email_notify_flag( $p_notify_type, 'reporter' ) ) {
$t_reporter_id = bug_get_field( $p_bug_id, 'reporter_id' );
$t_recipients[$t_reporter_id] = true;
log_event( LOG_EMAIL_RECIPIENT, sprintf( 'Issue = #%d, add Reporter = @U%d', $p_bug_id, $t_reporter_id ) );
log_event( LOG_EMAIL_RECIPIENT, 'Issue = #%d, add Reporter = @U%d', $p_bug_id, $t_reporter_id );
}

# add Handler
Expand All @@ -250,7 +250,7 @@ function email_collect_recipients( $p_bug_id, $p_notify_type, $p_extra_user_ids_

if( $t_handler_id > 0 ) {
$t_recipients[$t_handler_id] = true;
log_event( LOG_EMAIL_RECIPIENT, sprintf( 'Issue = #%d, add Handler = @U%d', $p_bug_id, $t_handler_id ) );
log_event( LOG_EMAIL_RECIPIENT, 'Issue = #%d, add Handler = @U%d', $p_bug_id, $t_handler_id );
}
}

Expand All @@ -267,7 +267,7 @@ function email_collect_recipients( $p_bug_id, $p_notify_type, $p_extra_user_ids_
while( $t_row = db_fetch_array( $t_result ) ) {
$t_user_id = $t_row['user_id'];
$t_recipients[$t_user_id] = true;
log_event( LOG_EMAIL_RECIPIENT, sprintf( 'Issue = #%d, add Monitor = @U%d', $p_bug_id, $t_user_id ) );
log_event( LOG_EMAIL_RECIPIENT, 'Issue = #%d, add Monitor = @U%d', $p_bug_id, $t_user_id );
}
}

Expand All @@ -287,7 +287,7 @@ function email_collect_recipients( $p_bug_id, $p_notify_type, $p_extra_user_ids_
while( $t_row = db_fetch_array( $t_result ) ) {
$t_user_id = $t_row['reporter_id'];
$t_recipients[$t_user_id] = true;
log_event( LOG_EMAIL_RECIPIENT, sprintf( 'Issue = #%d, add Note Author = @U%d', $p_bug_id, $t_user_id ) );
log_event( LOG_EMAIL_RECIPIENT, 'Issue = #%d, add Note Author = @U%d', $p_bug_id, $t_user_id );
}
}

Expand All @@ -300,7 +300,7 @@ function email_collect_recipients( $p_bug_id, $p_notify_type, $p_extra_user_ids_
if( $t_user['access_level'] <= $t_threshold_max ) {
if( !$t_bug_is_private || access_compare_level( $t_user['access_level'], config_get( 'private_bug_threshold' ) ) ) {
$t_recipients[$t_user['id']] = true;
log_event( LOG_EMAIL_RECIPIENT, sprintf( 'Issue = #%d, add Project User = @U%d', $p_bug_id, $t_user['id'] ) );
log_event( LOG_EMAIL_RECIPIENT, 'Issue = #%d, add Project User = @U%d', $p_bug_id, $t_user['id'] );
}
}
}
Expand All @@ -313,7 +313,7 @@ function email_collect_recipients( $p_bug_id, $p_notify_type, $p_extra_user_ids_
if ( is_array( $t_recipients_included ) ) {
foreach( $t_recipients_included as $t_user_id ) {
$t_recipients[ $t_user_id ] = true;
log_event( LOG_EMAIL_RECIPIENT, sprintf( 'Issue = #%d, %s plugin added user @U%d', $p_bug_id, $t_plugin, $t_user_id ) );
log_event( LOG_EMAIL_RECIPIENT, 'Issue = #%d, %s plugin added user @U%d', $p_bug_id, $t_plugin, $t_user_id );
}
}
}
Expand Down Expand Up @@ -370,21 +370,21 @@ function email_collect_recipients( $p_bug_id, $p_notify_type, $p_extra_user_ids_
foreach( $t_recipients as $t_id => $t_ignore ) {
# Possibly eliminate the current user
if(( auth_get_current_user_id() == $t_id ) && ( OFF == config_get( 'email_receive_own' ) ) ) {
log_event( LOG_EMAIL_RECIPIENT, sprintf( 'Issue = #%d, drop @U%d (own)', $p_bug_id, $t_id ) );
log_event( LOG_EMAIL_RECIPIENT, 'Issue = #%d, drop @U%d (own)', $p_bug_id, $t_id );
continue;
}

# Eliminate users who don't exist anymore or who are disabled
if( !user_exists( $t_id ) || !user_is_enabled( $t_id ) ) {
log_event( LOG_EMAIL_RECIPIENT, sprintf( 'Issue = #%d, drop @U%d (disabled)', $p_bug_id, $t_id ) );
log_event( LOG_EMAIL_RECIPIENT, 'Issue = #%d, drop @U%d (disabled)', $p_bug_id, $t_id );
continue;
}

# Exclude users who have this notification type turned off
if( $t_pref_field ) {
$t_notify = user_pref_get_pref( $t_id, $t_pref_field );
if( OFF == $t_notify ) {
log_event( LOG_EMAIL_RECIPIENT, sprintf( 'Issue = #%d, drop @U%d (pref %s off)', $p_bug_id, $t_id, $t_pref_field ) );
log_event( LOG_EMAIL_RECIPIENT, 'Issue = #%d, drop @U%d (pref %s off)', $p_bug_id, $t_id, $t_pref_field );
continue;
} else {
# Users can define the severity of an issue before they are emailed for
Expand All @@ -394,7 +394,7 @@ function email_collect_recipients( $p_bug_id, $p_notify_type, $p_extra_user_ids_
$t_bug_severity = bug_get_field( $p_bug_id, 'severity' );

if( $t_bug_severity < $t_min_sev_notify ) {
log_event( LOG_EMAIL_RECIPIENT, sprintf( 'Issue = #%d, drop @U%d (pref threshold)', $p_bug_id, $t_id ) );
log_event( LOG_EMAIL_RECIPIENT, 'Issue = #%d, drop @U%d (pref threshold)', $p_bug_id, $t_id );
continue;
}
}
Expand All @@ -405,7 +405,7 @@ function email_collect_recipients( $p_bug_id, $p_notify_type, $p_extra_user_ids_
if( !access_has_bug_level( VIEWER, $p_bug_id, $t_id )
|| $t_bug_date == $t_bugnote_date && !access_has_bugnote_level( VIEWER, $t_bugnote_id, $t_id )
) {
log_event( LOG_EMAIL_RECIPIENT, sprintf( 'Issue = #%d, drop @U%d (access level)', $p_bug_id, $t_id ) );
log_event( LOG_EMAIL_RECIPIENT, 'Issue = #%d, drop @U%d (access level)', $p_bug_id, $t_id );
continue;
}

Expand All @@ -417,7 +417,7 @@ function email_collect_recipients( $p_bug_id, $p_notify_type, $p_extra_user_ids_
# exclude if any plugin returns true (excludes the user)
if ( $t_recipient_excluded ) {
$t_exclude = true;
log_event( LOG_EMAIL_RECIPIENT, sprintf( 'Issue = #%d, %s plugin dropped user @U%d', $p_bug_id, $t_plugin, $t_id ) );
log_event( LOG_EMAIL_RECIPIENT, 'Issue = #%d, %s plugin dropped user @U%d', $p_bug_id, $t_plugin, $t_id );
}
}
}
Expand All @@ -430,7 +430,7 @@ function email_collect_recipients( $p_bug_id, $p_notify_type, $p_extra_user_ids_
# Finally, let's get their emails, if they've set one
$t_email = user_get_email( $t_id );
if( is_blank( $t_email ) ) {
log_event( LOG_EMAIL_RECIPIENT, sprintf( 'Issue = #%d, drop @U%d (no email)', $p_bug_id, $t_id ) );
log_event( LOG_EMAIL_RECIPIENT, 'Issue = #%d, drop @U%d (no email)', $p_bug_id, $t_id );
} else {
# @@@ we could check the emails for validity again but I think
# it would be too slow
Expand Down Expand Up @@ -477,7 +477,7 @@ function email_signup( $p_user_id, $p_password, $p_confirm_hash, $p_admin_name =
# or else users won't be able to sign up
if( !is_blank( $t_email ) ) {
email_store( $t_email, $t_subject, $t_message );
log_event( LOG_EMAIL, sprintf( 'Signup Email = %s, Hash = %s, User = @U%d', $t_email, $p_confirm_hash, $p_user_id ) );
log_event( LOG_EMAIL, 'Signup Email = %s, Hash = %s, User = @U%d', $t_email, $p_confirm_hash, $p_user_id );

if( OFF == config_get( 'email_send_using_cronjob' ) ) {
email_send_all();
Expand Down Expand Up @@ -513,7 +513,7 @@ function email_send_confirm_hash_url( $p_user_id, $p_confirm_hash ) {
# or else users won't be able to receive their reset pws
if( !is_blank( $t_email ) ) {
email_store( $t_email, $t_subject, $t_message );
log_event( LOG_EMAIL, sprintf( 'Password reset for user @U%d sent to %s', $p_user_id, $t_email ) );
log_event( LOG_EMAIL, 'Password reset for user @U%d sent to %s', $p_user_id, $t_email );

if( OFF == config_get( 'email_send_using_cronjob' ) ) {
email_send_all();
Expand Down Expand Up @@ -545,7 +545,7 @@ function email_notify_new_account( $p_username, $p_email ) {

if( !is_blank( $t_recipient_email ) ) {
email_store( $t_recipient_email, $t_subject, $t_message );
log_event( LOG_EMAIL, sprintf( 'New Account Notify for email = \'%s\'', $t_recipient_email ) );
log_event( LOG_EMAIL, 'New Account Notify for email = \'%s\'', $t_recipient_email );

if( OFF == config_get( 'email_send_using_cronjob' ) ) {
email_send_all();
Expand Down Expand Up @@ -585,7 +585,7 @@ function email_generic( $p_bug_id, $p_notify_type, $p_message_id = null, $p_head
if( is_array( $t_recipients ) ) {
# send email to every recipient
foreach( $t_recipients as $t_user_id => $t_user_email ) {
log_event( LOG_EMAIL, sprintf( "Issue = #%d, Type = %s, Msg = '%s', User = @U%d, Email = '%s'.", $p_bug_id, $p_notify_type, $p_message_id, $t_user_id, $t_user_email ) );
log_event( LOG_EMAIL, "Issue = #%d, Type = %s, Msg = '%s', User = @U%d, Email = '%s'.", $p_bug_id, $p_notify_type, $p_message_id, $t_user_id, $t_user_email );

# load (push) user language here as build_visible_bug_data assumes current language
lang_push( user_pref_get_language( $t_user_id, $t_project_id ) );
Expand Down Expand Up @@ -614,7 +614,7 @@ function email_generic( $p_bug_id, $p_notify_type, $p_message_id = null, $p_head
* @return null
*/
function email_monitor_added( $p_bug_id, $p_user_id ) {
log_event( LOG_EMAIL, sprintf( 'Issue #%d monitored by user @U%d', $p_bug_id, $p_user_id ) );
log_event( LOG_EMAIL, 'Issue #%d monitored by user @U%d', $p_bug_id, $p_user_id );

$t_opt = array();
$t_opt[] = bug_format_id( $p_bug_id );
Expand All @@ -631,7 +631,7 @@ function email_monitor_added( $p_bug_id, $p_user_id ) {
* @return null
*/
function email_relationship_added( $p_bug_id, $p_related_bug_id, $p_rel_type ) {
log_event( LOG_EMAIL, sprintf( 'Relationship added: Issue #%d, related issue %d, relationship type %s.', $p_bug_id, $p_related_bug_id, $p_rel_type ) );
log_event( LOG_EMAIL, 'Relationship added: Issue #%d, related issue %d, relationship type %s.', $p_bug_id, $p_related_bug_id, $p_rel_type );

$t_opt = array();
$t_opt[] = bug_format_id( $p_related_bug_id );
Expand All @@ -650,7 +650,7 @@ function email_relationship_added( $p_bug_id, $p_related_bug_id, $p_rel_type ) {
* @return null
*/
function email_relationship_deleted( $p_bug_id, $p_related_bug_id, $p_rel_type ) {
log_event( LOG_EMAIL, sprintf( 'Relationship deleted: Issue #%d, related issue %d, relationship type %s.', $p_bug_id, $p_related_bug_id, $p_rel_type ) );
log_event( LOG_EMAIL, 'Relationship deleted: Issue #%d, related issue %d, relationship type %s.', $p_bug_id, $p_related_bug_id, $p_rel_type );

$t_opt = array();
$t_opt[] = bug_format_id( $p_related_bug_id );
Expand Down
21 changes: 10 additions & 11 deletions core/logging_api.php
View file
Expand Up @@ -50,10 +50,9 @@
* Log an event
* @param int $p_level Valid debug log level
* @param string|array $p_msg Either a string, or an array structured as (string,execution time)
* @param object $p_backtrace [Optional] debug_backtrace() stack to use
* @return null
*/
function log_event( $p_level, $p_msg, $p_backtrace = null ) {
function log_event( $p_level, $p_msg /*,args*/) {
global $g_log_levels;

# check to see if logging is enabled
Expand All @@ -67,15 +66,16 @@ function log_event( $p_level, $p_msg, $p_backtrace = null ) {
$t_event = $p_msg;
$s_msg = var_export( $p_msg, true );
} else {
$args = func_get_args();
array_shift($args); // skip level
array_shift($args); // skip message
$p_msg = vsprintf( $p_msg, $args);

$t_event = array( $p_msg, 0 );
$s_msg = $p_msg;
}

if( $p_backtrace === null ) {
$t_backtrace = debug_backtrace();
} else {
$t_backtrace = $p_backtrace;
}
$t_caller = basename( $t_backtrace[0]['file'] );
$t_caller .= ":" . $t_backtrace[0]['line'];

Expand All @@ -90,7 +90,7 @@ function log_event( $p_level, $p_msg, $p_backtrace = null ) {
$t_now = date( config_get_global( 'complete_date_format' ) );
$t_level = $g_log_levels[$p_level];

$t_plugin_event = '[' . $t_level . '] ' . $s_msg;
$t_plugin_event = '[' . $t_level . '] ' . $p_msg;
if( function_exists( 'event_signal' ) )
event_signal( 'EVENT_LOG', array( $t_plugin_event ) );

Expand Down Expand Up @@ -120,8 +120,8 @@ function log_event( $p_level, $p_msg, $p_backtrace = null ) {
break;
case 'firebug':
if( !class_exists( 'FirePHP' ) ) {
if( file_exists( config_get_global( 'library_path' ) . 'FirePHPCore' . DIRECTORY_SEPARATOR . 'FirePHP.class.php' ) ) {
require_lib( 'FirePHPCore' . DIRECTORY_SEPARATOR . 'FirePHP.class.php' );
if( file_exists( config_get_global( 'library_path' ) . 'FirePHPCore/FirePHP.class.php' ) ) {

This comment has been minimized.

Sign in to view

Copy link
@atrol

atrol Jan 5, 2014

Member

Using "/" as a hardcoded directory separator seems to work also on windows according http://us2.php.net/manual/en/function.dirname.php

But we should always use DIRECTORY_SEPARATOR or we should never use DIRECTORY_SEPARATOR
What do you think?

This comment has been minimized.

Sign in to view

Copy link
@vboctor

vboctor Jan 5, 2014

Member

I recall seeing somewhere a mention of stopping to use DIRECTORY_SEPARATOR from discussions related to master/master2 branches. I personally use it and I'm not aware of the reason we should stop using it. Paul, any reasoning for this?

This comment has been minimized.

Sign in to view

Copy link
@giallu

giallu via email Jan 7, 2014

Member

This comment has been minimized.

Sign in to view

Copy link
@dregad

dregad Jan 7, 2014

Member

My understanding is that it is not necessary to use it when building a path, since PHP/Windows will handle the forward slash just fine.

DIRECTORY_SEPARATOR is required however when parsing paths retrieved from the OS, e.g. with explode() function.

This comment has been minimized.

Sign in to view

Copy link
@atrol

atrol Jan 7, 2014

Member

My understanding is that it is not necessary to use it when building a path, since PHP/Windows will handle the forward slash just fine.

DIRECTORY_SEPARATOR is required however when parsing paths retrieved from the OS, e.g. with explode() function.

This is also my understanding.
The question is, should we remove all usages of DIRECTORY_SEPARATOR and replace it by / when building paths?

Advantages:

  • better readability
  • a bit better performance when removing also string concatenations if possible

Disadvantages:

  • confusing + harder to parse, especially when running on windows and appending to paths retrieved from OS

See also notes at
http://www.php.net/manual/en/dir.constants.php

This comment has been minimized.

Sign in to view

Copy link
@atrol

atrol Jan 12, 2014

Member

grangeway via email:
I think it was daryn that replaced DIRECTORY_SEPERATOR with / - in terms of windows, PHP supports both, so stripping out DIRECTORY_SEPERATOR, removing the string concatenation was the approach dhx/daryn originally agreed to take. I think it probably makes sense to continue with this approach as it simplifies the php code slightly, and there's no downsides to using / on linux or windows.
require_lib( 'FirePHPCore/FirePHP.class.php' );
}
}
if( class_exists( 'FirePHP' ) ) {
Expand Down Expand Up @@ -158,13 +158,12 @@ function log_print_to_page() {
$t_total_queries_count = 0;
$t_total_event_count = count( $g_log_events );

if( $t_total_event_count == 0 ) {
echo "\n\n<!--Mantis Debug Log Output-->";
if( $t_total_event_count == 0 ) {
echo "<!--END Mantis Debug Log Output-->\n\n";
return;
}

echo "\n\n<!--Mantis Debug Log Output-->";
echo "<hr />\n";
echo "<table id=\"log-event-list\">\n";
echo "\t<thead>\n";
Expand Down
4 changes: 2 additions & 2 deletions manage_user_update.php
View file
Expand Up @@ -199,9 +199,9 @@
$t_message = $t_updated_msg . "\n\n" . config_get( 'path' ) . 'account_page.php' . "\n\n" . $t_changes;

if( null === email_store( $t_email, $t_subject, $t_message ) ) {
log_event( LOG_EMAIL, sprintf( 'Notification was NOT sent to ' . $f_username ) );
log_event( LOG_EMAIL, 'Notification was NOT sent to ' . $f_username );
} else {
log_event( LOG_EMAIL, sprintf( 'Account update notification sent to ' . $f_username . ' (' . $t_email . ')' ) );
log_event( LOG_EMAIL, 'Account update notification sent to ' . $f_username . ' (' . $t_email . ')' );
if ( config_get( 'email_send_using_cronjob' ) == OFF ) {
email_send_all();
}
Expand Down

0 comments on commit 2d0df25

Please sign in to comment.