[ticket/16639] Add types and small changes in tests

PHPBB3-16639

phpBB/phpbb/storage/adapter/adapter_interface.php

@@ -22,29 +22,26 @@ interface adapter_interface
 	 *
 	 * @param array	options	Storage-specific options.
 	 */
-	public function configure($options);
+	public function configure(array $options): void;
 
 	/**
 	 * Dumps content into a file
 	 *
-	 * @param string	path		The file to be written to.
+	 * @param string $path
-	 * @param string	content		The data to write into the file.
+	 * @param string $content
-	 *
 	 * @throws exception When the file cannot be written
 	 */
-	public function put_contents($path, $content);
+	public function put_contents(string $path, string $content): void;
 
 	/**
 	 * Read the contents of a file
 	 *
 	 * @param string $path The file to read
 	 *
-	 * @throws exception	When cannot read file contents
-	 *
 	 * @return string Returns file contents
-	 *
+	 * @throws exception When cannot read file contents
 	 */
-	public function get_contents($path);
+	public function get_contents(string $path): string;
 
 	/**
 	 * Checks the existence of files or directories
@@ -53,7 +50,7 @@ interface adapter_interface
 	 *
 	 * @return bool Returns true if the file/directory exist, false otherwise.
 	 */
-	public function exists($path);
+	public function exists(string $path): bool;
 
 	/**
 	 * Removes files or directories
@@ -62,7 +59,7 @@ interface adapter_interface
 	 *
 	 * @throws exception When removal fails.
 	 */
-	public function delete($path);
+	public function delete(string $path): void;
 
 	/**
 	 * Rename a file or a directory
@@ -72,7 +69,7 @@ interface adapter_interface
 	 *
 	 * @throws exception When file/directory cannot be renamed
 	 */
-	public function rename($path_orig, $path_dest);
+	public function rename(string $path_orig, string $path_dest): void;
 
 	/**
 	 * Copies a file
@@ -82,7 +79,7 @@ interface adapter_interface
 	 *
 	 * @throws exception When the file cannot be copied
 	 */
-	public function copy($path_orig, $path_dest);
+	public function copy(string $path_orig, string $path_dest): void;
 
 	/**
 	 * Get direct link
@@ -90,16 +87,14 @@ interface adapter_interface
 	 * @param string $path The file
 	 *
 	 * @return string Returns link.
-	 *
 	 */
-	public function get_link($path);
+	public function get_link(string $path): string;
 
 	/**
 	 * Get space available in bytes
 	 *
-	 * @throws exception		When unable to retrieve available storage space
-	 *
 	 * @return float Returns available space
+	 * @throws exception When unable to retrieve available storage space
 	 */
-	public function free_space();
+	public function free_space(): float;
 }

phpBB/phpbb/storage/adapter/local.php

@@ -104,7 +104,7 @@ class local implements adapter_interface, stream_interface
 	/**
 	 * {@inheritdoc}
 	 */
