From 8e0f40d3d14ab0f7b47af4a21464d7a51cedf5e1 Mon Sep 17 00:00:00 2001 From: Jon Surrell Date: Tue, 24 Jan 2017 14:10:00 +0100 Subject: [PATCH 1/3] Add WC_Log_Handler_Interface Abstract class `WC_Log_Handler` implements interface --- .../abstracts/abstract-wc-log-handler.php | 15 +---------- .../class-wc-log-handler-interface.php | 27 +++++++++++++++++++ woocommerce.php | 1 + 3 files changed, 29 insertions(+), 14 deletions(-) create mode 100644 includes/interfaces/class-wc-log-handler-interface.php diff --git a/includes/abstracts/abstract-wc-log-handler.php b/includes/abstracts/abstract-wc-log-handler.php index 39d74dd92c9..a8f5898ffd7 100644 --- a/includes/abstracts/abstract-wc-log-handler.php +++ b/includes/abstracts/abstract-wc-log-handler.php @@ -11,20 +11,7 @@ if ( ! defined( 'ABSPATH' ) ) { * @category Abstract Class * @author WooThemes */ -abstract class WC_Log_Handler { - - - /** - * Handle a log entry. - * - * @param int $timestamp Log timestamp. - * @param string $level emergency|alert|critical|error|warning|notice|info|debug - * @param string $message Log message. - * @param array $context Additional information for log handlers. - * - * @return bool False if value was not handled and true if value was handled. - */ - abstract public function handle( $timestamp, $level, $message, $context ); +abstract class WC_Log_Handler implements WC_Log_Handler_Interface { /** * Formats a timestamp for use in log messages. diff --git a/includes/interfaces/class-wc-log-handler-interface.php b/includes/interfaces/class-wc-log-handler-interface.php new file mode 100644 index 00000000000..1f5dfc67d35 --- /dev/null +++ b/includes/interfaces/class-wc-log-handler-interface.php @@ -0,0 +1,27 @@ + Date: Tue, 24 Jan 2017 23:07:58 +0100 Subject: [PATCH 2/3] Validate handlers implement WC_Log_handler_Interface Add tests for interface validation. --- includes/class-wc-logger.php | 19 ++++++++++- .../class-wc-log-handler-interface.php | 1 + tests/unit-tests/log/logger.php | 32 +++++++++++++++---- 3 files changed, 44 insertions(+), 8 deletions(-) diff --git a/includes/class-wc-logger.php b/includes/class-wc-logger.php index 4a11e458fc4..543b829135a 100644 --- a/includes/class-wc-logger.php +++ b/includes/class-wc-logger.php @@ -43,6 +43,23 @@ class WC_Logger implements WC_Logger_Interface { $handlers = apply_filters( 'woocommerce_register_log_handlers', array() ); } + $register_handlers = array(); + foreach ( $handlers as $handler ) { + $implements = class_implements( $handler ); + if ( is_object( $handler ) && is_array( $implements ) && in_array( 'WC_Log_Handler_Interface', $implements ) ) { + $register_handlers[] = $handler; + } else { + wc_doing_it_wrong( + __METHOD__, + sprintf( + __( 'The provided handler %s does not implement WC_Log_Handler_Interface.', 'woocommerce' ), + esc_html( is_object( $handler ) ? get_class( $handler ) : $handler ) + ), + '2.7' + ); + } + } + if ( null !== $threshold ) { $threshold = WC_Log_Levels::get_level_severity( $threshold ); } elseif ( defined( 'WC_LOG_THRESHOLD' ) && WC_Log_Levels::is_valid_level( WC_LOG_THRESHOLD ) ) { @@ -51,7 +68,7 @@ class WC_Logger implements WC_Logger_Interface { $threshold = null; } - $this->handlers = $handlers; + $this->handlers = $register_handlers; $this->threshold = $threshold; } diff --git a/includes/interfaces/class-wc-log-handler-interface.php b/includes/interfaces/class-wc-log-handler-interface.php index 1f5dfc67d35..5bcd92934ad 100644 --- a/includes/interfaces/class-wc-log-handler-interface.php +++ b/includes/interfaces/class-wc-log-handler-interface.php @@ -13,6 +13,7 @@ if ( ! defined( 'ABSPATH' ) ) { * @author WooThemes */ interface WC_Log_Handler_Interface { + /** * Handle a log entry. * diff --git a/tests/unit-tests/log/logger.php b/tests/unit-tests/log/logger.php index 8e8508e1f65..ee9bc4919b4 100644 --- a/tests/unit-tests/log/logger.php +++ b/tests/unit-tests/log/logger.php @@ -15,7 +15,7 @@ class WC_Tests_Logger extends WC_Unit_Test_Case { public function test_add() { $time = time(); $handler = $this - ->getMockBuilder( 'WC_Log_Handler' ) + ->getMockBuilder( 'WC_Log_Handler_Interface' ) ->setMethods( array( 'handle' ) ) ->getMock(); $handler @@ -82,19 +82,19 @@ class WC_Tests_Logger extends WC_Unit_Test_Case { */ public function test_log_handlers() { $false_handler = $this - ->getMockBuilder( 'WC_Log_Handler' ) + ->getMockBuilder( 'WC_Log_Handler_Interface' ) ->setMethods( array( 'handle' ) ) ->getMock(); $false_handler->expects( $this->exactly( 8 ) )->method( 'handle' )->will( $this->returnValue( false ) ); $true_handler = $this - ->getMockBuilder( 'WC_Log_Handler' ) + ->getMockBuilder( 'WC_Log_Handler_Interface' ) ->setMethods( array( 'handle' ) ) ->getMock(); $false_handler->expects( $this->exactly( 8 ) )->method( 'handle' )->will( $this->returnValue( true ) ); $final_handler = $this - ->getMockBuilder( 'WC_Log_Handler' ) + ->getMockBuilder( 'WC_Log_Handler_Interface' ) ->setMethods( array( 'handle' ) ) ->getMock(); $final_handler->expects( $this->exactly( 8 ) )->method( 'handle' ); @@ -159,7 +159,7 @@ class WC_Tests_Logger extends WC_Unit_Test_Case { // Test no filtering by default delete_option( 'woocommerce_log_threshold' ); $handler = $this - ->getMockBuilder( 'WC_Log_Handler' ) + ->getMockBuilder( 'WC_Log_Handler_Interface' ) ->setMethods( array( 'handle' ) ) ->getMock(); $handler @@ -192,6 +192,24 @@ class WC_Tests_Logger extends WC_Unit_Test_Case { $log->debug( 'debug message' ); } + /** + * Test that the logger validates handlers + * + * If a handler does not implement WC_Log_Handler_Interface, WC_Logger should complain + * (wc_doing_it_wrong) and not register the handler. This handler should receive no messages. + * + * @since 2.7.0 + */ + public function test_validate_handler_interface() { + $handler = $this + ->getMockBuilder( 'stdClass' ) + ->setMethods( array( 'handle' ) ) + ->getMock(); + $handler->expects( $this->never() )->method( 'handle' ); + new WC_Logger( array( $handler ) ); + $this->setExpectedIncorrectUsage( 'WC_Logger::__construct' ); + } + /** * Helper for 'woocommerce_register_log_handlers' filter test. * @@ -204,7 +222,7 @@ class WC_Tests_Logger extends WC_Unit_Test_Case { */ public function return_assertion_handlers() { $handler = $this - ->getMockBuilder( 'WC_Log_Handler' ) + ->getMockBuilder( 'WC_Log_Handler_Interface' ) ->setMethods( array( 'handle' ) ) ->getMock(); $handler->expects( $this->exactly( 8 ) )->method( 'handle' ); @@ -224,7 +242,7 @@ class WC_Tests_Logger extends WC_Unit_Test_Case { public function create_mock_handler() { $time = time(); $handler = $this - ->getMockBuilder( 'WC_Log_Handler' ) + ->getMockBuilder( 'WC_Log_Handler_Interface' ) ->setMethods( array( 'handle' ) ) ->getMock(); From 65b5e4181f850443c53cef134f58d69bb8319b51 Mon Sep 17 00:00:00 2001 From: Jon Surrell Date: Tue, 24 Jan 2017 23:09:19 +0100 Subject: [PATCH 3/3] Use __METHOD__ over "{$class}::{$method}" --- includes/class-wc-logger.php | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/includes/class-wc-logger.php b/includes/class-wc-logger.php index 543b829135a..49c3bafa1eb 100644 --- a/includes/class-wc-logger.php +++ b/includes/class-wc-logger.php @@ -119,9 +119,7 @@ class WC_Logger implements WC_Logger_Interface { */ public function log( $level, $message, $context = array() ) { if ( ! WC_Log_Levels::is_valid_level( $level ) ) { - $class = __CLASS__; - $method = __FUNCTION__; - wc_doing_it_wrong( "{$class}::{$method}", sprintf( __( 'WC_Logger::log was called with an invalid level "%s".', 'woocommerce' ), $level ), '2.7' ); + wc_doing_it_wrong( __METHOD__, sprintf( __( 'WC_Logger::log was called with an invalid level "%s".', 'woocommerce' ), $level ), '2.7' ); } if ( $this->should_handle( $level ) ) {