Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce advanced Exclude tab to replace Connectors tab #251 #278

Merged
merged 15 commits into from
Mar 7, 2014
Merged
Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
88 changes: 86 additions & 2 deletions classes/connector.php
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,9 @@ public static function register() {
if ( ! self::is_logging_enabled_for_user() ) {
return;
}
if ( ! self::is_logging_enabled_for_ip() ) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding checks in the base class of all connector will cause it to be executed by all connectors while they try to register themselves, which is something we probably don't need. We probably need to move those checks ( including is_logging_enabled_for_user by @jonathanbardo ) to includes/connectors.php so it is done once on plugin load.
@fjarrett @jonathanbardo What do you think ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I think it would be more efficient.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@faishal If you agree as well on this, please carry on with moving any redundant checks ( IP / User exclusion ) to the WP_Stream_Connectors class.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@shadyvb I am also agree with that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@shadyvb should i move functions is_logging_enabled_for_user & is_logging_enabled_for_ip into WP_Stream_Settings because it is a part of settings

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer putting them in the same class that'll use them, just to make it easier for people dealing with the same functionality later. Unless other classes would be using them, which is a far aim in this scenario.

return;
}

foreach ( $class::$actions as $action ) {
add_action( $action, array( $class, 'callback' ), null, 5 );
Expand Down Expand Up @@ -97,8 +100,44 @@ public static function is_logging_enabled_for_user( $user = null ) {
} else {
// If a user is part of a role that we don't want to log, we disable it
$user_roles = array_values( $user->roles );
$roles_logged = WP_Stream_Settings::$options['general_log_activity_for'];
$bool = ! ( count( array_intersect( $user_roles, $roles_logged ) ) === 0 );
$roles_logged = WP_Stream_Settings::$options['exclude_authors_and_roles'];
$bool = ( count( array_intersect( $user_roles, $roles_logged ) ) === 0 );
//Check user id in exclude array
if ( $bool ){
$bool = ! ( in_array( $user->ID , $roles_logged ) );
}
}

/**
* Filter sets boolean result value for this method
*
* @param bool
* @param obj $user Current user object
* @param string Current class name
* @return bool
*/
return apply_filters( 'wp_stream_record_log', $bool, $user, get_called_class() );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@faishal nice! even including hook docs.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@faishal Unless we have this filter somewhere else with the same name, and receiving same input, we should change the filter name to reflect what it really does.
It'd also be great if we can change the description to something like Filter to exclude actions of a specific user from being logged or something.
Nice work man! /five!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@faishal Also wanted to leave a /five for this!

}

/**
* Check if we need to record action for IP
*
* @param null $ip
*
* @return mixed|void
*/
public static function is_logging_enabled_for_ip( $ip = null ) {
if ( is_null( $ip ) ){
$ip = filter_input( INPUT_SERVER, 'REMOTE_ADDR', FILTER_VALIDATE_IP );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@faishal We have recently switched to wp_stream_filter_input, our own implementation of filters, read more on this at #257. So you'd just need to add the wp_stream_ prefix to filter_input and filter_var, and it should work as is with no further changes needed. Please do so for all instances of those two functions.

} else {
$ip = filter_var( $ip, FILTER_VALIDATE_IP );
}

// If ip is not valid the we will log the action
if ( $ip === false ) {
$bool = true;
} else {
$bool = self::is_logging_enabled( 'ip_addresses', $ip );
}

/**
Expand All @@ -109,9 +148,43 @@ public static function is_logging_enabled_for_user( $user = null ) {
* @param string Current class name
* @return bool
*/

$user = wp_get_current_user();
return apply_filters( 'wp_stream_record_log', $bool, $user, get_called_class() );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@faishal I'm not sure of the purpose of this filter here, since it handles users not IPs while inside the check for exclusion of IPs.

}

/**
* @param $action string action slug to check whether logging is enable or not for that action
*
* @return bool
*/
public static function is_logging_enabled_for_action( $action ) {
return self::is_logging_enabled( 'actions', $action );
}

/**
* @param $context string context slug to check whether logging is enable or not for that context
*
* @return bool
*/
public static function is_logging_enabled_for_context( $context ) {
return self::is_logging_enabled( 'contexts', $context );
}

