Clean up misc. submission handler inconsistencies (#2557)

The various submission handlers were not originally developed under a
strict inheritance hierarchy, and while their behavior is often similar,
there are many slight differences between them. This PR contains
incremental work towards complete standardization of the submission
handlers.

app/Jobs/ProcessSubmission.php

@@ -2,6 +2,7 @@
 
 namespace App\Jobs;
 
+use AbstractSubmissionHandler;
 use App\Utils\UnparsedSubmissionProcessor;
 use App\Models\SuccessfulJob;
 
@@ -292,7 +293,7 @@ private function getSubmissionFileHandle($filename)
         }
     }
 
-    private function getBuildFromHandler($handler)
+    private function getBuildFromHandler(AbstractSubmissionHandler $handler)
     {
         $build = null;
         $builds = $handler->getBuilds();

app/cdash/app/Model/Project.php

@@ -336,7 +336,7 @@ public function Fill(): void
             return;
         }
 
-        if (!$this->Id) {
+        if (!isset($this->Id)) {
             throw new RuntimeException('ID not set for project');
         }
 

app/cdash/app/Model/Subscriber.php

@@ -62,8 +62,6 @@ public function __construct(
     /**
      * @param ActionableBuildInterface $submission
      * @return bool
-     * @throws \DI\DependencyException
-     * @throws \DI\NotFoundException
      */
     public function hasBuildTopics(ActionableBuildInterface $submission)
     {

app/cdash/include/Messaging/Subscription/Subscription.php

@@ -82,10 +82,7 @@ public function setProject(Project $project)
         return $this;
     }
 
-    /**
-     * @return Project
-     */
-    public function getProject()
+    public function getProject(): Project
     {
         return $this->project;
     }

app/cdash/include/Messaging/Topic/Topic.php

@@ -147,10 +147,7 @@ public function getTopicDescription()
         return '';
     }
 
-    /**
-     * @return BuildCollection
-     */
-    public function getBuildCollection()
+    public function getBuildCollection(): BuildCollection
     {
         if (!$this->buildCollection) {
             $this->buildCollection = new BuildCollection();

app/cdash/include/sendemail.php

@@ -546,7 +546,7 @@ function send_update_email(UpdateHandler $handler, int $projectid): void
     if ($update_errors['errors']) {
         // Find the site maintainer(s)
         $sitename = $handler->getSiteName();
-        $siteid = $handler->getSiteId();
+        $siteid = $handler->GetSite()->id;
         $recipients = [];
 
         $db = Database::getInstance();
@@ -556,7 +556,7 @@ function send_update_email(UpdateHandler $handler, int $projectid): void
                                WHERE
                                    ' . qid('user') . '.id=site2user.userid
                                    AND site2user.siteid=?
-                           ', [intval($siteid)]);
+                           ', [$siteid]);
         foreach ($email_addresses as $email_addresses_array) {
             $recipients[] = $email_addresses_array['email'];
         }

app/cdash/tests/case/CDash/ConfigUseCaseTest.php

@@ -1,6 +1,5 @@
 <?php
 
-use CDash\Collection\BuildCollection;
 use CDash\Model\BuildConfigure;
 use CDash\Test\CDashUseCaseTestCase;
 use CDash\Test\UseCase\ConfigUseCase;
@@ -55,7 +54,6 @@ public function testConfigUseCaseBuild()
         $this->assertInstanceOf(ConfigureHandler::class, $handler);
 
         $buildCollection = $handler->GetBuildCollection();
-        $this->assertInstanceOf(BuildCollection::class, $buildCollection);
 
         $this->assertCount(4, $buildCollection);
         $build = $buildCollection->get('MyExperimentalFeature');

app/cdash/tests/case/CDash/Messaging/Subscription/CommitAuthorSubscriptionBuilderTest.php

@@ -38,7 +38,7 @@ public function testBuildGivenSubmissionWithBuildErrors()
         $mock_submission->expects($this->any())
             ->method('GetTopicCollectionForSubscriber')
             ->willReturnCallback(function ($subscriber) {
-                $handler = new BuildHandler(0, 0);
+                $handler = new BuildHandler(0);
                 return $handler->GetTopicCollectionForSubscriber($subscriber);
             });
 
@@ -60,7 +60,7 @@ public function testBuildGivenSubmissionWithBuildWarnings()
         $mock_submission->expects($this->any())
             ->method('GetTopicCollectionForSubscriber')
             ->willReturnCallback(function ($subscriber) {
-                $handler = new BuildHandler(0, 0);
+                $handler = new BuildHandler(0);
                 return $handler->GetTopicCollectionForSubscriber($subscriber);
             });
 
@@ -82,7 +82,7 @@ public function testBuildGivenSubmissionWithTestFailures()
         $mock_submission->expects($this->any())
             ->method('GetTopicCollectionForSubscriber')
             ->willReturnCallback(function ($subscriber) {
-                $handler = new TestingHandler(0, 0);
+                $handler = new TestingHandler(0);
                 return $handler->GetTopicCollectionForSubscriber($subscriber);
             });
 

app/cdash/tests/case/CDash/Messaging/Subscription/UserSubscriptionBuilderTest.php

@@ -50,7 +50,7 @@ public function testBuildGivenBuildSubmission()
         $mock_build_submission->expects($this->any())
             ->method('GetTopicCollectionForSubscriber')
             ->willReturnCallback(function ($subscriber) {
-                $handler = new BuildHandler(0, 0);
+                $handler = new BuildHandler(0);
                 return $handler->GetTopicCollectionForSubscriber($subscriber);
             });
 

app/cdash/tests/case/CDash/XmlHandler/BuildHandlerTest.php

@@ -19,7 +19,6 @@
 use CDash\Messaging\Subscription\CommitAuthorSubscriptionBuilder;
 use CDash\Messaging\Subscription\UserSubscriptionBuilder;
 use CDash\Messaging\Topic\Topic;
-use CDash\Messaging\Topic\TopicCollection;
 use CDash\Model\Subscriber;
 use Tests\TestCase;
 use CDash\Submission\CommitAuthorHandlerInterface;
@@ -28,20 +27,19 @@ class BuildHandlerTest extends TestCase
 {
     public function testBuildHandlerIsACommitAuthorHandler()
     {
-        $sut = new BuildHandler(0, 0);
+        $sut = new BuildHandler(0);
         $this->assertInstanceOf(CommitAuthorHandlerInterface::class, $sut);
     }
 
     public function testGetTopicCollectionForSubscriber()
     {
-        $sut = new BuildHandler(1, 0);
+        $sut = new BuildHandler(1);
         $preferences = new BitmaskNotificationPreferences();
         $subscriber = new Subscriber($preferences);
 
         $collection = $sut->GetTopicCollectionForSubscriber($subscriber);
 
         // Given the preferences the collection should be empty
-        $this->assertInstanceOf(TopicCollection::class, $collection);
         self::assertCount(0, $collection);
 
         $preferences->set(NotifyOn::BUILD_ERROR, true);
@@ -71,7 +69,7 @@ public function testGetTopicCollectionForSubscriber()
 
     public function testGetSubscriptionBuilderCollection()
     {
-        $sut = new BuildHandler(0, 0);
+        $sut = new BuildHandler(0);
         $builders = $sut->GetSubscriptionBuilderCollection();
         $this->assertCount(2, $builders);
         $this->assertTrue($builders->has(UserSubscriptionBuilder::class));

app/cdash/tests/case/CDash/XmlHandler/ConfigureHandlerTest.php

@@ -18,22 +18,20 @@
 use CDash\Messaging\Preferences\BitmaskNotificationPreferences;
 use CDash\Messaging\Subscription\UserSubscriptionBuilder;
 use CDash\Messaging\Topic\Topic;
-use CDash\Messaging\Topic\TopicCollection;
 use CDash\Model\Subscriber;
 use Tests\TestCase;
 
 class ConfigureHandlerTest extends TestCase
 {
     public function testGetBuildTopic()
     {
-        $sut = new ConfigureHandler(1, 0);
+        $sut = new ConfigureHandler(1);
 
         $preferences = new BitmaskNotificationPreferences();
         $subscriber = new Subscriber($preferences);
 
         $collection = $sut->GetTopicCollectionForSubscriber($subscriber);
 
-        $this->assertInstanceOf(TopicCollection::class, $collection);
         self::assertCount(0, $collection);
 
         $preferences->set(NotifyOn::CONFIGURE, true);
@@ -46,7 +44,7 @@ public function testGetBuildTopic()
 
     public function testGetSubscriptionBuilderCollection()
     {
-        $sut = new ConfigureHandler(0, 0);
+        $sut = new ConfigureHandler(0);
         $builders = $sut->GetSubscriptionBuilderCollection();
 
         self::assertCount(1, $builders);

app/cdash/tests/case/CDash/XmlHandler/DynamicAnalysisHandlerTest.php

@@ -4,7 +4,6 @@
 use CDash\Messaging\Preferences\BitmaskNotificationPreferences;
 use CDash\Messaging\Subscription\UserSubscriptionBuilder;
 use CDash\Messaging\Topic\Topic;
-use CDash\Messaging\Topic\TopicCollection;
 use CDash\Model\Subscriber;
 use Tests\TestCase;
 
@@ -27,14 +26,13 @@ class DynamicAnalysisHandlerTest extends TestCase
 {
     public function testGetBuildTopic()
     {
-        $sut = new DynamicAnalysisHandler(1, 0);
+        $sut = new DynamicAnalysisHandler(1);
 
         $preferences = new BitmaskNotificationPreferences();
         $subscriber = new Subscriber($preferences);
 
         $collection = $sut->GetTopicCollectionForSubscriber($subscriber);
 
-        $this->assertInstanceOf(TopicCollection::class, $collection);
         self::assertCount(0, $collection);
 
         $preferences->set(NotifyOn::DYNAMIC_ANALYSIS, true);
@@ -46,7 +44,7 @@ public function testGetBuildTopic()
 
     public function testGetSubscriptionBuilderCollection()
     {
-        $sut = new DynamicAnalysisHandler(0, 0);
+        $sut = new DynamicAnalysisHandler(0);
         $builders = $sut->GetSubscriptionBuilderCollection();
 
         self::assertCount(1, $builders);

app/cdash/tests/case/CDash/XmlHandler/TestingHandlerTest.php

@@ -19,7 +19,6 @@
 use CDash\Messaging\Subscription\CommitAuthorSubscriptionBuilder;
 use CDash\Messaging\Subscription\UserSubscriptionBuilder;
 use CDash\Messaging\Topic\Topic;
-use CDash\Messaging\Topic\TopicCollection;
 use CDash\Model\Subscriber;
 use Tests\TestCase;
 use CDash\Submission\CommitAuthorHandlerInterface;
@@ -28,20 +27,19 @@ class TestingHandlerTest extends TestCase
 {
     public function testTestingHandlerIsACommitAuthorHandler()
     {
-        $sut = new TestingHandler(0, 0);
+        $sut = new TestingHandler(0);
         $this->assertInstanceOf(CommitAuthorHandlerInterface::class, $sut);
     }
 
     public function testGetBuildTopic()
     {
-        $sut = new TestingHandler(1, 0);
+        $sut = new TestingHandler(1);
 
         $preferences = new BitmaskNotificationPreferences();
         $subscriber = new Subscriber($preferences);
 
         $collection = $sut->GetTopicCollectionForSubscriber($subscriber);
 
-        $this->assertInstanceOf(TopicCollection::class, $collection);
         self::assertCount(0, $collection);
 
         $preferences->set(NotifyOn::TEST_FAILURE, true);
@@ -55,7 +53,7 @@ public function testGetBuildTopic()
 
     public function testGetSubscriptionBuilderCollection()
     {
-        $sut = new TestingHandler(0, 0);
+        $sut = new TestingHandler(0);
         $collection = $sut->GetSubscriptionBuilderCollection();
 
         self::assertCount(2, $collection);

app/cdash/tests/case/CDash/XmlHandler/UpdateHandlerTest.php

@@ -19,7 +19,6 @@
 use CDash\Messaging\Subscription\CommitAuthorSubscriptionBuilder;
 use CDash\Messaging\Subscription\UserSubscriptionBuilder;
 use CDash\Messaging\Topic\Topic;
-use CDash\Messaging\Topic\TopicCollection;
 use CDash\Model\Build;
 use CDash\Model\Subscriber;
 use CDash\ServiceContainer;
@@ -30,7 +29,7 @@ class UpdateHandlerTest extends CDashTestCase
 {
     public function testUpdateHandlerIsACommitAuthorHandler()
     {
-        $sut = new UpdateHandler(0, 0);
+        $sut = new UpdateHandler(0);
         $this->assertInstanceOf(CommitAuthorHandlerInterface::class, $sut);
     }
 
@@ -48,20 +47,19 @@ public function testGetCommitAuthors()
             return $build;
         });
 
-        $sut = new UpdateHandler(0, 0);
+        $sut = new UpdateHandler(0);
         $sut->GetCommitAuthors();
     }
 
     public function testGetBuildTopic()
     {
-        $sut = new UpdateHandler(1, 0);
+        $sut = new UpdateHandler(1);
 
         $preferences = new BitmaskNotificationPreferences();
         $subscriber = new Subscriber($preferences);
 
         $collection = $sut->GetTopicCollectionForSubscriber($subscriber);
 
-        $this->assertInstanceOf(TopicCollection::class, $collection);
         self::assertCount(0, $collection);
 
         $preferences->set(NotifyOn::UPDATE_ERROR, true);
@@ -74,7 +72,7 @@ public function testGetBuildTopic()
 
     public function testGetSubscriptionBuilderCollection()
     {
-        $sut = new UpdateHandler(0, 0);
+        $sut = new UpdateHandler(0);
         $collection = $sut->GetSubscriptionBuilderCollection();
 
         self::assertCount(2, $collection);

app/cdash/xml_handlers/abstract_xml_handler.php

@@ -21,17 +21,18 @@
 abstract class AbstractXmlHandler extends AbstractSubmissionHandler
 {
     private Stack $stack;
-    protected $projectid;
     protected bool $Append = false;
     protected Site $Site;
     protected $SubProjectName;
 
-    protected $ModelFactory;
-    protected Project $Project;
+    private $ModelFactory;
+    private Project $Project;
 
     public function __construct($projectid)
     {
-        $this->projectid = $projectid;
+        $this->Project = new Project();
+        $this->Project->Id = $projectid;
+
         $this->stack = new Stack();
     }
 
@@ -70,11 +71,6 @@ public function getSiteName(): string
         return $this->Site->name;
     }
 
-    public function getSiteId(): int
-    {
-        return $this->Site->id;
-    }
-
     public function getBuildStamp()
     {
         return $this->Build->GetStamp();
@@ -98,20 +94,13 @@ protected function getModelFactory(): \CDash\ServiceContainer
         return $this->ModelFactory;
     }
 
-    public function GetProject()
+    public function GetProject(): Project
     {
-        if (!isset($this->Project)) {
-            $this->Project = $this->getModelFactory()->create(Project::class);
-            $this->Project->Id = $this->projectid;
-            $this->Project->Fill();
-        }
+        $this->Project->Fill();
         return $this->Project;
     }
 
-    /**
-     * @return Site
-     */
-    public function GetSite()
+    public function GetSite(): Site
     {
         return $this->Site;
     }