#240 - Fix FileMoverTask: autoincrement applied to the whole path

CHANGELOG.md

@@ -11,6 +11,7 @@ Latest
 * [#143](https://github.com/cleverage/process-bundle/issues/143) Fix level 8 edge cases failing with a `TypeError` or an `Error`: `CsvResource` rejects a blank header line and checks `ftell()`, XpathEvaluatorTransformer throws an explicit `\UnexpectedValueException` on an invalid query (with the libxml messages), a namespace node to unwrap or a node without document, SlugifyTransformer on a value that cannot be transliterated, `ProcessState::getContext()` returns `[]` until the context is defined, GenericTransformer and TransformerTrait throw a `\LogicException` when not initialized, IterableBatchTask and JsonStreamReaderTask can be flushed / iterated before their first execution, RegistryCompilerPass checks its configuration, SubprocessInstance checks the PHP executable. Update documentation, add tests.
 * [#239](https://github.com/cleverage/process-bundle/issues/239) Fix PropertyGetterTask: catch any `\Throwable`, so that the `TypeError` of the property accessor on a scalar input is set on the state (with the `property` error context) and handled according to the task `error_strategy` instead of escaping it. Update documentation, add tests.
 * [#241](https://github.com/cleverage/process-bundle/issues/241) Fix CsvWriterTask: a non-array input throws the documented `\UnexpectedValueException` instead of a `TypeError` when `headers` is `null`, and the CSV tasks release their file on `finalize()` so that the task can be executed again. Update documentation, add tests.
+* [#240](https://github.com/cleverage/process-bundle/issues/240) Fix FileMoverTask: with `autoincrement`, the suffix was computed on the whole path (a dot in a directory name was taken as the extension, making the move fail for a file without extension) and an existing numeric suffix of the file name was replaced (`report-2024.csv` became `report-1.csv`). The suffix is now appended to the file name only (`report-2024-1.csv`, `file-1`, `.env-1`). Update documentation, add tests.
 
 v5.1
 -----

docs/reference/tasks/file_mover_task.md

@@ -26,7 +26,7 @@ Options
 |-----------------|----------|:--------:|---------|--------------------------------------------------------------------------------------------------------------------------|
 | `destination`   | `string` |  **X**   |         | Destination path. If it is an existing directory, the original file name is kept                                         |
 | `overwrite`     | `bool`   |          | `false` | Allow overwriting an existing file at destination (otherwise an exception is thrown)                                     |
-| `autoincrement` | `bool`   |          | `false` | If the destination file exists, add or increment a numeric suffix before the extension (e.g. `file-1.csv`, `file-2.csv`) |
+| `autoincrement` | `bool`   |          | `false` | If the destination file exists, append the first free numeric suffix to the file name, before its extension (e.g. `file-1.csv`, `file-2.csv`) |
 
 Examples
 --------
@@ -46,3 +46,6 @@ Notes
 -----
 
 * Underlying method is Symfony `Filesystem::rename()`.
+* With `autoincrement`, only the file name is changed (never the directory part), and the suffix is always appended to
+  the original name: `report-2024.csv` becomes `report-2024-1.csv`, `file` becomes `file-1`, `archive.tar.gz` becomes
+  `archive.tar-1.gz` and a hidden file `.env` becomes `.env-1`.

src/Task/File/FileMoverTask.php

@@ -57,16 +57,26 @@ protected function configureOptions(OptionsResolver $resolver): void
     protected function makeFilenameUnique(string $dest): string
     {
         $fs = new Filesystem();
+        // Only the file name is changed, the directory part may contain dots
+        $basename = basename($dest);
+        $directory = substr($dest, 0, \strlen($dest) - \strlen($basename));
+        // A leading dot (hidden file) is not an extension separator
+        $dotPosition = strrpos($basename, '.');
+        if (false === $dotPosition || 0 === $dotPosition) {
+            $name = $basename;
+            $extension = '';
+        } else {
+            $name = substr($basename, 0, $dotPosition);
+            $extension = substr($basename, $dotPosition);
+        }
+
+        $uniqueDest = $dest;
         $i = 1;
-        while ($fs->exists($dest)) {
-            if (preg_match('/^(.*?)(-\d+)?(\.[^.]*)$/', $dest, $matches)) {
-                $dest = $matches[1].'-'.$i.$matches[3];
-                ++$i;
-            } else {
-                $dest .= '-'.$i; // Fallback brutal mode
-            }
+        while ($fs->exists($uniqueDest)) {
+            $uniqueDest = $directory.$name.'-'.$i.$extension;
+            ++$i;
         }
 
-        return $dest;
+        return $uniqueDest;
     }
 }

tests/Task/File/FileMoverTaskTest.php

@@ -42,7 +42,6 @@ class FileMoverTaskTest extends TestCase
 
     protected function setUp(): void
     {
-        // No more_entropy: the directory name must not contain a dot (see makeFilenameUnique)
         $this->tmpDir = sys_get_temp_dir().\DIRECTORY_SEPARATOR.uniqid('file_mover_test_');
         $this->filesystem = new Filesystem();
         $this->filesystem->dumpFile($this->tmpDir.'/src/file.csv', 'source');
@@ -170,6 +169,51 @@ public function testAutoincrementAppendsSuffixToFileWithoutExtension(): void
         self::assertSame('source', file_get_contents($this->tmpDir.'/dest/file-1'));
     }
 
+    public function testAutoincrementIncrementsExistingSuffixOfFileWithoutExtension(): void
+    {
+        $this->filesystem->dumpFile($this->tmpDir.'/src/file', 'source');
+        $this->filesystem->dumpFile($this->tmpDir.'/dest/file', 'existing');
+        $this->filesystem->dumpFile($this->tmpDir.'/dest/file-1', 'existing 1');
+
+        $state = $this->execute(
+            ['destination' => $this->tmpDir.'/dest', 'autoincrement' => true],
+            $this->tmpDir.'/src/file',
+        );
+
+        self::assertSame($this->tmpDir.'/dest/file-2', $state->getOutput());
+        self::assertSame('source', file_get_contents($this->tmpDir.'/dest/file-2'));
+    }
+
+    /**
+     * @return iterable<string, array{string, string}>
+     */
+    public static function autoincrementFileNameProvider(): iterable
+    {
+        yield 'file with extension' => ['file.csv', 'file-1.csv'];
+        yield 'file without extension' => ['file', 'file-1'];
+        yield 'file with numeric suffix' => ['report-2024.csv', 'report-2024-1.csv'];
+        yield 'file with several dots' => ['archive.tar.gz', 'archive.tar-1.gz'];
+        yield 'hidden file' => ['.env', '.env-1'];
+        yield 'hidden file with extension' => ['.env.local', '.env-1.local'];
+    }
+
+    #[DataProvider('autoincrementFileNameProvider')]
+    public function testAutoincrementOnlyChangesFileNameInDottedDirectory(string $fileName, string $expectedFileName): void
+    {
+        $dir = $this->tmpDir.'/repro.d';
+        $this->filesystem->dumpFile($dir.'/src/'.$fileName, 'source');
+        $this->filesystem->dumpFile($dir.'/dest/'.$fileName, 'existing');
+
+        $state = $this->execute(
+            ['destination' => $dir.'/dest', 'autoincrement' => true],
+            $dir.'/src/'.$fileName,
+        );
+
+        self::assertSame($dir.'/dest/'.$expectedFileName, $state->getOutput());
+        self::assertSame('source', file_get_contents($dir.'/dest/'.$expectedFileName));
+        self::assertSame('existing', file_get_contents($dir.'/dest/'.$fileName));
+    }
+
     public function testThrowsOnMissingDestinationOption(): void
     {
         $this->expectException(MissingOptionsException::class);