#205 - Fix FileSplitterTask

CHANGELOG.md

@@ -15,6 +15,7 @@ Latest
 * [#202](https://github.com/cleverage/process-bundle/issues/202) Fix XmlReaderTask: throw an explicit `\UnexpectedValueException` on an empty file or invalid XML (with the libxml error messages) instead of a `ValueError` or a silent empty `\DOMDocument`. Update documentation, add tests.
 * [#203](https://github.com/cleverage/process-bundle/issues/203) Fix `ProcessState::addErrorContextValue()`: accept any value type, so that iterable tasks on an empty iterable are skipped and PropertySetterTask reports the original exception instead of a `TypeError`. Update documentation, add tests.
 * [#204](https://github.com/cleverage/process-bundle/issues/204) Fix CachedTransformer, SlugifyTransformer and TypeSetterTransformer edge cases: accept a non-string input in `cached` (key built after `key_transformers`), reject an invalid `transliterator` in `slugify` with an `InvalidOptionsException`, remove the unreachable error branch of `type_setter`. Update documentation, add tests.
+* [#205](https://github.com/cleverage/process-bundle/issues/205) Fix FileSplitterTask: produced files lost the first line, doubled line breaks and ended with an empty file; each file now contains exactly `max_lines` lines of the source. Update documentation, add tests.
 
 ## Deprecated
 * [#189](https://github.com/cleverage/process-bundle/issues/189) EventDispatcherTask: when `event_name` is set, listening to `CleverAge\ProcessBundle\Event\EventDispatcherTaskEvent` is deprecated (the event is still dispatched under its class name, with an `E_USER_DEPRECATED` error, if it has listeners). Listen to the configured `event_name` instead: the BC layer will be removed in v6.0.

docs/reference/tasks/file_splitter_task.md

@@ -19,8 +19,8 @@ file path as input. Any other input is ignored.
 Possible outputs
 ----------------
 
-`string`: path of a temporary file (created in the system temporary directory, with a `.tmp` extension) containing a
-chunk of lines of the source file.
+`string`: path of a temporary file (created in the system temporary directory, with a `.tmp` extension) containing the
+next `max_lines` lines of the source file (the last file may contain fewer lines).
 
 Options
 -------
@@ -49,5 +49,8 @@ Notes
 -----
 
 * Values given as input are merged after option resolution, so they are not validated.
+* Every line of the source file is kept, in order, including empty lines. Line content is preserved, but each line
+  break (`\n` or `\r\n`) is written as `PHP_EOL`, and a missing line break on the last line is added.
+* An empty source file produces no output (the task is skipped).
 * Temporary files are not deleted by the task, use [FileRemoverTask](file_remover_task.md) if needed.
 * For CSV files, prefer [CsvSplitterTask](csv_splitter_task.md) which keeps the headers in each produced file.

src/Task/File/FileSplitterTask.php

@@ -26,17 +26,27 @@ class FileSplitterTask extends AbstractConfigurableTask implements IterableTaskI
 {
     protected ?SplFile $file = null;
 
-    private ?array $splFileObjectFlags = null;
-
-    private int $lineCount;
+    /**
+     * Next line of the source file to write (read ahead to detect the end of the file), null when there is none.
+     */
+    private ?string $nextLine = null;
 
     public function execute(ProcessState $state): void
     {
         $options = $this->getMergedOptions($state);
-        $this->splFileObjectFlags = [\SplFileObject::READ_AHEAD, \SplFileObject::SKIP_EMPTY];
         if (!$this->file instanceof SplFile) {
-            $this->file = new SplFile($options['file_path'], 'rb', $this->splFileObjectFlags);
-            $this->lineCount = $this->file->getLineCount();
+            // No flag: lines are read with fgets(), which ignores DROP_NEW_LINE/SKIP_EMPTY and must not be preceded
+            // by a rewind() in READ_AHEAD mode (the first line would be skipped)
+            $this->file = new SplFile($options['file_path'], 'rb', []);
+            $this->nextLine = $this->file->readLine();
+        }
+
+        if (null === $this->nextLine) {
+            // Empty source file: nothing to split
+            $this->file = null;
+            $state->setSkipped(true);
+
+            return;
         }
 
         // Return a temporary file containing a limited number of lines
@@ -55,26 +65,26 @@ public function next(ProcessState $state): bool
             return false;
         }
 
-        // Fix issue on PHP 8 with empty line at the end, even if SKIP_EMPTY is set
-        $endOfFile = $this->file->isEndOfFile() || $this->file->getLineNumber() > $this->lineCount;
-        if ($endOfFile) {
+        if (null === $this->nextLine) {
             $this->file = null;
+
+            return false;
         }
 
-        return !$endOfFile;
+        return true;
     }
 
     protected function splitFile(SplFile $file, int $maxLines): string
     {
         $tmpFilePath = sys_get_temp_dir().\DIRECTORY_SEPARATOR.'php_'.uniqid('process', false).'.tmp';
-        $splitFile = new SplFile($tmpFilePath, 'wb', $this->splFileObjectFlags);
-
-        while ($splitFile->getLineNumber() <= $maxLines && !$file->isEndOfFile()) {
-            $line = $file->readLine();
-            if ('' === $line || null === $line) {
-                continue; // This is probably an empty line, no harm to skip it
-            }
-            $splitFile->writeLine($line);
+        $splitFile = new SplFile($tmpFilePath, 'wb', []);
+
+        $writtenLines = 0;
+        while (null !== $this->nextLine && $writtenLines < $maxLines) {
+            // fgets() keeps the line break while writeLine() appends one
+            $splitFile->writeLine($this->stripLineBreak($this->nextLine));
+            ++$writtenLines;
+            $this->nextLine = $file->readLine();
         }
 
         return $tmpFilePath;
@@ -107,4 +117,16 @@ protected function getMergedOptions(ProcessState $state): array
 
         return array_merge($options, $input);
     }
+
+    private function stripLineBreak(string $line): string
+    {
+        if (str_ends_with($line, "\r\n")) {
+            return substr($line, 0, -2);
+        }
+        if (str_ends_with($line, "\n")) {
+            return substr($line, 0, -1);
+        }
+
+        return $line;
+    }
 }

tests/Task/File/FileSplitterTaskTest.php

@@ -0,0 +1,169 @@
+<?php
+
+declare(strict_types=1);
+
+/*
+ * This file is part of the CleverAge/ProcessBundle package.
+ *
+ * Copyright (c) Clever-Age
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace CleverAge\ProcessBundle\Tests\Task\File;
+
+use CleverAge\ProcessBundle\Configuration\ProcessConfiguration;
+use CleverAge\ProcessBundle\Configuration\TaskConfiguration;
+use CleverAge\ProcessBundle\Context\ContextualOptionResolver;
+use CleverAge\ProcessBundle\Filesystem\SplFile;
+use CleverAge\ProcessBundle\Model\ProcessHistory;
+use CleverAge\ProcessBundle\Model\ProcessState;
+use CleverAge\ProcessBundle\Task\File\FileSplitterTask;
+use PHPUnit\Framework\Attributes\DataProvider;
+use PHPUnit\Framework\TestCase;
+
+#[\PHPUnit\Framework\Attributes\CoversClass(FileSplitterTask::class)]
+#[\PHPUnit\Framework\Attributes\UsesClass(SplFile::class)]
+#[\PHPUnit\Framework\Attributes\UsesClass(ProcessConfiguration::class)]
+#[\PHPUnit\Framework\Attributes\UsesClass(TaskConfiguration::class)]
+#[\PHPUnit\Framework\Attributes\UsesClass(ContextualOptionResolver::class)]
+#[\PHPUnit\Framework\Attributes\UsesClass(ProcessHistory::class)]
+#[\PHPUnit\Framework\Attributes\UsesClass(ProcessState::class)]
+class FileSplitterTaskTest extends TestCase
+{
+    /** @var list<string> */
+    private array $tmpFiles = [];
+
+    protected function tearDown(): void
+    {
+        foreach ($this->tmpFiles as $tmpFile) {
+            if (is_file($tmpFile)) {
+                unlink($tmpFile);
+            }
+        }
+        $this->tmpFiles = [];
+    }
+
+    /**
+     * @return iterable<string, array{string, int, list<string>}>
+     */
+    public static function provideSplitCases(): iterable
+    {
+        yield 'last chunk is shorter' => ["a\nb\nc\nd\ne\n", 2, ["a\nb\n", "c\nd\n", "e\n"]];
+        yield 'exact multiple of max_lines' => ["a\nb\nc\nd\n", 2, ["a\nb\n", "c\nd\n"]];
+        yield 'no trailing line break' => ["a\nb\nc", 2, ["a\nb\n", "c\n"]];
+        yield 'one line per file' => ["a\nb\nc\n", 1, ["a\n", "b\n", "c\n"]];
+        yield 'max_lines greater than line count' => ["a\nb\nc\n", 10, ["a\nb\nc\n"]];
+        yield 'empty lines are kept' => ["a\n\nb\n\n", 2, ["a\n\n", "b\n\n"]];
+        yield 'CRLF line breaks are not doubled' => ["a\r\nb\r\nc\r\n", 2, ["a\nb\n", "c\n"]];
+        yield 'line content is preserved' => ["  a;b  \n\tc\r\n", 5, ["  a;b  \n\tc\n"]];
+    }
+
+    /**
+     * @param list<string> $expectedChunks
+     */
+    #[DataProvider('provideSplitCases')]
+    public function testSplit(string $content, int $maxLines, array $expectedChunks): void
+    {
+        $filePath = $this->createSourceFile($content);
+
+        $chunks = $this->runTask(new FileSplitterTask(), ['file_path' => $filePath, 'max_lines' => $maxLines]);
+
+        $this->assertSame(
+            array_map(static fn (string $chunk): string => str_replace("\n", \PHP_EOL, $chunk), $expectedChunks),
+            $chunks,
+        );
+    }
+
+    public function testEmptyFileProducesNoOutput(): void
+    {
+        $filePath = $this->createSourceFile('');
+        $task = new FileSplitterTask();
+        $state = $this->createState(['file_path' => $filePath, 'max_lines' => 2]);
+
+        $task->execute($state);
+
+        $this->assertTrue($state->isSkipped());
+        $this->assertNull($state->getOutput());
+        $this->assertFalse($task->next($state));
+    }
+
+    public function testFilePathAndMaxLinesCanBeGivenAsInput(): void
+    {
+        $filePath = $this->createSourceFile("a\nb\nc\n");
+
+        $chunks = $this->runTask(
+            new FileSplitterTask(),
+            ['file_path' => '/does/not/exist', 'max_lines' => 10],
+            ['file_path' => $filePath, 'max_lines' => 2],
+        );
+
+        $this->assertSame(['a'.\PHP_EOL.'b'.\PHP_EOL, 'c'.\PHP_EOL], $chunks);
+    }
+
+    public function testTaskCanBeReusedAfterIteration(): void
+    {
+        $filePath = $this->createSourceFile("a\nb\nc\n");
+        $task = new FileSplitterTask();
+        $options = ['file_path' => $filePath, 'max_lines' => 2];
+        $expected = ['a'.\PHP_EOL.'b'.\PHP_EOL, 'c'.\PHP_EOL];
+
+        $firstRun = $this->runTask($task, $options);
+        $secondRun = $this->runTask($task, $options);
+
+        $this->assertSame($expected, $firstRun);
+        $this->assertSame($firstRun, $secondRun);
+    }
+
+    /**
+     * Mimics the process manager loop on an iterable task: execute(), then next() until it returns false.
+     *
+     * @param array<string, mixed> $options
+     *
+     * @return list<string> content of each produced file
+     */
+    private function runTask(FileSplitterTask $task, array $options, mixed $input = null): array
+    {
+        $chunks = [];
+        $iterations = 0;
+        do {
+            $state = $this->createState($options);
+            $state->setInput($input);
+            $task->execute($state);
+            if (!$state->isSkipped()) {
+                $outputFile = $state->getOutput();
+                $this->assertIsString($outputFile);
+                $this->tmpFiles[] = $outputFile;
+                $chunks[] = (string) file_get_contents($outputFile);
+            }
+            $this->assertLessThan(100, ++$iterations, 'Infinite iteration');
+        } while ($task->next($state));
+
+        return $chunks;
+    }
+
+    private function createSourceFile(string $content): string
+    {
+        $filePath = (string) tempnam(sys_get_temp_dir(), 'file_splitter_test_');
+        file_put_contents($filePath, $content);
+        $this->tmpFiles[] = $filePath;
+
+        return $filePath;
+    }
+
+    /**
+     * @param array<string, mixed> $options
+     */
+    private function createState(array $options): ProcessState
+    {
+        $processConfiguration = new ProcessConfiguration('test', []);
+        $state = new ProcessState($processConfiguration, new ProcessHistory($processConfiguration));
+        $state->setContextualOptionResolver(new ContextualOptionResolver());
+        $state->setContext([]);
+        $state->setSkipped(false);
+        $state->setTaskConfiguration(new TaskConfiguration('split', FileSplitterTask::class, $options));
+
+        return $state;
+    }
+}