Refactor readers for better di (#457)

* Refactor readers to get a proper DI

Similar to what was done with writers, readers also needed to be updated to match the new way of doing things.
This commits promotes a better DI (factories, injection through constructors).

* Escapers should not be singletons

Instead, they should be proper object that can be injected where needed.

tests/Spout/Reader/CSV/ReaderTest.php

@@ -2,9 +2,14 @@
 
 namespace Box\Spout\Reader\CSV;
 
+use Box\Spout\Common\Creator\HelperFactory;
+use Box\Spout\Common\Helper\GlobalFunctionsHelper;
+use Box\Spout\Reader\CSV\Creator\EntityFactory;
+use Box\Spout\Reader\CSV\Manager\OptionsManager;
 use Box\Spout\Reader\ReaderFactory;
 use Box\Spout\Common\Type;
 use Box\Spout\Common\Helper\EncodingHelper;
+use Box\Spout\Reader\ReaderInterface;
 use Box\Spout\TestUsingResource;
 
 /**
@@ -23,7 +28,7 @@ class ReaderTest extends \PHPUnit_Framework_TestCase
      */
     public function testOpenShouldThrowExceptionIfFileDoesNotExist()
     {
-        ReaderFactory::create(Type::CSV)->open('/path/to/fake/file.csv');
+        $this->createCSVReader()->open('/path/to/fake/file.csv');
     }
 
     /**
@@ -33,7 +38,7 @@ class ReaderTest extends \PHPUnit_Framework_TestCase
      */
     public function testOpenShouldThrowExceptionIfTryingToReadBeforeOpeningReader()
     {
-        ReaderFactory::create(Type::CSV)->getSheetIterator();
+        $this->createCSVReader()->getSheetIterator();
     }
 
     /**
@@ -43,6 +48,7 @@ class ReaderTest extends \PHPUnit_Framework_TestCase
      */
     public function testOpenShouldThrowExceptionIfFileNotReadable()
     {
+        /** @var \Box\Spout\Common\Helper\GlobalFunctionsHelper|\PHPUnit_Framework_MockObject_MockObject $helperStub */
         $helperStub = $this->getMockBuilder('\Box\Spout\Common\Helper\GlobalFunctionsHelper')
                         ->setMethods(['is_readable'])
                         ->getMock();
@@ -50,8 +56,7 @@ class ReaderTest extends \PHPUnit_Framework_TestCase
 
         $resourcePath = $this->getResourcePath('csv_standard.csv');
 
-        $reader = ReaderFactory::create(Type::CSV);
-        $reader->setGlobalFunctionsHelper($helperStub);
+        $reader = $this->createCSVReader(null, $helperStub);
         $reader->open($resourcePath);
     }
 
@@ -62,6 +67,7 @@ class ReaderTest extends \PHPUnit_Framework_TestCase
      */
     public function testOpenShouldThrowExceptionIfCannotOpenFile()
     {
+        /** @var \Box\Spout\Common\Helper\GlobalFunctionsHelper|\PHPUnit_Framework_MockObject_MockObject $helperStub */
         $helperStub = $this->getMockBuilder('\Box\Spout\Common\Helper\GlobalFunctionsHelper')
                         ->setMethods(['fopen'])
                         ->getMock();
@@ -69,8 +75,7 @@ class ReaderTest extends \PHPUnit_Framework_TestCase
 
         $resourcePath = $this->getResourcePath('csv_standard.csv');
 
-        $reader = ReaderFactory::create(Type::CSV);
-        $reader->setGlobalFunctionsHelper($helperStub);
+        $reader = $this->createCSVReader(null, $helperStub);
         $reader->open($resourcePath);
     }
 
@@ -139,7 +144,7 @@ class ReaderTest extends \PHPUnit_Framework_TestCase
     {
         $allRows = $this->getAllRowsForFile(
             'csv_with_multiple_empty_lines.csv',
-            ',', '"', "\n", EncodingHelper::ENCODING_UTF8,
+            ',', '"', EncodingHelper::ENCODING_UTF8,
             $shouldPreserveEmptyRows = true
         );
 
@@ -215,21 +220,6 @@ class ReaderTest extends \PHPUnit_Framework_TestCase
         $this->assertEquals('This is, a comma', $allRows[0][0]);
     }
 
-    /**
-     * @return void
-     */
-    public function testReadCustomEOLs()
-    {
-        $allRows = $this->getAllRowsForFile('csv_with_CR_EOL.csv', ',', '"', "\r");
-
-        $expectedRows = [
-            ['csv--11', 'csv--12', 'csv--13'],
-            ['csv--21', 'csv--22', 'csv--23'],
-            ['csv--31', 'csv--32', 'csv--33'],
-        ];
-        $this->assertEquals($expectedRows, $allRows);
-    }
-
     /**
      * @return void
      */
@@ -262,7 +252,7 @@ class ReaderTest extends \PHPUnit_Framework_TestCase
      */
     public function testReadShouldSkipBom($fileName, $fileEncoding)
     {
-        $allRows = $this->getAllRowsForFile($fileName, ',', '"', "\n", $fileEncoding);
+        $allRows = $this->getAllRowsForFile($fileName, ',', '"', $fileEncoding);
 
         $expectedRows = [
             ['csv--11', 'csv--12', 'csv--13'],
@@ -313,9 +303,8 @@ class ReaderTest extends \PHPUnit_Framework_TestCase
         $helperStub->method('function_exists')->will($this->returnValueMap($returnValueMap));
 
         /** @var \Box\Spout\Reader\CSV\Reader $reader */
-        $reader = ReaderFactory::create(Type::CSV);
+        $reader = $this->createCSVReader(null, $helperStub);
         $reader
-            ->setGlobalFunctionsHelper($helperStub)
             ->setEncoding($fileEncoding)
             ->open($resourcePath);
 
@@ -343,7 +332,7 @@ class ReaderTest extends \PHPUnit_Framework_TestCase
         $allRows = [];
         $resourcePath = $this->getResourcePath('csv_standard.csv');
 
-        $reader = ReaderFactory::create(Type::CSV);
+        $reader = $this->createCSVReader();
         $reader->open($resourcePath);
 
         foreach ($reader->getSheetIterator() as $sheet) {
@@ -428,47 +417,6 @@ class ReaderTest extends \PHPUnit_Framework_TestCase
         $this->assertEquals($expectedRows, $allRows, 'Cell values should not be trimmed');
     }
 
-    /**
-     * @param string $fileName
-     * @param string|void $fieldDelimiter
-     * @param string|void $fieldEnclosure
-     * @param string|void $endOfLineCharacter
-     * @param string|void $encoding
-     * @param bool|void $shouldPreserveEmptyRows
-     * @return array All the read rows the given file
-     */
-    private function getAllRowsForFile(
-        $fileName,
-        $fieldDelimiter = ',',
-        $fieldEnclosure = '"',
-        $endOfLineCharacter = "\n",
-        $encoding = EncodingHelper::ENCODING_UTF8,
-        $shouldPreserveEmptyRows = false)
-    {
-        $allRows = [];
-        $resourcePath = $this->getResourcePath($fileName);
-
-        /** @var \Box\Spout\Reader\CSV\Reader $reader */
-        $reader = ReaderFactory::create(Type::CSV);
-        $reader
-            ->setFieldDelimiter($fieldDelimiter)
-            ->setFieldEnclosure($fieldEnclosure)
-            ->setEndOfLineCharacter($endOfLineCharacter)
-            ->setEncoding($encoding)
-            ->setShouldPreserveEmptyRows($shouldPreserveEmptyRows)
-            ->open($resourcePath);
-
-        foreach ($reader->getSheetIterator() as $sheetIndex => $sheet) {
-            foreach ($sheet->getRowIterator() as $rowIndex => $row) {
-                $allRows[] = $row;
-            }
-        }
-
-        $reader->close();
-
-        return $allRows;
-    }
-
     /**
      * @return void
      */
@@ -481,7 +429,7 @@ class ReaderTest extends \PHPUnit_Framework_TestCase
         stream_wrapper_register('spout', SpoutTestStream::CLASS_NAME);
 
         /** @var \Box\Spout\Reader\CSV\Reader $reader */
-        $reader = ReaderFactory::create(Type::CSV);
+        $reader = $this->createCSVReader();
         $reader->open($resourcePath);
 
         foreach ($reader->getSheetIterator() as $sheet) {
@@ -511,8 +459,60 @@ class ReaderTest extends \PHPUnit_Framework_TestCase
     public function testReadWithUnsupportedCustomStreamWrapper()
     {
         /** @var \Box\Spout\Reader\CSV\Reader $reader */
-        $reader = ReaderFactory::create(Type::CSV);
+        $reader = $this->createCSVReader();
         $reader->open('unsupported://foobar');
     }
 
+    /**
+     * @param \Box\Spout\Common\Helper\GlobalFunctionsHelper|null $optionsManager
+     * @param \Box\Spout\Common\Manager\OptionsManagerInterface|null $globalFunctionsHelper
+     * @return ReaderInterface
+     */
+    private function createCSVReader($optionsManager = null, $globalFunctionsHelper = null)
+    {
+        $optionsManager = $optionsManager ?: new OptionsManager();
+        $globalFunctionsHelper = $globalFunctionsHelper ?: new GlobalFunctionsHelper();
+        $entityFactory = new EntityFactory(new HelperFactory());
+
+        return new Reader($optionsManager, $globalFunctionsHelper, $entityFactory);
+    }
+
+    /**
+     * @param string $fileName
+     * @param string|void $fieldDelimiter
+     * @param string|void $fieldEnclosure
+     * @param string|void $encoding
+     * @param bool|void $shouldPreserveEmptyRows
+     * @return array All the read rows the given file
+     */
+    private function getAllRowsForFile(
+        $fileName,
+        $fieldDelimiter = ',',
+        $fieldEnclosure = '"',
+        $encoding = EncodingHelper::ENCODING_UTF8,
+        $shouldPreserveEmptyRows = false)
+    {
+        $allRows = [];
+        $resourcePath = $this->getResourcePath($fileName);
+
+        /** @var \Box\Spout\Reader\CSV\Reader $reader */
+        $reader = $this->createCSVReader();
+        $reader
+            ->setFieldDelimiter($fieldDelimiter)
+            ->setFieldEnclosure($fieldEnclosure)
+            ->setEncoding($encoding)
+            ->setShouldPreserveEmptyRows($shouldPreserveEmptyRows)
+            ->open($resourcePath);
+
+        foreach ($reader->getSheetIterator() as $sheetIndex => $sheet) {
+            foreach ($sheet->getRowIterator() as $rowIndex => $row) {
+                $allRows[] = $row;
+            }
+        }
+
+        $reader->close();
+
+        return $allRows;
+    }
+
 }

tests/Spout/Reader/XLSX/Helper/CellValueFormatterTest.php

@@ -2,6 +2,8 @@
 
 namespace Box\Spout\Reader\XLSX\Helper;
 
+use Box\Spout\Common\Helper\Escaper;
+
 /**
  * Class CellValueFormatterTest
  *
@@ -71,7 +73,7 @@ class CellValueFormatterTest extends \PHPUnit_Framework_TestCase
             ->with(123)
             ->will($this->returnValue(true));
 
-        $formatter = new CellValueFormatter(null, $styleHelperMock, false);
+        $formatter = new CellValueFormatter(null, $styleHelperMock, false, new Escaper\XLSX());
         $result = $formatter->extractAndFormatNodeValue($nodeMock);
 
         if ($expectedDateAsString === null) {
@@ -124,7 +126,7 @@ class CellValueFormatterTest extends \PHPUnit_Framework_TestCase
             ->method('shouldFormatNumericValueAsDate')
             ->will($this->returnValue(false));
 
-        $formatter = new CellValueFormatter(null, $styleHelperMock, false);
+        $formatter = new CellValueFormatter(null, $styleHelperMock, false, new Escaper\XLSX());
         $formattedValue = \ReflectionHelper::callMethodOnObject($formatter, 'formatNumericCellValue', $value, 0);
 
         $this->assertEquals($expectedFormattedValue, $formattedValue);
@@ -167,7 +169,7 @@ class CellValueFormatterTest extends \PHPUnit_Framework_TestCase
             ->with(CellValueFormatter::XML_NODE_INLINE_STRING_VALUE)
             ->will($this->returnValue($nodeListMock));
 
-        $formatter = new CellValueFormatter(null, null, false);
+        $formatter = new CellValueFormatter(null, null, false, new Escaper\XLSX());
         $formattedValue = \ReflectionHelper::callMethodOnObject($formatter, 'formatInlineStringCellValue', $nodeMock);
 
         $this->assertEquals($expectedFormattedValue, $formattedValue);

tests/Spout/Reader/XLSX/Helper/SharedStringsCaching/CachingStrategyFactoryTest.php

@@ -1,6 +1,7 @@
 <?php
 
 namespace Box\Spout\Reader\XLSX\Helper\SharedStringsCaching;
+use Box\Spout\Reader\XLSX\Creator\HelperFactory;
 
 /**
  * Class CachingStrategyFactoryTest
@@ -12,7 +13,7 @@ class CachingStrategyFactoryTest extends \PHPUnit_Framework_TestCase
     /**
      * @return array
      */
-    public function dataProviderForTestGetBestCachingStrategy()
+    public function dataProviderForTestCreateBestCachingStrategy()
     {
         return [
             [null, -1, 'FileBasedStrategy'],
@@ -26,14 +27,14 @@ class CachingStrategyFactoryTest extends \PHPUnit_Framework_TestCase
     }
 
     /**
-     * @dataProvider dataProviderForTestGetBestCachingStrategy
+     * @dataProvider dataProviderForTestCreateBestCachingStrategy
      *
      * @param int|null $sharedStringsUniqueCount
      * @param int $memoryLimitInKB
      * @param string $expectedStrategyClassName
      * @return void
      */
-    public function testGetBestCachingStrategy($sharedStringsUniqueCount, $memoryLimitInKB, $expectedStrategyClassName)
+    public function testCreateBestCachingStrategy($sharedStringsUniqueCount, $memoryLimitInKB, $expectedStrategyClassName)
     {
         /** @var CachingStrategyFactory|\PHPUnit_Framework_MockObject_MockObject $factoryStub */
         $factoryStub = $this
@@ -44,15 +45,14 @@ class CachingStrategyFactoryTest extends \PHPUnit_Framework_TestCase
 
         $factoryStub->method('getMemoryLimitInKB')->willReturn($memoryLimitInKB);
 
-        \ReflectionHelper::setStaticValue('\Box\Spout\Reader\XLSX\Helper\SharedStringsCaching\CachingStrategyFactory', 'instance', $factoryStub);
-
-        $strategy = $factoryStub->getBestCachingStrategy($sharedStringsUniqueCount, null);
+        $tempFolder = sys_get_temp_dir();
+        $helperFactory = new HelperFactory($factoryStub);
+        $strategy = $factoryStub->createBestCachingStrategy($sharedStringsUniqueCount, $tempFolder, $helperFactory);
 
         $fullExpectedStrategyClassName = 'Box\Spout\Reader\XLSX\Helper\SharedStringsCaching\\' . $expectedStrategyClassName;
         $this->assertEquals($fullExpectedStrategyClassName, get_class($strategy));
 
         $strategy->clearCache();
-        \ReflectionHelper::reset();
     }
 
     /**

tests/Spout/Reader/XLSX/Helper/SharedStringsHelperTest.php

@@ -2,6 +2,8 @@
 
 namespace Box\Spout\Reader\XLSX\Helper;
 
+use Box\Spout\Reader\XLSX\Creator\EntityFactory;
+use Box\Spout\Reader\XLSX\Creator\HelperFactory;
 use Box\Spout\Reader\XLSX\Helper\SharedStringsCaching\CachingStrategyFactory;
 use Box\Spout\Reader\XLSX\Helper\SharedStringsCaching\FileBasedStrategy;
 use Box\Spout\Reader\XLSX\Helper\SharedStringsCaching\InMemoryStrategy;
@@ -24,8 +26,7 @@ class SharedStringsHelperTest extends \PHPUnit_Framework_TestCase
      */
     public function setUp()
     {
-        $resourcePath = $this->getResourcePath('one_sheet_with_shared_strings.xlsx');
-        $this->sharedStringsHelper = new SharedStringsHelper($resourcePath);
+        $this->sharedStringsHelper = null;
     }
 
     /**
@@ -33,7 +34,26 @@ class SharedStringsHelperTest extends \PHPUnit_Framework_TestCase
      */
     public function tearDown()
     {
-        $this->sharedStringsHelper->cleanup();
+        if ($this->sharedStringsHelper !== null) {
+            $this->sharedStringsHelper->cleanup();
+        }
+    }
+
+    /**
+     * @param string $resourceName
+     * @return SharedStringsHelper
+     */
+    private function createSharedStringsHelper($resourceName = 'one_sheet_with_shared_strings.xlsx')
+    {
+        $resourcePath = $this->getResourcePath($resourceName);
+        $tempFolder = sys_get_temp_dir();
+        $cachingStrategyFactory = new CachingStrategyFactory();
+        $helperFactory = new HelperFactory($cachingStrategyFactory);
+        $entityFactory = new EntityFactory($helperFactory);
+
+        $this->sharedStringsHelper = new SharedStringsHelper($resourcePath, $tempFolder, $entityFactory, $helperFactory, $cachingStrategyFactory);
+
+        return $this->sharedStringsHelper;
     }
 
     /**
@@ -42,8 +62,9 @@ class SharedStringsHelperTest extends \PHPUnit_Framework_TestCase
      */
     public function testGetStringAtIndexShouldThrowExceptionIfStringNotFound()
     {
-        $this->sharedStringsHelper->extractSharedStrings();
-        $this->sharedStringsHelper->getStringAtIndex(PHP_INT_MAX);
+        $sharedStringsHelper = $this->createSharedStringsHelper();
+        $sharedStringsHelper->extractSharedStrings();
+        $sharedStringsHelper->getStringAtIndex(PHP_INT_MAX);
     }
 
     /**
@@ -51,15 +72,16 @@ class SharedStringsHelperTest extends \PHPUnit_Framework_TestCase
      */
     public function testGetStringAtIndexShouldReturnTheCorrectStringIfFound()
     {
-        $this->sharedStringsHelper->extractSharedStrings();
+        $sharedStringsHelper = $this->createSharedStringsHelper();
+        $sharedStringsHelper->extractSharedStrings();
 
-        $sharedString = $this->sharedStringsHelper->getStringAtIndex(0);
+        $sharedString = $sharedStringsHelper->getStringAtIndex(0);
         $this->assertEquals('s1--A1', $sharedString);
 
-        $sharedString = $this->sharedStringsHelper->getStringAtIndex(24);
+        $sharedString = $sharedStringsHelper->getStringAtIndex(24);
         $this->assertEquals('s1--E5', $sharedString);
 
-        $usedCachingStrategy = \ReflectionHelper::getValueOnObject($this->sharedStringsHelper, 'cachingStrategy');
+        $usedCachingStrategy = \ReflectionHelper::getValueOnObject($sharedStringsHelper, 'cachingStrategy');
         $this->assertTrue($usedCachingStrategy instanceof InMemoryStrategy);
     }
 
@@ -68,8 +90,7 @@ class SharedStringsHelperTest extends \PHPUnit_Framework_TestCase
      */
     public function testGetStringAtIndexShouldWorkWithMultilineStrings()
     {
-        $resourcePath = $this->getResourcePath('one_sheet_with_shared_multiline_strings.xlsx');
-        $sharedStringsHelper = new SharedStringsHelper($resourcePath);
+        $sharedStringsHelper = $this->createSharedStringsHelper('one_sheet_with_shared_multiline_strings.xlsx');
 
         $sharedStringsHelper->extractSharedStrings();
 
@@ -78,8 +99,6 @@ class SharedStringsHelperTest extends \PHPUnit_Framework_TestCase
 
         $sharedString = $sharedStringsHelper->getStringAtIndex(24);
         $this->assertEquals("s1\nE5", $sharedString);
-
-        $sharedStringsHelper->cleanup();
     }
 
     /**
@@ -87,15 +106,12 @@ class SharedStringsHelperTest extends \PHPUnit_Framework_TestCase
      */
     public function testGetStringAtIndexShouldWorkWithStringsContainingTextAndHyperlinkInSameCell()
     {
-        $resourcePath = $this->getResourcePath('one_sheet_with_shared_strings_containing_text_and_hyperlink_in_same_cell.xlsx');
-        $sharedStringsHelper = new SharedStringsHelper($resourcePath);
+        $sharedStringsHelper = $this->createSharedStringsHelper('one_sheet_with_shared_strings_containing_text_and_hyperlink_in_same_cell.xlsx');
 
         $sharedStringsHelper->extractSharedStrings();
 
         $sharedString = $sharedStringsHelper->getStringAtIndex(0);
         $this->assertEquals('go to https://github.com please', $sharedString);
-
-        $sharedStringsHelper->cleanup();
     }
 
     /**
@@ -103,15 +119,12 @@ class SharedStringsHelperTest extends \PHPUnit_Framework_TestCase
      */
     public function testGetStringAtIndexShouldNotDoubleDecodeHTMLEntities()
     {
-        $resourcePath = $this->getResourcePath('one_sheet_with_pre_encoded_html_entities.xlsx');
-        $sharedStringsHelper = new SharedStringsHelper($resourcePath);
+        $sharedStringsHelper = $this->createSharedStringsHelper('one_sheet_with_pre_encoded_html_entities.xlsx');
 
         $sharedStringsHelper->extractSharedStrings();
 
         $sharedString = $sharedStringsHelper->getStringAtIndex(0);
         $this->assertEquals('quote: " - ampersand: &', $sharedString);
-
-        $sharedStringsHelper->cleanup();
     }
 
     /**
@@ -123,8 +136,7 @@ class SharedStringsHelperTest extends \PHPUnit_Framework_TestCase
         $originalMemoryLimit = ini_get('memory_limit');
         ini_set('memory_limit', '-1');
 
-        $resourcePath = $this->getResourcePath('sheet_with_lots_of_shared_strings.xlsx');
-        $sharedStringsHelper = new SharedStringsHelper($resourcePath);
+        $sharedStringsHelper = $this->createSharedStringsHelper('sheet_with_lots_of_shared_strings.xlsx');
 
         $sharedStringsHelper->extractSharedStrings();
 
@@ -137,8 +149,6 @@ class SharedStringsHelperTest extends \PHPUnit_Framework_TestCase
         $usedCachingStrategy = \ReflectionHelper::getValueOnObject($sharedStringsHelper, 'cachingStrategy');
         $this->assertTrue($usedCachingStrategy instanceof FileBasedStrategy);
 
-        $sharedStringsHelper->cleanup();
-
         ini_set('memory_limit', $originalMemoryLimit);
     }
 }

tests/Spout/Reader/XLSX/Helper/StyleHelperTest.php

@@ -1,6 +1,9 @@
 <?php
 
 namespace Box\Spout\Reader\XLSX\Helper;
+use Box\Spout\Reader\XLSX\Creator\EntityFactory;
+use Box\Spout\Reader\XLSX\Creator\HelperFactory;
+use Box\Spout\Reader\XLSX\Helper\SharedStringsCaching\CachingStrategyFactory;
 
 /**
  * Class StyleManagerTest
@@ -17,9 +20,11 @@ class StyleHelperTest extends \PHPUnit_Framework_TestCase
      */
     private function getStyleHelperMock($styleAttributes = [], $customNumberFormats = [])
     {
+        $entityFactory = new EntityFactory(new HelperFactory(new CachingStrategyFactory()));
+
         /** @var StyleHelper $styleHelper */
         $styleHelper = $this->getMockBuilder('\Box\Spout\Reader\XLSX\Helper\StyleHelper')
-                            ->setConstructorArgs(['/path/to/file.xlsx'])
+                            ->setConstructorArgs(['/path/to/file.xlsx', $entityFactory])
                             ->setMethods(['getCustomNumberFormats', 'getStylesAttributes'])
                             ->getMock();