From e9a08933e8e450a9f2fbac29f3bad709b96fc3af Mon Sep 17 00:00:00 2001 From: oleibman <10341515+oleibman@users.noreply.github.com> Date: Wed, 12 Feb 2025 20:04:40 -0800 Subject: [PATCH 1/4] IOFactory::identify and Custom Reader/Writer Fix #4357.`identify` returns a type, not a class name, and this result is not always usable for createReader. The docs say that it is usable in that manner. Changing the behavior of `identify` would be a breaking change, but adding an optional parameter (defaulting to current behavior) allowing it to return a class name rather than a type would not. This PR adds that parameter and updates the documentation to suggest its use for the code in question. To complete this change, `createReader` now accepts either a type (current behavior) or class name (new). Although it is not particularly identified as a possible problem by the issue, `createWriter` is similarly changed for consistency's sake. --- docs/topics/reading-files.md | 12 ++- src/PhpSpreadsheet/IOFactory.php | 36 +++++--- tests/PhpSpreadsheetTests/CustomReader.php | 12 +++ tests/PhpSpreadsheetTests/CustomWriter.php | 12 +++ .../IOFactoryRegisterTest.php | 88 +++++++++++++++++++ tests/PhpSpreadsheetTests/IOFactoryTest.php | 40 ++------- 6 files changed, 152 insertions(+), 48 deletions(-) create mode 100644 tests/PhpSpreadsheetTests/CustomReader.php create mode 100644 tests/PhpSpreadsheetTests/CustomWriter.php create mode 100644 tests/PhpSpreadsheetTests/IOFactoryRegisterTest.php diff --git a/docs/topics/reading-files.md b/docs/topics/reading-files.md index 4aba019ff7..9b80f5b0d1 100644 --- a/docs/topics/reading-files.md +++ b/docs/topics/reading-files.md @@ -123,7 +123,10 @@ method to identify the reader that you need, before using the ```php $inputFileName = './sampleData/example1.xls'; -/** Identify the type of $inputFileName **/ +/** + * Identify the type of $inputFileName. + * See below for a possible improvement for release 4.1.0+. + */ $inputFileType = \PhpOffice\PhpSpreadsheet\IOFactory::identify($inputFileName); /** Create a new Reader of the type that has been identified **/ $reader = \PhpOffice\PhpSpreadsheet\IOFactory::createReader($inputFileType); @@ -134,6 +137,13 @@ $spreadsheet = $reader->load($inputFileName); See `samples/Reader/04_Simple_file_reader_using_the_IOFactory_to_identify_a_reader_to_use.php` for a working example of this code. +Prior to release 4.1.0, `identify` returns a file type. +It may be more useful to return a fully-qualified class name, +which can be accomplished using a parameter introduced in 4.1.0: +```php +$inputFileType = \PhpOffice\PhpSpreadsheet\IOFactory::identify($inputFileName, null, true); +``` + As with the IOFactory `load()` method, you can also pass an array of formats for the `identify()` method to check against if you know that it will only be in a subset of the possible formats that PhpSpreadsheet supports. diff --git a/src/PhpSpreadsheet/IOFactory.php b/src/PhpSpreadsheet/IOFactory.php index db8776db61..73734fc702 100644 --- a/src/PhpSpreadsheet/IOFactory.php +++ b/src/PhpSpreadsheet/IOFactory.php @@ -59,12 +59,16 @@ abstract class IOFactory */ public static function createWriter(Spreadsheet $spreadsheet, string $writerType): IWriter { - if (!isset(self::$writers[$writerType])) { - throw new Writer\Exception("No writer found for type $writerType"); - } + /** @var class-string */ + $className = $writerType; + if (!in_array($writerType, self::$writers, true)) { + if (!isset(self::$writers[$writerType])) { + throw new Writer\Exception("No writer found for type $writerType"); + } - // Instantiate writer - $className = self::$writers[$writerType]; + // Instantiate writer + $className = self::$writers[$writerType]; + } return new $className($spreadsheet); } @@ -74,12 +78,16 @@ public static function createWriter(Spreadsheet $spreadsheet, string $writerType */ public static function createReader(string $readerType): IReader { - if (!isset(self::$readers[$readerType])) { - throw new Reader\Exception("No reader found for type $readerType"); - } + /** @var class-string */ + $className = $readerType; + if (!in_array($readerType, self::$readers, true)) { + if (!isset(self::$readers[$readerType])) { + throw new Reader\Exception("No reader found for type $readerType"); + } - // Instantiate reader - $className = self::$readers[$readerType]; + // Instantiate reader + $className = self::$readers[$readerType]; + } return new $className(); } @@ -109,12 +117,14 @@ public static function load(string $filename, int $flags = 0, ?array $readers = /** * Identify file type using automatic IReader resolution. */ - public static function identify(string $filename, ?array $readers = null): string + public static function identify(string $filename, ?array $readers = null, bool $fullClassName = false): string { $reader = self::createReaderForFile($filename, $readers); $className = $reader::class; + if ($fullClassName) { + return $className; + } $classType = explode('\\', $className); - unset($reader); return array_pop($classType); } @@ -224,6 +234,8 @@ public static function registerWriter(string $writerType, string $writerClass): /** * Register a reader with its type and class name. + * + * @param class-string $readerClass */ public static function registerReader(string $readerType, string $readerClass): void { diff --git a/tests/PhpSpreadsheetTests/CustomReader.php b/tests/PhpSpreadsheetTests/CustomReader.php new file mode 100644 index 0000000000..9d48ae8ef9 --- /dev/null +++ b/tests/PhpSpreadsheetTests/CustomReader.php @@ -0,0 +1,12 @@ +expectException(Writer\Exception::class); + $this->expectExceptionMessage('writers must implement'); + IOFactory::registerWriter('foo', 'bar'); // @phpstan-ignore-line + } + + public function testRegisterInvalidReader(): void + { + $this->expectException(ReaderException::class); + $this->expectExceptionMessage('readers must implement'); + IOFactory::registerReader('foo', 'bar'); // @phpstan-ignore-line + } + + public static function testRegisterCustomReader(): void + { + IOFactory::registerReader(IOFactory::READER_XLSX, CustomReader::class); + $inputFileName = 'tests/data/Reader/Xlsx/1900_Calendar.xlsx'; + $loadSpreadsheet = IOFactory::load($inputFileName); + $sheet = $loadSpreadsheet->getActiveSheet(); + self::assertSame('2022-01-01', $sheet->getCell('A1')->getFormattedValue()); + $loadSpreadsheet->disconnectWorksheets(); + + $reader = new CustomReader(); + $newSpreadsheet = $reader->load($inputFileName); + $newSheet = $newSpreadsheet->getActiveSheet(); + self::assertSame('2022-01-01', $newSheet->getCell('A1')->getFormattedValue()); + $newSpreadsheet->disconnectWorksheets(); + + $inputFileType = IOFactory::identify($inputFileName, null, true); + $objReader = IOFactory::createReader($inputFileType); + self::assertInstanceOf(CustomReader::class, $objReader); + $objSpreadsheet = $objReader->load($inputFileName); + $objSheet = $objSpreadsheet->getActiveSheet(); + self::assertSame('2022-01-01', $objSheet->getCell('A1')->getFormattedValue()); + $objSpreadsheet->disconnectWorksheets(); + } + + public static function testRegisterCustomWriter(): void + { + $spreadsheet = new Spreadsheet(); + $sheet = $spreadsheet->getActiveSheet(); + $sheet->setCellValue('A1', 1); + $writer = new CustomWriter($spreadsheet); + $html = $writer->generateHtmlAll(); + self::assertStringContainsString('1', $html); + IOFactory::registerWriter(IOFactory::WRITER_HTML, CustomWriter::class); + $objWriter = IOFactory::createWriter($spreadsheet, CustomWriter::class); + self::assertInstanceOf(CustomWriter::class, $objWriter); + self::assertTrue(method_exists($objWriter, 'generateHtmlAll')); + $html2 = $objWriter->generateHtmlAll(); + self::assertStringContainsString('1', $html2); + $spreadsheet->disconnectWorksheets(); + } +} diff --git a/tests/PhpSpreadsheetTests/IOFactoryTest.php b/tests/PhpSpreadsheetTests/IOFactoryTest.php index 86244e7de5..c1ad84069c 100644 --- a/tests/PhpSpreadsheetTests/IOFactoryTest.php +++ b/tests/PhpSpreadsheetTests/IOFactoryTest.php @@ -9,11 +9,12 @@ use PhpOffice\PhpSpreadsheet\Reader\Exception as ReaderException; use PhpOffice\PhpSpreadsheet\Spreadsheet; use PhpOffice\PhpSpreadsheet\Writer; +use PHPUnit\Framework\Attributes\DataProvider; use PHPUnit\Framework\TestCase; class IOFactoryTest extends TestCase { - #[\PHPUnit\Framework\Attributes\DataProvider('providerCreateWriter')] + #[DataProvider('providerCreateWriter')] public function testCreateWriter(string $name, string $expected): void { $spreadsheet = new Spreadsheet(); @@ -35,15 +36,7 @@ public static function providerCreateWriter(): array ]; } - public function testRegisterWriter(): void - { - IOFactory::registerWriter('Pdf', Writer\Pdf\Mpdf::class); - $spreadsheet = new Spreadsheet(); - $actual = IOFactory::createWriter($spreadsheet, 'Pdf'); - self::assertInstanceOf(Writer\Pdf\Mpdf::class, $actual); - } - - #[\PHPUnit\Framework\Attributes\DataProvider('providerCreateReader')] + #[DataProvider('providerCreateReader')] public function testCreateReader(string $name, string $expected): void { $actual = IOFactory::createReader($name); @@ -64,22 +57,14 @@ public static function providerCreateReader(): array ]; } - public function testRegisterReader(): void - { - IOFactory::registerReader('Custom', Reader\Html::class); - $actual = IOFactory::createReader('Custom'); - self::assertInstanceOf(Reader\Html::class, $actual); - } - - #[\PHPUnit\Framework\Attributes\DataProvider('providerIdentify')] + #[DataProvider('providerIdentify')] public function testIdentifyCreateLoad(string $file, string $expectedName, string $expectedClass): void { $actual = IOFactory::identify($file); self::assertSame($expectedName, $actual); $actual = IOFactory::createReaderForFile($file); self::assertSame($expectedClass, $actual::class); - $actual = IOFactory::load($file); - self::assertInstanceOf(Spreadsheet::class, $actual); + IOFactory::load($file); } public static function providerIdentify(): array @@ -159,21 +144,6 @@ public function testIdentifyExistingDirectoryThrowExceptions(): void IOFactory::identify('.'); } - public function testRegisterInvalidWriter(): void - { - $this->expectException(Writer\Exception::class); - - // @phpstan-ignore-next-line - IOFactory::registerWriter('foo', 'bar'); - } - - public function testRegisterInvalidReader(): void - { - $this->expectException(ReaderException::class); - - IOFactory::registerReader('foo', 'bar'); - } - public function testCreateInvalidWriter(): void { $this->expectException(Writer\Exception::class); From 1e1ba2090c9ff44bba6bb5b446c8b87c7013bf82 Mon Sep 17 00:00:00 2001 From: oleibman <10341515+oleibman@users.noreply.github.com> Date: Wed, 12 Feb 2025 20:12:26 -0800 Subject: [PATCH 2/4] Wrong Case in File Name --- tests/PhpSpreadsheetTests/IOFactoryRegisterTest.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/PhpSpreadsheetTests/IOFactoryRegisterTest.php b/tests/PhpSpreadsheetTests/IOFactoryRegisterTest.php index 31f3ceedb6..ceb3797ef2 100644 --- a/tests/PhpSpreadsheetTests/IOFactoryRegisterTest.php +++ b/tests/PhpSpreadsheetTests/IOFactoryRegisterTest.php @@ -48,7 +48,7 @@ public function testRegisterInvalidReader(): void public static function testRegisterCustomReader(): void { IOFactory::registerReader(IOFactory::READER_XLSX, CustomReader::class); - $inputFileName = 'tests/data/Reader/Xlsx/1900_Calendar.xlsx'; + $inputFileName = 'tests/data/Reader/XLSX/1900_Calendar.xlsx'; $loadSpreadsheet = IOFactory::load($inputFileName); $sheet = $loadSpreadsheet->getActiveSheet(); self::assertSame('2022-01-01', $sheet->getCell('A1')->getFormattedValue()); From 133e1ecf682f264943167b55bde50383d5f756f6 Mon Sep 17 00:00:00 2001 From: oleibman <10341515+oleibman@users.noreply.github.com> Date: Sun, 16 Feb 2025 17:55:40 -0800 Subject: [PATCH 3/4] Update IOFactoryRegisterTest.php Now using Phpstan bleeding edge, and it detected a previously undetected problem. --- tests/PhpSpreadsheetTests/IOFactoryRegisterTest.php | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/PhpSpreadsheetTests/IOFactoryRegisterTest.php b/tests/PhpSpreadsheetTests/IOFactoryRegisterTest.php index ceb3797ef2..246fdc4205 100644 --- a/tests/PhpSpreadsheetTests/IOFactoryRegisterTest.php +++ b/tests/PhpSpreadsheetTests/IOFactoryRegisterTest.php @@ -80,7 +80,6 @@ public static function testRegisterCustomWriter(): void IOFactory::registerWriter(IOFactory::WRITER_HTML, CustomWriter::class); $objWriter = IOFactory::createWriter($spreadsheet, CustomWriter::class); self::assertInstanceOf(CustomWriter::class, $objWriter); - self::assertTrue(method_exists($objWriter, 'generateHtmlAll')); $html2 = $objWriter->generateHtmlAll(); self::assertStringContainsString('1', $html2); $spreadsheet->disconnectWorksheets(); From 3675f084f8b9ad5df3033d6c9b3fe0ec821cefac Mon Sep 17 00:00:00 2001 From: oleibman <10341515+oleibman@users.noreply.github.com> Date: Tue, 18 Feb 2025 20:37:20 -0800 Subject: [PATCH 4/4] Update CHANGELOG.md --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8d60ca306b..5957fac338 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,7 @@ and this project adheres to [Semantic Versioning](https://semver.org). ### Changed - Start migration to Phpstan 2. [PR #4359](https://github.com/PHPOffice/PhpSpreadsheet/pull/4359) +- IOFactory identify can return, and createReader and CreateWriter can accept, a class name rather than a file type. [Issue #4357](https://github.com/PHPOffice/PhpSpreadsheet/issues/4357) [PR #4361](https://github.com/PHPOffice/PhpSpreadsheet/pull/4361) ### Moved