/**
* This function is use to check whether logging is enabled
*
* @param $column string name of the setting key (actions|ip_addresses|contexts|connectors)
* @param $value string to check in excluded array
* @return array
*/
public static function is_logging_enabled( $column, $value ){

$excluded_values = WP_Stream_Settings::get_excluded_by_key( $column );
$bool = ( ! in_array( $value, $excluded_values ) ) ;

return $bool;
}
/**
* Log handler
*
Expand All @@ -125,6 +198,17 @@ public static function is_logging_enabled_for_user( $user = null ) {
* @return void
*/
public static function log( $message, $args, $object_id, $contexts, $user_id = null ) {
//Prevent inserting Excluded Context & Actions
foreach ( $contexts as $context => $action ){
if ( ! self::is_logging_enabled_for_context( $context ) ){
unset( $contexts[$context] );
} else if ( ! self::is_logging_enabled_for_action( $action ) ){
unset( $contexts[$context] );
}
}
if ( count( $contexts ) == 0 ){
return ;
}
$class = get_called_class();

return WP_Stream_Log::get_instance()->log(
Expand Down
24 changes: 20 additions & 4 deletions includes/connectors.php
Original file line number Diff line number Diff line change
Expand Up @@ -57,8 +57,8 @@ public static function load() {
*/
self::$connectors = apply_filters( 'wp_stream_connectors', $classes );

// Get active connectors
$active_connectors = WP_Stream_Settings::get_active_connectors();
// Get excluded connectors
$excluded_connectors = WP_Stream_Settings::get_excluded_connectors();

foreach ( self::$connectors as $connector ) {
// Check if the connectors extends the WP_Stream_Connector class, if not skip it
Expand All @@ -77,8 +77,18 @@ public static function load() {
self::$term_labels['stream_connector'][$connector::$name] = $connector::get_label();
}

// Check if connector is activated
if ( ! in_array( $connector::$name, $active_connectors ) ) {
/**
* Filter allows to continue register excluded connector
*
* @param boolean TRUE if exclude otherwise false
* @param string connector unique name
* @param array Excluded connector array
*/

$is_excluded_connector = apply_filters( 'wp_stream_check_connector_is_excluded', in_array( $connector::$name, $excluded_connectors ), $connector::$name, $excluded_connectors );


if ( $is_excluded_connector ) {
continue;
}

Expand All @@ -94,6 +104,12 @@ public static function load() {
$connector::get_context_labels()
);
}

/**
* This allow to perform action after all connectors registration
* @param array all register connectors labels array
*/
do_action( 'wp_stream_after_connectors_registration', self::$term_labels['stream_connector'] );
}


Expand Down
7 changes: 7 additions & 0 deletions includes/install.php
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,13 @@ public static function update( $db_version, $current ) {
if ( version_compare( $db_version, '1.1.7' ) == -1 ) {
$wpdb->query( "ALTER TABLE {$prefix}stream MODIFY ip varchar(39) NULL AFTER created" );
}

// If version is lower than 1.2.3, do the update routine for site options
//Backward setting compatibility for old version plugins
if ( version_compare( $db_version, '1.2.3', '<=' ) ) {
add_filter( 'wp_stream_after_connectors_registration', 'WP_Stream_Settings::migrate_old_options' );
}

}

}
34 changes: 31 additions & 3 deletions includes/list-table.php
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,35 @@ function get_records() {
$hide_disabled_connectors_records = apply_filters( 'wp_stream_list_table_hide_disabled_connectors_records', true );

if ( true === $hide_disabled_connectors_records ) {
$args['connector__in'] = WP_Stream_Settings::get_active_connectors();
$args['connector__not_in'] = WP_Stream_Settings::get_excluded_connectors();
}
}

// Exclude disabled context
if ( empty( $args['context'] ) ) {
/**
* Toggle visibility of records from disabled context on list table
*
* @param bool $hidden Visibility status, hidden by default.
*/
$hide_disabled_contexts_records = apply_filters( 'wp_stream_list_table_hide_disabled_contexts_records', true );

if ( true === $hide_disabled_contexts_records ) {
$args['context__not_in'] = WP_Stream_Settings::get_excluded_contexts();
}
}

// Exclude disabled connectors
if ( empty( $args['action'] ) ) {
/**
* Toggle visibility of records from disabled connectors on list table
*
* @param bool $hidden Visibility status, hidden by default.
*/
$hide_disabled_actions_records = apply_filters( 'wp_stream_list_table_hide_disabled_actions_records', true );

if ( true === $hide_disabled_actions_records ) {
$args['action__not_in'] = WP_Stream_Settings::get_excluded_actions();
}
}

Expand Down Expand Up @@ -402,9 +430,9 @@ function assemble_records( $column, $table = '' ) {
$hide_disabled_connectors_filter = apply_filters( 'wp_stream_list_table_hide_disabled_connectors', true );

if ( true === $hide_disabled_connectors_filter ) {
$active_connectors = WP_Stream_Settings::get_active_connectors();
$excluded_connectors = WP_Stream_Settings::get_excluded_connectors();
foreach ( array_keys( $all_records ) as $_connector ) {
if ( ! in_array( $_connector, $active_connectors ) ) {
if ( in_array( $_connector, $excluded_connectors ) ) {
unset( $all_records[ $_connector ] );
}
}
Expand Down
Loading