fix: keep upload files until their deletion is committed

src/Controller/Administrative/Disposition/DocumentUploadController.php

@@ -13,6 +13,7 @@ use Doctrine\ORM\EntityManagerInterface;
 use Flagception\Manager\FeatureManagerInterface;
 use Psr\Log\LoggerInterface;
 use Symfony\Bundle\FrameworkBundle\Controller\AbstractController;
+use Symfony\Component\Form\FormError;
 use Symfony\Component\HttpFoundation\Request;
 use Symfony\Component\HttpFoundation\Response;
 use Symfony\Component\Routing\Attribute\Route;
@@ -49,6 +50,12 @@ class DocumentUploadController extends AbstractController
         $form = $this->createFormBuilder()->getForm();
         $form->handleRequest($request);
 
+        // Checked before processUpload() removes the existing document, which it replaces.
+        if ($form->isSubmitted()
+            && true === $this->uploadHandler->getUploadSession()->getUploadsByType($type)->isEmpty()) {
+            $form->addError(new FormError('Bitte lade zuerst eine Datei hoch'));
+        }
+
         if ($form->isSubmitted() && $form->isValid()) {
             $this->processUpload($disposition, $type);
 
@@ -110,15 +117,17 @@ class DocumentUploadController extends AbstractController
 
         $disposition->addDocument($upload);
 
-        $this->entityManager->flush();
-
         $this->uploadHandler->moveUploadSessionFilesFromOrphanage($type, $uploadSession);
         $this->uploadHandler->removeUploadSessionUploadsByType($type);
 
+        // Before the flush: converting a non-PDF renames the file, and the new filename has to
+        // be persisted with the row.
         if (true === $this->featureManager->isActive('sanitize_uploads')) {
             $this->uploadHandler->ensurePdf($upload);
         }
 
+        $this->entityManager->flush();
+
         $this->eventDispatcher->dispatch(new DocumentUploadedEvent($upload), DocumentUploadedEvent::NAME);
         if (Upload::TYPE_CONTRACT === $type) {
             $this->eventDispatcher->dispatch(new DocumentConfirmedEvent($upload), DocumentConfirmedEvent::NAME);

src/Controller/Teamer/Disposition/DetailController.php

@@ -79,6 +79,14 @@ class DetailController extends AbstractController
 
     private function uploadContract(Disposition $disposition, UploadSessionDto $uploadSession): void
     {
+        // The session can hold files of other types, so a non-empty session alone does not
+        // mean there is anything to replace the existing document with.
+        if (true === $uploadSession->getUploadsByType(Upload::TYPE_CONTRACT)->isEmpty()) {
+            $this->addFlash('error', 'Bitte lade zuerst den Honorarvertrag hoch');
+
+            return;
+        }
+
         if (null !== $existingUpload = $disposition->getDocumentByType(Upload::TYPE_CONTRACT)) {
             $this->entityManager->remove($existingUpload);
         }
@@ -116,6 +124,14 @@ class DetailController extends AbstractController
 
     private function uploadInvoice(Disposition $disposition, UploadSessionDto $uploadSession): void
     {
+        // The session can hold files of other types, so a non-empty session alone does not
+        // mean there is anything to replace the existing document with.
+        if (true === $uploadSession->getUploadsByType(Upload::TYPE_INVOICE)->isEmpty()) {
+            $this->addFlash('error', 'Bitte lade zuerst die Honorarnote hoch');
+
+            return;
+        }
+
         if (null !== $existingUpload = $disposition->getDocumentByType(Upload::TYPE_INVOICE)) {
             $this->entityManager->remove($existingUpload);
         }

src/EventListener/DeleteUploadListener.php

@@ -5,31 +5,70 @@ namespace App\EventListener;
 use App\Entity\Upload;
 use App\Service\Upload\UploadHandler;
 use Doctrine\Bundle\DoctrineBundle\Attribute\AsDoctrineListener;
-use Doctrine\ORM\Event\PreRemoveEventArgs;
+use Doctrine\ORM\Event\OnFlushEventArgs;
+use Doctrine\ORM\Event\PostFlushEventArgs;
 use Doctrine\ORM\Events;
 use Liip\ImagineBundle\Imagine\Cache\CacheManager;
+use Symfony\Contracts\Service\ResetInterface;
 
-#[AsDoctrineListener(event: Events::preRemove)]
+/**
-class DeleteUploadListener
+ * Removes the files of deleted uploads from disk, once the deletion has been committed.
+ *
+ * Doctrine dispatches preRemove inside EntityManager::remove(), not at flush. Deleting the file
+ * there meant that a request which removed the old upload and then failed before its flush -
+ * a replacement that could not be built - lost the file while the row survived. The files are
+ * therefore collected at onFlush and only deleted at postFlush, which is reached only after the
+ * transaction has committed. A flush that throws never gets there, and the next onFlush drops
+ * what it collected.
+ */
+#[AsDoctrineListener(event: Events::onFlush)]
+#[AsDoctrineListener(event: Events::postFlush)]
+class DeleteUploadListener implements ResetInterface
 {
+    /**
+     * @var array<int, Upload>
+     */
+    private array $pending = [];
+
     public function __construct(
         private readonly UploadHandler $uploadHandler,
         private readonly CacheManager $cacheManager,
     ) {
     }
 
-    public function preRemove(PreRemoveEventArgs $eventArgs): void
+    public function onFlush(OnFlushEventArgs $args): void
     {
-        $object = $eventArgs->getObject();
+        $this->pending = [];
 
-        if (!$object instanceof Upload) {
+        // Includes uploads removed by cascade, e.g. the documents of a deleted disposition.
+        foreach ($args->getObjectManager()->getUnitOfWork()->getScheduledEntityDeletions() as $entity) {
+            if ($entity instanceof Upload) {
+                $this->pending[] = $entity;
+            }
+        }
+    }
+
+    public function postFlush(PostFlushEventArgs $args): void
+    {
+        if (0 === count($this->pending)) {
             return;
         }
 
-        // Remove uploaded original
+        // Taken and cleared up front, in case anything downstream flushes again.
-        $this->uploadHandler->removeUploadedFile($object);
+        $pending = $this->pending;
+        $this->pending = [];
 
-        // Remove potential variants from public folders
+        foreach ($pending as $upload) {
-        $this->cacheManager->remove($object->getFilename());
+            // Remove uploaded original
+            $this->uploadHandler->removeUploadedFile($upload);
+
+            // Remove potential variants from public folders
+            $this->cacheManager->remove($upload->getFilename());
+        }
+    }
+
+    public function reset(): void
+    {
+        $this->pending = [];
     }
 }

templates/administrative/disposition/modal_document_upload.html.twig

@@ -5,6 +5,7 @@
 {% block content %}
     {% set formAttr = stimulus_controller('form-upload-guard')|stimulus_action('form-upload-guard', 'guard', 'submit') %}
     {{ form_start(form, { 'attr': { 'hx-post': app.request.uri, 'hx-target': '#htmx-modal', 'hx-swap': 'outerHTML' }|merge(formAttr.toArray()) }) }}
+    {{ form_errors(form) }}
     <div class="pb-4">
         {% include '_partials/_upload_collection_form.html.twig' with {
             'endpoint_upload': path(endpointUpload),

tests/EventListener/DeleteUploadListenerTest.php

@@ -0,0 +1,144 @@
+<?php
+
+declare(strict_types=1);
+
+namespace App\Tests\EventListener;
+
+use App\Entity\Upload;
+use App\Service\Upload\UploadHandler;
+use Doctrine\DBAL\Exception as DBALException;
+use Doctrine\ORM\EntityManagerInterface;
+use Doctrine\Persistence\ManagerRegistry;
+use Symfony\Bundle\FrameworkBundle\Test\KernelTestCase;
+use Symfony\Component\Filesystem\Filesystem;
+
+/**
+ * Doctrine dispatches preRemove inside EntityManager::remove(), so a listener deleting the file
+ * there loses it whenever the request fails between remove() and flush() - the row survives and
+ * points at nothing. These tests pin that the file only goes once the deletion has committed.
+ */
+class DeleteUploadListenerTest extends KernelTestCase
+{
+    private EntityManagerInterface $entityManager;
+
+    private UploadHandler $uploadHandler;
+
+    /**
+     * @var array<int, string>
+     */
+    private array $createdFiles = [];
+
+    protected function setUp(): void
+    {
+        self::bootKernel();
+
+        $this->entityManager = self::getContainer()->get(EntityManagerInterface::class);
+        $this->uploadHandler = self::getContainer()->get(UploadHandler::class);
+
+        $this->entityManager->createQuery(sprintf('DELETE FROM %s', Upload::class))->execute();
+    }
+
+    protected function tearDown(): void
+    {
+        (new Filesystem())->remove($this->createdFiles);
+
+        parent::tearDown();
+    }
+
+    public function testTheFileIsRemovedOnceTheDeletionIsFlushed(): void
+    {
+        $upload = $this->storedUpload();
+
+        $this->entityManager->remove($upload);
+        $this->entityManager->flush();
+
+        $this->assertFileDoesNotExist($this->filepath($upload));
+    }
+
+    public function testTheFileIsKeptWhileTheDeletionIsNotFlushed(): void
+    {
+        $upload = $this->storedUpload();
+
+        $this->entityManager->remove($upload);
+
+        $this->assertFileExists($this->filepath($upload));
+    }
+
+    public function testTheFileIsKeptWhenTheFlushFails(): void
+    {
+        $upload = $this->storedUpload();
+        $filepath = $this->filepath($upload);
+
+        $this->entityManager->remove($upload);
+        // No filename: violates NOT NULL, so the flush rolls back.
+        $this->entityManager->persist((new Upload())
+            ->setType(Upload::TYPE_DOCUMENT)
+            ->setOriginalFilename('broken.pdf')
+            ->setMimeType('application/pdf')
+            ->setSize(1024)
+        );
+
+        try {
+            $this->entityManager->flush();
+            $this->fail('The flush was expected to fail');
+        } catch (DBALException) {
+        }
+
+        $this->assertFileExists($filepath);
+    }
+
+    public function testAFailedFlushDoesNotHandItsFilesToTheNextOne(): void
+    {
+        $upload = $this->storedUpload();
+        $filepath = $this->filepath($upload);
+
+        $this->entityManager->remove($upload);
+        $this->entityManager->persist((new Upload())
+            ->setType(Upload::TYPE_DOCUMENT)
+            ->setOriginalFilename('broken.pdf')
+            ->setMimeType('application/pdf')
+            ->setSize(1024)
+        );
+
+        try {
+            $this->entityManager->flush();
+        } catch (DBALException) {
+        }
+
+        /** @var EntityManagerInterface $entityManager */
+        $entityManager = self::getContainer()->get(ManagerRegistry::class)->resetManager();
+        $entityManager->persist($this->upload('unrelated.pdf'));
+        $entityManager->flush();
+
+        $this->assertFileExists($filepath);
+    }
+
+    private function storedUpload(): Upload
+    {
+        $upload = $this->upload(sprintf('test-delete-upload-%s.pdf', bin2hex(random_bytes(8))));
+        $this->entityManager->persist($upload);
+        $this->entityManager->flush();
+
+        $filepath = $this->filepath($upload);
+        (new Filesystem())->dumpFile($filepath, '%PDF-1.4');
+        $this->createdFiles[] = $filepath;
+
+        return $upload;
+    }
+
+    private function upload(string $filename): Upload
+    {
+        return (new Upload())
+            ->setType(Upload::TYPE_DOCUMENT)
+            ->setFilename($filename)
+            ->setOriginalFilename($filename)
+            ->setMimeType('application/pdf')
+            ->setSize(1024)
+        ;
+    }
+
+    private function filepath(Upload $upload): string
+    {
+        return $this->uploadHandler->getUploadFilepath($upload);
+    }
+}