Merge pull request #6652 from ampproject/feature/5939-support-request…

…-flow Optimize reading tail of error log
ampproject · Oct 19, 2021 · 0f94add · 0f94add
2 parents 099cc36 + 47cc8c0
commit 0f94add
Show file tree

Hide file tree

Showing 3 changed files with 71 additions and 11 deletions.
diff --git a/src/Admin/SupportScreen.php b/src/Admin/SupportScreen.php
@@ -173,7 +173,7 @@ public function enqueue_assets( $hook_suffix ) {
 			];
 		}
 
-		$support_data = $this->injector->make( SupportData::class, [ 'args' => $args ] );
+		$support_data = $this->injector->make( SupportData::class, compact( 'args' ) );
 		$data         = $support_data->get_data();
 
 		wp_add_inline_script(

diff --git a/src/Support/SupportData.php b/src/Support/SupportData.php
@@ -355,25 +355,41 @@ public function get_error_log() {
 			];
 		}
 
-		$no_of_lines = 200;
+		$max_lines = 200;
 
 		// phpcs:ignore WordPress.PHP.NoSilencedErrors.Discouraged, WordPress.WP.AlternativeFunctions.file_system_read_fopen
-		$file  = @fopen( $error_log_path, 'r' );
-		$lines = [];
+		$file         = @fopen( $error_log_path, 'r' );
+		$lines        = [];
+		$current_line = '';
+		$position     = 0;
 
 		if ( is_resource( $file ) ) {
-			while ( ! feof( $file ) ) {
-				$line       = fgets( $file );
-				$lines[]    = $line;
-				$line_count = count( $lines );
-				if ( $line_count > $no_of_lines ) {
-					array_shift( $lines );
+
+			while ( -1 !== fseek( $file, $position, SEEK_END ) ) {
+				$char = fgetc( $file );
+
+				if ( PHP_EOL === $char ) {
+					$lines[]      = $current_line;
+					$current_line = '';
+
+					if ( count( $lines ) > $max_lines ) {
+						break;
+					}
+				} else {
+					$current_line = $char . $current_line;
 				}
+
+				$position--;
 			}
 
+			$lines[] = $current_line;
+
 			fclose( $file );
 		}
 
+		$lines = array_filter( $lines );
+		$lines = array_reverse( $lines );
+
 		return [
 			'log_errors' => ini_get( 'log_errors' ),
 			'contents'   => implode( "\n", $lines ),

diff --git a/tests/php/src/Support/SupportDataTest.php b/tests/php/src/Support/SupportDataTest.php
@@ -31,8 +31,14 @@ class SupportDataTest extends DependencyInjectedTestCase {
 	 */
 	public $instance;
 
+	/** @var array */
+	private $previous_ini_config = [
+		'error_log'  => null,
+		'log_errors' => null,
+	];
+
 	/**
-	 * Setup.
+	 * Set up.
 	 *
 	 * @inheritdoc
 	 */
@@ -41,6 +47,22 @@ public function setUp() {
 		parent::setUp();
 
 		$this->instance = $this->injector->make( SupportData::class );
+
+		foreach ( array_keys( $this->previous_ini_config ) as $key ) {
+			$this->previous_ini_config[ $key ] = ini_get( $key );
+		}
+	}
+
+	/**
+	 * Tear down.
+	 *
+	 * @inheritdoc
+	 */
+	public function tearDown() {
+		parent::tearDown();
+		foreach ( $this->previous_ini_config as $key => $value ) {
+			ini_set( $key, $value ); // phpcs:ignore WordPress.PHP.IniSet.log_errors_Blacklisted, WordPress.PHP.IniSet.Risky
+		}
 	}
 
 	/**
@@ -300,6 +322,28 @@ public function test_get_error_log() {
 		$this->assertArrayHasKey( 'log_errors', $output );
 		$this->assertArrayHasKey( 'contents', $output );
 
+		$log_path = $this->temp_filename();
+		$this->assertTrue( is_writable( $log_path ) );
+
+		ini_set( 'log_errors', 1 ); // phpcs:ignore WordPress.PHP.IniSet.log_errors_Blacklisted, WordPress.PHP.IniSet.Risky
+		ini_set( 'error_log', $log_path ); // phpcs:ignore WordPress.PHP.IniSet.log_errors_Blacklisted, WordPress.PHP.IniSet.Risky
+
+		$input_content    = '';
+		$expected_content = '';
+
+		for ( $i = 1; $i <= 300; $i ++ ) {
+			$input_content .= "Line: $i\n";
+		}
+
+		for ( $i = 101; $i <= 300; $i ++ ) {
+			$expected_content .= "Line: $i\n";
+		}
+		$expected_content = trim( $expected_content, "\n" );
+
+		file_put_contents( $log_path, $input_content );
+		$output = $instance->get_error_log();
+
+		$this->assertEquals( $expected_content, $output['contents'] );
 	}
 
 	/**