From 2c2d7cae3c4910178e991e17d25999a05abf4528 Mon Sep 17 00:00:00 2001 From: Nicolas Joubert Date: Mon, 28 Sep 2026 12:04:17 +0200 Subject: [PATCH] fix(task) #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. Co-Authored-By: Claude Opus 5.5 (1M context) --- CHANGELOG.md | 1 + docs/reference/tasks/file_splitter_task.md | 7 +- src/Task/File/FileSplitterTask.php | 58 ++++--- tests/Task/File/FileSplitterTaskTest.php | 169 +++++++++++++++++++++ 4 files changed, 215 insertions(+), 20 deletions(-) create mode 100644 tests/Task/File/FileSplitterTaskTest.php diff --git a/CHANGELOG.md b/CHANGELOG.md index e918fb94..82ff1563 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,7 @@ Latest * [#192](https://github.com/cleverage/process-bundle/issues/192) Fix CommandRunnerTask: only pass the `options` option to `Process::setOptions()`, support string `commandline` through `Process::fromShellCommandline()`, validate option types. Update documentation, add tests. * [#194](https://github.com/cleverage/process-bundle/issues/194) Fix ProcessLauncherTask: the `process_options` normalizer returned an array despite its scalar return type, so the task always failed with a `TypeError`. Update documentation, add tests. * [#189](https://github.com/cleverage/process-bundle/issues/189) Fix EventDispatcherTask: dispatch the event under the `event_name` option (regression since v4.0). `event_name` is now optional: when `null` (default), the event is dispatched under its class name. 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. diff --git a/docs/reference/tasks/file_splitter_task.md b/docs/reference/tasks/file_splitter_task.md index eecd8b9f..761c4b5e 100644 --- a/docs/reference/tasks/file_splitter_task.md +++ b/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. diff --git a/src/Task/File/FileSplitterTask.php b/src/Task/File/FileSplitterTask.php index e87bc2e0..c159478e 100644 --- a/src/Task/File/FileSplitterTask.php +++ b/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; + } } diff --git a/tests/Task/File/FileSplitterTaskTest.php b/tests/Task/File/FileSplitterTaskTest.php new file mode 100644 index 00000000..1f0a48c2 --- /dev/null +++ b/tests/Task/File/FileSplitterTaskTest.php @@ -0,0 +1,169 @@ + */ + private array $tmpFiles = []; + + protected function tearDown(): void + { + foreach ($this->tmpFiles as $tmpFile) { + if (is_file($tmpFile)) { + unlink($tmpFile); + } + } + $this->tmpFiles = []; + } + + /** + * @return iterable}> + */ + 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 $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 $options + * + * @return list 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 $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; + } +}