Don't record VersionDeleted event if package was deleted

Author: stevenrombauts

src/Command/CleanSpamPackagesCommand.php

@@ -66,7 +66,7 @@ protected function execute(InputInterface $input, OutputInterface $output): int
             }
             $output->write('.');
             foreach ($package->getVersions() as $version) {
-                $versionRepo->remove($version);
+                $versionRepo->remove($version, false);
             }
 
             $this->providerManager->deletePackage($package);

src/Entity/VersionRepository.php

@@ -20,6 +20,7 @@
 use Doctrine\ORM\QueryBuilder;
 use Doctrine\Persistence\ManagerRegistry;
 use Predis\Client;
+use Symfony\Bundle\SecurityBundle\Security;
 
 /**
  * @author Jordi Boggiano <j.boggiano@seld.be>
@@ -34,6 +35,7 @@ public function __construct(
         ManagerRegistry $registry,
         private Client $redisCache,
         private VersionIdCache $versionIdCache,
+        private readonly Security $security,
     ) {
         parent::__construct($registry, Version::class);
     }
@@ -44,7 +46,7 @@ public function getEntityManager(): EntityManagerInterface
         return parent::getEntityManager();
     }
 
-    public function remove(Version $version): void
+    public function remove(Version $version, bool $createAuditRecord = true): void
     {
         $em = $this->getEntityManager();
         $package = $version->getPackage();
@@ -66,6 +68,12 @@ public function remove(Version $version): void
         $em->getConnection()->executeQuery('DELETE FROM php_stat WHERE version=:version AND depth = :depth AND package_id=:packageId', ['version' => $version->getId(), 'depth' => PhpStat::DEPTH_EXACT, 'packageId' => $version->getPackage()->getId()]);
 
         $em->remove($version);
+
+        if ($createAuditRecord) {
+            $user = $this->security->getUser();
+            $record = AuditRecord::versionDeleted($version, $user instanceof User ? $user : null);
+            $em->persist($record);
+        }
     }
 
     /**

src/EventListener/VersionListener.php

@@ -21,9 +21,7 @@
 use Doctrine\ORM\Event\PreUpdateEventArgs;
 use Doctrine\Persistence\Event\LifecycleEventArgs;
 use Doctrine\Persistence\ManagerRegistry;
-use Symfony\Bundle\SecurityBundle\Security;
 
-#[AsEntityListener(event: 'preRemove', entity: Version::class)]
 #[AsEntityListener(event: 'preUpdate', entity: Version::class)]
 #[AsEntityListener(event: 'postUpdate', entity: Version::class)]
 class VersionListener
@@ -35,20 +33,9 @@ class VersionListener
 
     public function __construct(
         private ManagerRegistry $doctrine,
-        private Security $security,
     ) {
     }
 
-    /**
-     * @param LifecycleEventArgs<EntityManager> $event
-     */
-    public function preRemove(Version $version, LifecycleEventArgs $event): void
-    {
-        $record = AuditRecord::versionDeleted($version, $this->getUser());
-        $this->getEM()->persist($record);
-        // let the record be flushed together with the entity
-    }
-
     public function preUpdate(Version $version, PreUpdateEventArgs $event): void
     {
         if (($event->hasChangedField('source') || $event->hasChangedField('dist')) && !$version->isDevelopment()) {
@@ -76,11 +63,4 @@ public function postUpdate(Version $version, LifecycleEventArgs $event): void
             $this->buffered = [];
         }
     }
-
-    private function getUser(): ?User
-    {
-        $user = $this->security->getUser();
-
-        return $user instanceof User ? $user : null;
-    }
 }

src/Model/PackageManager.php

@@ -62,7 +62,7 @@ public function deletePackage(Package $package): void
     {
         $versionRepo = $this->doctrine->getRepository(Version::class);
         foreach ($package->getVersions() as $version) {
-            $versionRepo->remove($version);
+            $versionRepo->remove($version, false);
         }
 
         if ($package->getAutoUpdated() === Package::AUTO_GITHUB_HOOK) {

src/Package/Updater.php

@@ -128,7 +128,6 @@ public function update(IOInterface $io, Config $config, Package $package, VcsRep
         $deleteDate = new \DateTimeImmutable('-1day');
 
         $em = $this->getEM();
-        $rootIdentifier = null;
 
         $driver = $repository->getDriver();
         if (!$driver) {

tests/Audit/VersionAuditRecordTest.php

@@ -92,12 +92,5 @@ public function testVersionChangesGetRecorded(): void
 
         $logs = $container->get(Connection::class)->fetchAllAssociative('SELECT * FROM audit_log ORDER BY id DESC');
         self::assertCount(2, $logs);
-
-        $em->remove($version);
-        $em->flush();
-
-        $logs = $container->get(Connection::class)->fetchAllAssociative('SELECT * FROM audit_log ORDER BY id DESC');
-        self::assertCount(3, $logs);
-        self::assertSame(AuditRecordType::VersionDeleted->value, $logs[0]['type']);
     }
 }

tests/Entity/VersionRepositoryTest.php

@@ -0,0 +1,65 @@
+<?php declare(strict_types=1);
+
+/*
+ * This file is part of Packagist.
+ *
+ * (c) Jordi Boggiano <j.boggiano@seld.be>
+ *     Nils Adermann <naderman@naderman.de>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace App\Tests\Entity;
+
+use App\Audit\AuditRecordType;
+use App\Entity\AuditRecord;
+use App\Entity\Version;
+use App\Entity\VersionRepository;
+use App\Tests\IntegrationTestCase;
+
+class VersionRepositoryTest extends IntegrationTestCase
+{
+    private VersionRepository $versionRepository;
+
+    protected function setUp(): void
+    {
+        parent::setUp();
+
+        $this->versionRepository = self::getEM()->getRepository(Version::class);
+    }
+
+    public function testRemoveVersionMarksForRemovalAndCreatesAuditRecord(): void
+    {
+        $em = self::getEM();
+
+        $package = self::createPackage('vendor/package', 'https://github.com/vendor/package');
+
+        $version = new Version();
+        $version->setPackage($package);
+        $version->setName($package->getName());
+        $version->setVersion('1.0.0');
+        $version->setNormalizedVersion('1.0.0.0');
+        $version->setDevelopment(false);
+        $version->setLicense([]);
+        $version->setAutoload([]);
+        $package->getVersions()->add($version);
+
+        $this->store($package, $version);
+
+        $versionId = $version->getId();
+        $this->versionRepository->remove($version);
+
+        $em->flush();
+        $em->clear();
+
+        $this->assertNull($this->versionRepository->find($versionId), 'Version was not deleted');
+
+        $auditRecord = $em->getRepository(AuditRecord::class)->findOneBy([
+            'type' => AuditRecordType::VersionDeleted->value,
+            'packageId' => $package->getId(),
+            'actorId' => null,
+        ]);
+        $this->assertNotNull($auditRecord, 'No audit record for version deletion created');
+    }
+}