-	public function configure($options)
+	public function configure(array $options): void
 	{
 		if (substr($options['path'], -1, 1) !== DIRECTORY_SEPARATOR)
 		{
@@ -119,7 +119,7 @@ class local implements adapter_interface, stream_interface
 	/**
 	 * {@inheritdoc}
 	 */
-	public function put_contents($path, $content)
+	public function put_contents(string $path, string $content): void
 	{
 		$this->ensure_directory_exists($path);
 
@@ -136,7 +136,7 @@ class local implements adapter_interface, stream_interface
 	/**
 	 * {@inheritdoc}
 	 */
-	public function get_contents($path)
+	public function get_contents(string $path): string
 	{
 		$content = @file_get_contents($this->root_path . $this->get_path($path) . $this->get_filename($path));
 
@@ -151,7 +151,7 @@ class local implements adapter_interface, stream_interface
 	/**
 	 * {@inheritdoc}
 	 */
-	public function exists($path)
+	public function exists(string $path): bool
 	{
 		return $this->filesystem->exists($this->root_path . $this->get_path($path) . $this->get_filename($path));
 	}
@@ -159,7 +159,7 @@ class local implements adapter_interface, stream_interface
 	/**
 	 * {@inheritdoc}
 	 */
-	public function delete($path)
+	public function delete(string $path): void
 	{
 		try
 		{
@@ -176,7 +176,7 @@ class local implements adapter_interface, stream_interface
 	/**
 	 * {@inheritdoc}
 	 */
-	public function rename($path_orig, $path_dest)
+	public function rename(string $path_orig, string $path_dest): void
 	{
 		$this->ensure_directory_exists($path_dest);
 
@@ -195,7 +195,7 @@ class local implements adapter_interface, stream_interface
 	/**
 	 * {@inheritdoc}
 	 */
-	public function copy($path_orig, $path_dest)
+	public function copy(string $path_orig, string $path_dest): void
 	{
 		$this->ensure_directory_exists($path_dest);
 
@@ -310,7 +310,7 @@ class local implements adapter_interface, stream_interface
 	/**
 	 * {@inheritdoc}
 	 */
-	public function read_stream($path)
+	public function read_stream(string $path)
 	{
 		$stream = @fopen($this->root_path . $this->get_path($path) . $this->get_filename($path), 'rb');
 
@@ -325,7 +325,7 @@ class local implements adapter_interface, stream_interface
 	/**
 	 * {@inheritdoc}
 	 */
-	public function write_stream($path, $resource)
+	public function write_stream(string $path, $resource): void
 	{
 		$this->ensure_directory_exists($path);
 
@@ -420,7 +420,7 @@ class local implements adapter_interface, stream_interface
 	 *
 	 * @return array	Properties
 	 */
-	public function file_image_height($path)
+	public function file_image_height($path): array
 	{
 		return $this->image_dimensions($path);
 	}
@@ -428,7 +428,7 @@ class local implements adapter_interface, stream_interface
 	/**
 	 * {@inheritdoc}
 	 */
-	public function get_link($path)
+	public function get_link(string $path): string
 	{
 		return generate_board_url() . '/' . $this->path . $path;
 	}
@@ -436,7 +436,7 @@ class local implements adapter_interface, stream_interface
 	/**
 	 * {@inheritdoc}
 	 */
-	public function free_space()
+	public function free_space(): float
 	{
 		if (function_exists('disk_free_space'))
 		{

phpBB/phpbb/storage/stream_interface.php

@@ -13,6 +13,8 @@
 
 namespace phpbb\storage;
 
+use phpbb\storage\exception\exception;
+
 interface stream_interface
 {
 	/**
@@ -20,11 +22,10 @@ interface stream_interface
 	 *
 	 * @param string $path File to read
 	 *
-	 * @throws \phpbb\storage\exception\exception		When unable to open file
-	 *
 	 * @return resource Returns a file pointer
+	 * @throws exception\exception When unable to open file
 	 */
-	public function read_stream($path);
+	public function read_stream(string $path);
 
 	/**
 	 * Writes a new file using a stream
@@ -32,8 +33,9 @@ interface stream_interface
 	 * @param string $path The target file
 	 * @param resource $resource The resource
 	 *
-	 * @throws \phpbb\storage\exception\exception		When target file exists
+	 * @return void
+	 * @throws exception When target file exists
 	 * When target file cannot be created
 	 */
-	public function write_stream($path, $resource);
+	public function write_stream(string $path, $resource): void;
 }

tests/storage/adapter/local_subfolders_test.php

@@ -11,95 +11,121 @@
  *
  */
 
- class phpbb_storage_adapter_local_subfolders_test extends phpbb_test_case
+require_once __DIR__ . '/local_test_case.php';
- {
-	protected $adapter;
-
-	protected $path;
-
-	protected $filesystem;
 
+class local_subfolders_test extends phpbb_local_test_case
+{
 	protected function setUp(): void
 	{
 		parent::setUp();
 
-		$this->filesystem = new \phpbb\filesystem\filesystem();
-		$phpbb_root_path = getcwd() . DIRECTORY_SEPARATOR;
-
-		$this->adapter = new \phpbb\storage\adapter\local($this->filesystem, new \FastImageSize\FastImageSize(), new \phpbb\mimetype\guesser(array(new \phpbb\mimetype\extension_guesser)), $phpbb_root_path);
 		$this->adapter->configure(['path' => 'test_path', 'subfolders' => true]);
-
-		$this->path = $phpbb_root_path . 'test_path/';
-		mkdir($this->path);
 	}
 
-	protected function tearDown(): void
+	public function test_put_contents(): void
-	{
-		$this->adapter = null;
-		rmdir($this->path);
-	}
-
-	public function test_put_contents()
 	{
+		// When
 		$this->adapter->put_contents('file.txt', 'abc');
-		$this->assertTrue(file_exists($this->path . '3d/8e/file.txt'));
+
-		$this->assertEquals(file_get_contents($this->path . '3d/8e/file.txt'), 'abc');
+		// Then
+		$this->assertFileExists($this->path . '3d/8e/file.txt');
+		$this->assertFileContains($this->path . '3d/8e/file.txt', 'abc');
+
+		// Clean test
 		unlink($this->path . '3d/8e/file.txt');
 		rmdir($this->path . '3d/8e');
 		rmdir($this->path . '3d');
 	}
 
-	public function test_get_contents()
+	public function test_get_contents(): void
 	{
+		// Given
 		mkdir($this->path . '3d/8e', 0777, true);
 		file_put_contents($this->path . '3d/8e/file.txt', 'abc');
-		$this->assertEquals($this->adapter->get_contents('file.txt'), 'abc');
+
+		// When
+		$content = $this->adapter->get_contents('file.txt');
+
+		// Then
+		$this->assertEquals('abc', $content);
+
+		// Clean test
 		unlink($this->path . '3d/8e/file.txt');
 		rmdir($this->path . '3d/8e');
 		rmdir($this->path . '3d');
 	}
 
-	public function test_exists()
+	public function test_exists(): void
 	{
+		// Given
 		mkdir($this->path . '3d/8e', 0777, true);
 		touch($this->path . '3d/8e/file.txt');
-		$this->assertTrue($this->adapter->exists('file.txt'));
+
-		$this->assertFalse($this->adapter->exists('3d/8e/file.txt'));
+		// When
+		$existent_file = $this->adapter->exists('file.txt');
+		$non_existent_file = $this->adapter->exists('noexist.txt');
+		$non_existent_file2 = $this->adapter->exists('3d/8e/file.txt');
+
+		// Then
+		$this->assertTrue($existent_file);
+		$this->assertFalse($non_existent_file);
+		$this->assertFalse($non_existent_file2);
+
+		// Clean test
 		unlink($this->path . '3d/8e/file.txt');
 		rmdir($this->path . '3d/8e');
 		rmdir($this->path . '3d');
 	}
 
-	public function test_delete_file()
+	public function test_delete_file(): void
 	{
+		// Given
 		mkdir($this->path . '3d/8e', 0777, true);
 		touch($this->path . '3d/8e/file.txt');
-		$this->assertTrue(file_exists($this->path . '3d/8e/file.txt'));
+		$this->assertFileExists($this->path . '3d/8e/file.txt');
+
+		// When
 		$this->adapter->delete('file.txt');
-		$this->assertFalse(file_exists($this->path . '3d/8e/file.txt'));
+
-		$this->assertFalse(file_exists($this->path . '3d'));
+		// Then
+		$this->assertFileNotExists($this->path . '3d/8e/file.txt');
+		$this->assertFileNotExists($this->path . '3d');
 	}
 
-	public function test_rename()
+	public function test_rename(): void
 	{
+		// Given
 		mkdir($this->path . '3d/8e', 0777, true);
 		touch($this->path . '3d/8e/file.txt');
+
+		// When
 		$this->adapter->rename('file.txt', 'file2.txt');
-		$this->assertFalse(file_exists($this->path . '3d/8e/file.txt'));
+
-		$this->assertTrue(file_exists($this->path . '27/36/file2.txt'));
+		// Then
-		$this->assertFalse(file_exists($this->path . '3d'));
+		$this->assertFileNotExists($this->path . '3d/8e/file.txt');
+		$this->assertFileExists($this->path . '27/36/file2.txt');
+		$this->assertFileNotExists($this->path . '3d');
+
+		// Clean test
 		unlink($this->path . '27/36/file2.txt');
 		rmdir($this->path . '27/36');
 		rmdir($this->path . '27');
 	}
 
-	public function test_copy()
+	public function test_copy(): void
 	{
+		// Given
 		mkdir($this->path . '3d/8e', 0777, true);
 		file_put_contents($this->path . '3d/8e/file.txt', 'abc');
+
+		// When
 		$this->adapter->copy('file.txt', 'file2.txt');
-		$this->assertEquals(file_get_contents($this->path . '3d/8e/file.txt'), 'abc');
+
-		$this->assertEquals(file_get_contents($this->path . '27/36/file2.txt'), 'abc');
+		// Then
+		$this->assertFileContains($this->path . '3d/8e/file.txt', 'abc');
+		$this->assertFileContains($this->path . '27/36/file2.txt', 'abc');
+
+		// Clean test
 		unlink($this->path . '3d/8e/file.txt');
 		rmdir($this->path . '3d/8e');
 		rmdir($this->path . '3d');
@@ -108,29 +134,42 @@
 		rmdir($this->path . '27');
 	}
 
-	public function test_read_stream()
+	public function test_read_stream(): void
 	{
+		// Given
 		mkdir($this->path . '3d/8e', 0777, true);
 		touch($this->path . '3d/8e/file.txt');
+
+		// When
 		$stream = $this->adapter->read_stream('file.txt');
-		$this->assertTrue(is_resource($stream));
+
+		// Then
+		$this->assertIsResource($stream);
+
+		// Clean test
 		fclose($stream);
 		unlink($this->path . '3d/8e/file.txt');
 		rmdir($this->path . '3d/8e');
 		rmdir($this->path . '3d');
 	}
 
-	public function test_write_stream()
+	public function test_write_stream(): void
 	{
+		// Given
 		file_put_contents($this->path . 'file.txt', 'abc');
 		$stream = fopen($this->path . 'file.txt', 'rb');
+
+		// When
 		$this->adapter->write_stream('file2.txt', $stream);
 		fclose($stream);
-		$this->assertEquals(file_get_contents($this->path . '27/36/file2.txt'), 'abc');
+
+		// Then
+		$this->assertFileContains($this->path . '27/36/file2.txt', 'abc');
+
+		// Clean test
 		unlink($this->path . 'file.txt');
 		unlink($this->path . '27/36/file2.txt');
 		rmdir($this->path . '27/36');
 		rmdir($this->path . '27');
 	}
-
+}
- }

tests/storage/adapter/local_test.php

@@ -11,103 +11,141 @@
  *
  */
 
- class phpbb_storage_adapter_local_test extends phpbb_test_case
+require_once __DIR__ . '/local_test_case.php';
- {
-	protected $adapter;
-
-	protected $path;
-
-	protected $filesystem;
 
+class phpbb_storage_adapter_local_test extends phpbb_local_test_case
+{
 	protected function setUp(): void
 	{
 		parent::setUp();
 
-		$this->filesystem = new \phpbb\filesystem\filesystem();
-		$phpbb_root_path = getcwd() . DIRECTORY_SEPARATOR;
-
-		$this->adapter = new \phpbb\storage\adapter\local($this->filesystem, new \FastImageSize\FastImageSize(), new \phpbb\mimetype\guesser(array(new \phpbb\mimetype\extension_guesser)), $phpbb_root_path);
 		$this->adapter->configure(['path' => 'test_path', 'subfolders' => false]);
-
-		$this->path = $phpbb_root_path . 'test_path/';
-		mkdir($this->path);
 	}
 
-	protected function tearDown(): void
+	public function test_put_contents(): void
-	{
-		$this->adapter = null;
-		rmdir($this->path);
-	}
-
-	public function test_put_contents()
 	{
+		// When
 		$this->adapter->put_contents('file.txt', 'abc');
-		$this->assertTrue(file_exists($this->path . 'file.txt'));
+
-		$this->assertEquals(file_get_contents($this->path . 'file.txt'), 'abc');
+		// Then
+		$this->assertFileExists($this->path . 'file.txt');
+		$this->assertFileContains($this->path . 'file.txt', 'abc');
+
+		// Clean test
 		unlink($this->path . 'file.txt');
 	}
 
-	public function test_get_contents()
+	public function test_get_contents(): void
 	{
+		// Given
 		file_put_contents($this->path . 'file.txt', 'abc');
-		$this->assertEquals($this->adapter->get_contents('file.txt'), 'abc');
+
+		// When
+		$content = $this->adapter->get_contents('file.txt');
+
+		// Then
+		$this->assertEquals('abc', $content);
+
+		// Clean test
 		unlink($this->path . 'file.txt');
 	}
 
-	public function test_exists()
+	public function test_exists(): void
 	{
+		// Given
 		touch($this->path . 'file.txt');
-		$this->assertTrue($this->adapter->exists('file.txt'));
+
-		$this->assertFalse($this->adapter->exists('noexist.txt'));
+		// When
+		$existent_file = $this->adapter->exists('file.txt');
+		$non_existent_file = $this->adapter->exists('noexist.txt');
+
+		// Then
+		$this->assertTrue($existent_file);
+		$this->assertFalse($non_existent_file);
+
+		// Clean test
 		unlink($this->path . 'file.txt');
 	}
 
-	public function test_delete_file()
+	public function test_delete_file(): void
 	{
+		// Given
 		touch($this->path . 'file.txt');
-		$this->assertTrue(file_exists($this->path . 'file.txt'));
+		$this->assertFileExists($this->path . 'file.txt');
+
+		// When
 		$this->adapter->delete('file.txt');
-		$this->assertFalse(file_exists($this->path . 'file.txt'));
+
+		// Then
+		$this->assertFileNotExists($this->path . 'file.txt');
 	}
 
-	public function test_rename()
+	public function test_rename(): void
 	{
+		// Given
 		touch($this->path . 'file.txt');
+		$this->assertFileExists($this->path . 'file.txt');
+		$this->assertFileNotExists($this->path . 'file2.txt');
+
+		// When
 		$this->adapter->rename('file.txt', 'file2.txt');
-		$this->assertFalse(file_exists($this->path . 'file.txt'));
+
-		$this->assertTrue(file_exists($this->path . 'file2.txt'));
+		// Then
-		$this->assertFalse(file_exists($this->path . 'file.txt'));
+		$this->assertFileNotExists($this->path . 'file.txt');
+		$this->assertFileExists($this->path . 'file2.txt');
+
+		// Clean test
 		unlink($this->path . 'file2.txt');
 	}
 
-	public function test_copy()
+	public function test_copy(): void
 	{
+		// Given
 		file_put_contents($this->path . 'file.txt', 'abc');
+
+		// When
 		$this->adapter->copy('file.txt', 'file2.txt');
-		$this->assertEquals(file_get_contents($this->path . 'file.txt'), 'abc');
+
-		$this->assertEquals(file_get_contents($this->path . 'file2.txt'), 'abc');
+		// Then
+		$this->assertFileContains($this->path . 'file.txt', 'abc');
+		$this->assertFileContains($this->path . 'file2.txt', 'abc');
+
+		// Clean test
 		unlink($this->path . 'file.txt');
 		unlink($this->path . 'file2.txt');
 	}
 
 	public function test_read_stream()
 	{
+		// Given
 		touch($this->path . 'file.txt');
+
+		// When
 		$stream = $this->adapter->read_stream('file.txt');
-		$this->assertTrue(is_resource($stream));
+
+		// Then
+		$this->assertIsResource($stream);
+
+		// Clean test
 		fclose($stream);
 		unlink($this->path . 'file.txt');
 	}
 
 	public function test_write_stream()
 	{
+		// Given
 		file_put_contents($this->path . 'file.txt', 'abc');
 		$stream = fopen($this->path . 'file.txt', 'rb');
+
+		// When
 		$this->adapter->write_stream('file2.txt', $stream);
 		fclose($stream);
-		$this->assertEquals(file_get_contents($this->path . 'file2.txt'), 'abc');
+
+		// Then
+		$this->assertFileContains($this->path . 'file2.txt', 'abc');
+
+		// Clean test
 		unlink($this->path . 'file.txt');
 		unlink($this->path . 'file2.txt');
 	}
-
+}
- }

tests/storage/adapter/local_test_case.php

@@ -0,0 +1,63 @@
+<?php
+/**
+ *
+ * This file is part of the phpBB Forum Software package.
+ *
+ * @copyright (c) phpBB Limited <https://www.phpbb.com>
+ * @license GNU General Public License, version 2 (GPL-2.0)
+ *
+ * For full copyright and license information, please see
+ * the docs/CREDITS.txt file.
+ *
+ */
+
+use FastImageSize\FastImageSize;
+use phpbb\mimetype\extension_guesser;
+use phpbb\mimetype\guesser;
+use phpbb\storage\adapter\local;
+
+class phpbb_local_test_case extends phpbb_test_case
+{
+	protected $adapter;
+
+	protected $path;
+
+	protected $filesystem;
+
+	protected function setUp(): void
+	{
+		parent::setUp();
+
+		$this->filesystem = new \phpbb\filesystem\filesystem();
+		$phpbb_root_path = getcwd() . DIRECTORY_SEPARATOR;
+
+		$this->adapter = new local(
+			$this->filesystem,
+			new FastImageSize(),
+			new guesser(array(new extension_guesser)),
+			$phpbb_root_path
+		);
+
+		$this->path = $phpbb_root_path . 'test_path/';
+		mkdir($this->path);
+	}
+
+	protected function tearDown(): void
+	{
+		parent::tearDown();
+
+		$this->adapter = null;
+		rmdir($this->path);
+	}
+
+	/**
+	 * Check if a file contains a string
+	 *
+	 * @param string $file
+	 * @param string $content
+	 */
+	protected function assertFileContains(string $file, string $content): void
+	{
+		$this->assertEquals($content, file_get_contents($file));
+	}
+}