Merge pull request #68 from kasimi/array-cast

Improved error handling when importing invalid & empty YML files
phpbb · Oct 7, 2017 · b05ccb0 · b05ccb0
2 parents d2fc4e0 + 741328f
commit b05ccb0
Show file tree

Hide file tree

Showing 7 changed files with 82 additions and 18 deletions.
diff --git a/src/Files/Type/YmlFile.php b/src/Files/Type/YmlFile.php
@@ -29,6 +29,11 @@ public function __construct($debug, $filename, $rundir)
 		{
 			$content = Yaml::parse($this->fileData);
 
+			if (!is_array($content))
+			{
+				throw new ParseException("Empty file");
+			}
+
 			// Look for imports
 			if (isset($content['imports']) && is_array($content['imports']))
 			{
@@ -40,9 +45,18 @@ public function __construct($debug, $filename, $rundir)
 				{
 					if (isset($import['resource']))
 					{
-						$importYmlFileName = $dirname . '/' . $import['resource'];
-						$importYmlFile = new YmlFile($debug, $importYmlFileName, $rundir);
-						$extraContent = $importYmlFile->getYaml();
+						try
+						{
+							$importYmlFileName = $dirname . '/' . $import['resource'];
+							$importYmlFile = new YmlFile($debug, $importYmlFileName, $rundir);
+							$extraContent = $importYmlFile->getYaml();
+						}
+						catch (FileLoadException $ex)
+						{
+							// The imported yml file will be loaded individually later.
+							// Let's avoid duplicate error messages here and continue with the current yml.
+							$extraContent = array();
+						}
 
 						// Imports are at the top of the yaml file, so these should be loaded first.
 						// The values of the current yaml file will overwrite existing array values of the imports.

diff --git a/tests/file_loader_test.php b/tests/file_loader_test.php
@@ -8,29 +8,62 @@
  *
  */
 
+use Phpbb\Epv\Files\FileLoader;
+use Phpbb\Epv\Files\Type\LangFile;
+use Phpbb\Epv\Files\Type\MigrationFile;
+use Phpbb\Epv\Files\Type\PHPFile;
+use Phpbb\Epv\Files\Type\PHPFileInterface;
+use Phpbb\Epv\Files\Type\YmlFile;
+use Phpbb\Epv\Tests\Mock\Output;
+
 class file_loader_test extends PHPUnit_Framework_TestCase {
+
+	/** @var FileLoader */
+	private static $loader;
+
 	public static function setUpBeforeClass()
 	{
 		require_once('./tests/Mock/Output.php');
+
+		static::$loader = new FileLoader(new Output(), false, 'tests/testFiles/', '.');
 	}
 
-	private function getLoader()
+	public function test_file_php() {
+
+		$type = static::$loader->loadFile('tests/testFiles/test.txt.php');
+		$typePhp = static::$loader->loadFile('tests/testFiles/test.php');
+		$typeMigration = static::$loader->loadFile('tests/testFiles/migrations/test.php');
+
+		$this->assertTrue($type instanceof PHPFile);
+		$this->assertTrue($typePhp instanceof PHPFile);
+		$this->assertFalse($typePhp instanceof MigrationFile);
+		$this->assertFalse($typePhp instanceof LangFile);
+		$this->assertTrue($typeMigration instanceof PHPFileInterface); // It extends from the interface!
+		$this->assertTrue($typeMigration instanceof MigrationFile, 'type is migration file');
+	}
+
+	public function test_file_yml()
 	{
-		return $file = new \Phpbb\Epv\Files\FileLoader(new \Phpbb\Epv\Tests\Mock\Output(), false, 'tests/testFiles/', '.');
+		$validYml = static::$loader->loadFile('tests/testFiles/valid.yml');
+		$invalidImportYml = static::$loader->loadFile('tests/testFiles/invalid_import.yml');
+		$emptyImportYml = static::$loader->loadFile('tests/testFiles/empty_import.yml');
+
+		$this->assertTrue($validYml instanceof YmlFile);
+		$this->assertTrue($invalidImportYml instanceof YmlFile);
+		$this->assertTrue($emptyImportYml instanceof YmlFile);
 	}
 
-	public function test_file_php() {
-		$file = $this->getLoader();
-
-		$type = $file->loadFile('tests/testFiles/test.txt.php');
-		$typePhp = $file->loadFile('tests/testFiles/test.php');
-		$typeMigration = $file->loadFile('tests/testFiles/migrations/test.php');
-
-		$this->assertTrue($type instanceof \Phpbb\Epv\Files\Type\PHPFile);
-		$this->assertTrue($typePhp instanceof \Phpbb\Epv\Files\Type\PHPFile);
-		$this->assertFalse($typePhp instanceof \Phpbb\Epv\Files\Type\MigrationFile);
-		$this->assertFalse($typePhp instanceof \Phpbb\Epv\Files\Type\LangFile);
-		$this->assertTrue($typeMigration instanceof \Phpbb\Epv\Files\Type\PHPFileInterface); // It extends from the interface!
-		$this->assertTrue($typeMigration instanceof \Phpbb\Epv\Files\Type\MigrationFile, 'type is migration file');
+	public function test_file_invalid_yml()
+	{
+		$this->setExpectedException(Exception::class);
+		$invalidYml = static::$loader->loadFile('tests/testFiles/invalid.yml');
+		$this->assertNull($invalidYml);
+	}
+
+	public function test_file_empty_yml()
+	{
+		$this->setExpectedException(Exception::class);
+		$emptyYml = static::$loader->loadFile('tests/testFiles/empty.yml');
+		$this->assertNull($emptyYml);
 	}
 }
diff --git a/tests/testFiles/empty.yml b/tests/testFiles/empty.yml
diff --git a/tests/testFiles/empty_import.yml b/tests/testFiles/empty_import.yml
@@ -0,0 +1,6 @@
+imports:
+    - { resource: empty.yml }
+
+services:
+    some.service.name:
+        class: a\b\c
diff --git a/tests/testFiles/invalid.yml b/tests/testFiles/invalid.yml
@@ -0,0 +1,2 @@
+services:
+	some.
diff --git a/tests/testFiles/invalid_import.yml b/tests/testFiles/invalid_import.yml
@@ -0,0 +1,6 @@
+imports:
+    - { resource: invalid.yml }
+
+services:
+    some.service.name:
+        class: a\b\c
diff --git a/tests/testFiles/valid.yml b/tests/testFiles/valid.yml
@@ -0,0 +1,3 @@
+services:
+    some.service.name:
+        class: a\b\c