From: Eileen McNaughton <emcnaughton@wikimedia.org>
Date: Thu, 6 Apr 2023 03:08:25 +0000 (+1200)
Subject: Type hints & strictness fixes within tests
X-Git-Url: https://vcs.fsf.org/?a=commitdiff_plain;h=85491b9c510690e49146212b99dbc41780ed2746;p=civicrm-core.git

Type hints & strictness fixes within tests
---

diff --git a/Civi/Test/DbTestTrait.php b/Civi/Test/DbTestTrait.php
index 700aad2804..ff81b0e0d5 100644
--- a/Civi/Test/DbTestTrait.php
+++ b/Civi/Test/DbTestTrait.php
@@ -169,7 +169,7 @@ trait DbTestTrait {
    * Example: $this->assertSql(2, 'select count(*) from foo where foo.bar like "%1"',
    * array(1 => array("Whiz", "String")));
    *
-   * @param string|null $expected
+   * @param string|null|int $expected
    * @param string $query
    * @param array $params
    * @param string $message
@@ -177,7 +177,7 @@ trait DbTestTrait {
    * @noinspection PhpUnhandledExceptionInspection
    * @noinspection PhpDocMissingThrowsInspection
    */
-  public function assertDBQuery(?string $expected, string $query, array $params = [], string $message = ''): void {
+  public function assertDBQuery($expected, string $query, array $params = [], string $message = ''): void {
     if ($message) {
       $message .= ': ';
     }
diff --git a/tests/phpunit/CiviTest/CiviUnitTestCase.php b/tests/phpunit/CiviTest/CiviUnitTestCase.php
index 31ae9d8e3c..128680d306 100644
--- a/tests/phpunit/CiviTest/CiviUnitTestCase.php
+++ b/tests/phpunit/CiviTest/CiviUnitTestCase.php
@@ -2426,10 +2426,10 @@ class CiviUnitTestCase extends PHPUnit\Framework\TestCase {
   protected function assertAttachmentExistence(bool $exists, array $apiResult): void {
     $this->assertIsNumeric($apiResult['id']);
     $this->assertEquals($exists, file_exists($apiResult['values'][$apiResult['id']]['path']));
-    $this->assertDBQuery($exists ? 1 : 0, 'SELECT count(*) FROM civicrm_file WHERE id = %1', [
+    $this->assertDBQuery((int) $exists, 'SELECT count(*) FROM civicrm_file WHERE id = %1', [
       1 => [$apiResult['id'], 'Int'],
     ]);
-    $this->assertDBQuery($exists ? 1 : 0, 'SELECT count(*) FROM civicrm_entity_file WHERE id = %1', [
+    $this->assertDBQuery((int) $exists, 'SELECT count(*) FROM civicrm_entity_file WHERE id = %1', [
       1 => [$apiResult['id'], 'Int'],
     ]);
   }
diff --git a/tests/phpunit/api/v3/AttachmentTest.php b/tests/phpunit/api/v3/AttachmentTest.php
index 151b4de8f4..03e05bc0f8 100644
--- a/tests/phpunit/api/v3/AttachmentTest.php
+++ b/tests/phpunit/api/v3/AttachmentTest.php
@@ -38,9 +38,9 @@ class api_v3_AttachmentTest extends CiviUnitTestCase {
   /**
    * @return string
    */
-  public static function getFilePrefix() {
+  public static function getFilePrefix(): ?string {
     if (!self::$filePrefix) {
-      self::$filePrefix = "test_" . CRM_Utils_String::createRandom(5, CRM_Utils_String::ALPHANUMERIC) . '_';
+      self::$filePrefix = 'test_' . CRM_Utils_String::createRandom(5, CRM_Utils_String::ALPHANUMERIC) . '_';
     }
     return self::$filePrefix;
   }
@@ -62,7 +62,7 @@ class api_v3_AttachmentTest extends CiviUnitTestCase {
   /**
    * @return array
    */
-  public function okCreateProvider() {
+  public function okCreateProvider(): array {
     // array($entityClass, $createParams, $expectedContent)
     $cases = [];
 
@@ -117,7 +117,7 @@ class api_v3_AttachmentTest extends CiviUnitTestCase {
   /**
    * @return array
    */
-  public function badCreateProvider() {
+  public function badCreateProvider(): array {
     // array($entityClass, $createParams, $expectedError)
     $cases = [];
 
@@ -162,7 +162,7 @@ class api_v3_AttachmentTest extends CiviUnitTestCase {
         'description' => 'My test description',
         'content' => 'My test content',
       ],
-      "/Malformed name/",
+      '/Malformed name/',
     ];
 
     return $cases;
@@ -171,7 +171,7 @@ class api_v3_AttachmentTest extends CiviUnitTestCase {
   /**
    * @return array
    */
-  public function badUpdateProvider() {
+  public function badUpdateProvider(): array {
     // array($entityClass, $createParams, $updateParams, $expectedError)
     $cases = [];
 
@@ -204,7 +204,7 @@ class api_v3_AttachmentTest extends CiviUnitTestCase {
   /**
    * @return array
    */
-  public function okGetProvider() {
+  public function okGetProvider(): array {
     // array($getParams, $expectedNames)
     $cases = [];
 
@@ -257,7 +257,7 @@ class api_v3_AttachmentTest extends CiviUnitTestCase {
   /**
    * @return array
    */
-  public function badGetProvider() {
+  public function badGetProvider(): array {
     // array($getParams, $expectedNames)
     $cases = [];
 
@@ -279,19 +279,19 @@ class api_v3_AttachmentTest extends CiviUnitTestCase {
     ];
     $cases[] = [
       ['entity_table' => 'civicrm_activity', 'entity_id' => '123', 'name' => 'example_456.csv'],
-      "/Get by name is not currently supported/",
+      '/Get by name is not currently supported/',
     ];
     $cases[] = [
       ['entity_table' => 'civicrm_activity', 'entity_id' => '123', 'content' => 'test'],
-      "/Get by content is not currently supported/",
+      '/Get by content is not currently supported/',
     ];
     $cases[] = [
       ['entity_table' => 'civicrm_activity', 'entity_id' => '123', 'path' => '/home/foo'],
-      "/Get by path is not currently supported/",
+      '/Get by path is not currently supported/',
     ];
     $cases[] = [
       ['entity_table' => 'civicrm_activity', 'entity_id' => '123', 'url' => '/index.php'],
-      "/Get by url is not currently supported/",
+      '/Get by url is not currently supported/',
     ];
 
     return $cases;
@@ -304,25 +304,27 @@ class api_v3_AttachmentTest extends CiviUnitTestCase {
    *   E.g. "CRM_Core_DAO_Activity".
    * @param array $createParams
    * @param string $expectedContent
+   *
    * @dataProvider okCreateProvider
+   * @throws \CRM_Core_Exception
    */
-  public function testCreate($testEntityClass, $createParams, $expectedContent) {
+  public function testCreate(string $testEntityClass, array $createParams, string $expectedContent): void {
     $entity = CRM_Core_DAO::createTestObject($testEntityClass);
     $entity_table = CRM_Core_DAO_AllCoreTables::getTableForClass($testEntityClass);
-    $this->assertTrue(is_numeric($entity->id));
+    $this->assertIsNumeric($entity->id);
 
     $createResult = $this->callAPISuccess('Attachment', 'create', $createParams + [
       'entity_table' => $entity_table,
       'entity_id' => $entity->id,
     ]);
     $fileId = $createResult['id'];
-    $this->assertTrue(is_numeric($fileId));
+    $this->assertIsNumeric($fileId);
     $this->assertEquals($entity_table, $createResult['values'][$fileId]['entity_table']);
     $this->assertEquals($entity->id, $createResult['values'][$fileId]['entity_id']);
     $this->assertEquals('My test description', $createResult['values'][$fileId]['description']);
     $this->assertRegExp('/\d\d\d\d-\d\d-\d\d \d\d:\d\d:\d\d/', $createResult['values'][$fileId]['upload_date']);
-    $this->assertTrue(!isset($createResult['values'][$fileId]['content']));
-    $this->assertTrue(!empty($createResult['values'][$fileId]['url']));
+    $this->assertNotTrue(isset($createResult['values'][$fileId]['content']));
+    $this->assertNotEmpty($createResult['values'][$fileId]['url']);
     $this->assertAttachmentExistence(TRUE, $createResult);
 
     $getResult = $this->callAPISuccess('Attachment', 'get', [
@@ -331,7 +333,7 @@ class api_v3_AttachmentTest extends CiviUnitTestCase {
     ]);
     $this->assertEquals(1, $getResult['count']);
     foreach (['id', 'entity_table', 'entity_id', 'url'] as $field) {
-      if ($field == 'url') {
+      if ($field === 'url') {
         $this->assertEquals(substr($createResult['values'][$fileId][$field], 0, -15), substr($getResult['values'][$fileId][$field], 0, -15));
         $this->assertEquals(substr($createResult['values'][$fileId][$field], -3), substr($getResult['values'][$fileId][$field], -3));
         $this->assertApproxEquals(substr($createResult['values'][$fileId][$field], -14, 10), substr($getResult['values'][$fileId][$field], -14, 10), 2);
@@ -340,7 +342,7 @@ class api_v3_AttachmentTest extends CiviUnitTestCase {
         $this->assertEquals($createResult['values'][$fileId][$field], $getResult['values'][$fileId][$field], "Expect field $field to match");
       }
     }
-    $this->assertTrue(!isset($getResult['values'][$fileId]['content']));
+    $this->assertNotTrue(isset($getResult['values'][$fileId]['content']));
 
     $getResult2 = $this->callAPISuccess('Attachment', 'get', [
       'entity_table' => $entity_table,
@@ -350,7 +352,7 @@ class api_v3_AttachmentTest extends CiviUnitTestCase {
     $this->assertEquals($expectedContent, $getResult2['values'][$fileId]['content']);
     // Do this again even though we just tested above to demonstrate that these fields should be returned even if you only ask to return 'content'.
     foreach (['id', 'entity_table', 'entity_id', 'url'] as $field) {
-      if ($field == 'url') {
+      if ($field === 'url') {
         $this->assertEquals(substr($createResult['values'][$fileId][$field], 0, -15), substr($getResult2['values'][$fileId][$field], 0, -15));
         $this->assertEquals(substr($createResult['values'][$fileId][$field], -3), substr($getResult2['values'][$fileId][$field], -3));
         $this->assertApproxEquals(substr($createResult['values'][$fileId][$field], -14, 10), substr($getResult2['values'][$fileId][$field], -14, 10), 2);
@@ -367,10 +369,10 @@ class api_v3_AttachmentTest extends CiviUnitTestCase {
    * @param $expectedError
    * @dataProvider badCreateProvider
    */
-  public function testCreateFailure($testEntityClass, $createParams, $expectedError) {
+  public function testCreateFailure($testEntityClass, $createParams, $expectedError): void {
     $entity = CRM_Core_DAO::createTestObject($testEntityClass);
     $entity_table = CRM_Core_DAO_AllCoreTables::getTableForClass($testEntityClass);
-    $this->assertTrue(is_numeric($entity->id));
+    $this->assertIsNumeric($entity->id);
 
     $createResult = $this->callAPIFailure('Attachment', 'create', $createParams + [
       'entity_table' => $entity_table,
@@ -386,17 +388,17 @@ class api_v3_AttachmentTest extends CiviUnitTestCase {
    * @param $expectedError
    * @dataProvider badUpdateProvider
    */
-  public function testCreateWithBadUpdate($testEntityClass, $createParams, $updateParams, $expectedError) {
+  public function testCreateWithBadUpdate($testEntityClass, $createParams, $updateParams, $expectedError): void {
     $entity = CRM_Core_DAO::createTestObject($testEntityClass);
     $entity_table = CRM_Core_DAO_AllCoreTables::getTableForClass($testEntityClass);
-    $this->assertTrue(is_numeric($entity->id));
+    $this->assertIsNumeric($entity->id);
 
     $createResult = $this->callAPISuccess('Attachment', 'create', $createParams + [
       'entity_table' => $entity_table,
       'entity_id' => $entity->id,
     ]);
     $fileId = $createResult['id'];
-    $this->assertTrue(is_numeric($fileId));
+    $this->assertIsNumeric($fileId);
 
     $updateResult = $this->callAPIFailure('Attachment', 'create', $updateParams + [
       'id' => $fileId,
@@ -408,9 +410,9 @@ class api_v3_AttachmentTest extends CiviUnitTestCase {
    * If one submits a weird file name, it should be automatically converted
    * to something safe.
    */
-  public function testCreateWithWeirdName() {
+  public function testCreateWithWeirdName(): void {
     $entity = CRM_Core_DAO::createTestObject('CRM_Activity_DAO_Activity');
-    $this->assertTrue(is_numeric($entity->id));
+    $this->assertIsNumeric($entity->id);
 
     $createResult = $this->callAPISuccess('Attachment', 'create', [
       'name' => self::getFilePrefix() . 'weird:na"me.txt',
@@ -421,19 +423,19 @@ class api_v3_AttachmentTest extends CiviUnitTestCase {
       'entity_id' => $entity->id,
     ]);
     $fileId = $createResult['id'];
-    $this->assertTrue(is_numeric($fileId));
+    $this->assertIsNumeric($fileId);
     $this->assertEquals(self::getFilePrefix() . 'weird_na_me.txt', $createResult['values'][$fileId]['name']);
     // Check for appropriate icon
     $this->assertEquals('fa-file-text-o', $createResult['values'][$fileId]['icon']);
   }
 
-  public function testCreateShouldSetCreatedIdAsTheLoggedInUser() {
+  public function testCreateShouldSetCreatedIdAsTheLoggedInUser(): void {
     $loggedInUser = $this->createLoggedInUser();
 
     $testEntityClass = 'CRM_Activity_DAO_Activity';
     $entity = CRM_Core_DAO::createTestObject($testEntityClass);
     $entity_table = CRM_Core_DAO_AllCoreTables::getTableForClass($testEntityClass);
-    $this->assertTrue(is_numeric($entity->id));
+    $this->assertIsNumeric($entity->id);
 
     $createResult = $this->callAPISuccess('Attachment', 'create', [
       'name' => self::getFilePrefix() . 'exampleFromContent.txt',
@@ -451,7 +453,7 @@ class api_v3_AttachmentTest extends CiviUnitTestCase {
     $testEntityClass = 'CRM_Activity_DAO_Activity';
     $entity = CRM_Core_DAO::createTestObject($testEntityClass);
     $entity_table = CRM_Core_DAO_AllCoreTables::getTableForClass($testEntityClass);
-    $this->assertTrue(is_numeric($entity->id));
+    $this->assertIsNumeric($entity->id);
 
     $createResult = $this->callAPISuccess('Attachment', 'create', [
       'name' => self::getFilePrefix() . 'exampleFromContent.txt',
@@ -469,7 +471,7 @@ class api_v3_AttachmentTest extends CiviUnitTestCase {
     $testEntityClass = 'CRM_Activity_DAO_Activity';
     $entity = CRM_Core_DAO::createTestObject($testEntityClass);
     $entity_table = CRM_Core_DAO_AllCoreTables::getTableForClass($testEntityClass);
-    $this->assertTrue(is_numeric($entity->id));
+    $this->assertIsNumeric($entity->id);
 
     $attachmentParams = [
       'name' => self::getFilePrefix() . 'exampleFromContent.txt',
@@ -504,11 +506,12 @@ class api_v3_AttachmentTest extends CiviUnitTestCase {
   }
 
   /**
-   * @param $getParams
-   * @param $expectedNames
+   * @param array $getParams
+   * @param array $expectedNames
+   *
    * @dataProvider okGetProvider
    */
-  public function testGet($getParams, $expectedNames) {
+  public function testGet(array $getParams, array $expectedNames): void {
     foreach ([123, 456] as $entity_id) {
       foreach (['text/plain' => '.txt', 'text/csv' => '.csv'] as $mime => $ext) {
         $this->callAPISuccess('Attachment', 'create', [
@@ -528,7 +531,7 @@ class api_v3_AttachmentTest extends CiviUnitTestCase {
     foreach ($getResult['values'] as $result) {
       $queryResult = [];
       $parsedURl = parse_url($result['url']);
-      $parsedQuery = parse_str($parsedURl['query'], $queryResult);
+      parse_str($parsedURl['query'], $queryResult);
       $this->assertTrue(CRM_Core_BAO_File::validateFileHash($queryResult['fcs'], $queryResult['eid'], $queryResult['id']));
     }
 
@@ -542,7 +545,7 @@ class api_v3_AttachmentTest extends CiviUnitTestCase {
    * @param $expectedError
    * @dataProvider badGetProvider
    */
-  public function testGetError($getParams, $expectedError) {
+  public function testGetError($getParams, $expectedError): void {
     foreach ([123, 456] as $entity_id) {
       foreach (['text/plain' => '.txt', 'text/csv' => '.csv'] as $mime => $ext) {
         $this->callAPISuccess('Attachment', 'create', [
@@ -565,9 +568,9 @@ class api_v3_AttachmentTest extends CiviUnitTestCase {
    * the full thing back in as an update ("create"). This ensures some
    * consistency in the acceptable formats.
    */
-  public function testGetThenUpdate() {
+  public function testGetThenUpdate(): void {
     $entity = CRM_Core_DAO::createTestObject('CRM_Activity_DAO_Activity');
-    $this->assertTrue(is_numeric($entity->id));
+    $this->assertIsNumeric($entity->id);
 
     $createResult = $this->callAPISuccess('Attachment', 'create', [
       'name' => self::getFilePrefix() . 'getThenUpdate.txt',
@@ -577,17 +580,17 @@ class api_v3_AttachmentTest extends CiviUnitTestCase {
       'entity_table' => 'civicrm_activity',
       'entity_id' => $entity->id,
     ]);
-    $fileId = $createResult['id'];
-    $this->assertTrue(is_numeric($fileId));
-    $this->assertEquals(self::getFilePrefix() . 'getThenUpdate.txt', $createResult['values'][$fileId]['name']);
+    $fileID = $createResult['id'];
+    $this->assertIsNumeric($fileID);
+    $this->assertEquals(self::getFilePrefix() . 'getThenUpdate.txt', $createResult['values'][$fileID]['name']);
     $this->assertAttachmentExistence(TRUE, $createResult);
 
     $getResult = $this->callAPISuccess('Attachment', 'get', [
-      'id' => $fileId,
+      'id' => $fileID,
     ]);
-    $this->assertTrue(is_array($getResult['values'][$fileId]));
+    $this->assertIsArray($getResult['values'][$fileID]);
 
-    $updateParams = $getResult['values'][$fileId];
+    $updateParams = $getResult['values'][$fileID];
     $updateParams['description'] = 'new description';
     $this->callAPISuccess('Attachment', 'create', $updateParams);
     $this->assertAttachmentExistence(TRUE, $createResult);
@@ -597,9 +600,9 @@ class api_v3_AttachmentTest extends CiviUnitTestCase {
    * Create an attachment and delete using its ID. Assert that the records are correctly created and destroyed
    * in the DB and the filesystem.
    */
-  public function testDeleteByID() {
+  public function testDeleteByID(): void {
     $entity = CRM_Core_DAO::createTestObject('CRM_Activity_DAO_Activity');
-    $this->assertTrue(is_numeric($entity->id));
+    $this->assertIsNumeric($entity->id);
 
     foreach (['first', 'second'] as $n) {
       $createResults[$n] = $this->callAPISuccess('Attachment', 'create', [
@@ -609,7 +612,7 @@ class api_v3_AttachmentTest extends CiviUnitTestCase {
         'entity_table' => 'civicrm_activity',
         'entity_id' => $entity->id,
       ]);
-      $this->assertTrue(is_numeric($createResults[$n]['id']));
+      $this->assertIsNumeric($createResults[$n]['id']);
       $this->assertEquals(self::getFilePrefix() . 'testDeleteByID.txt', $createResults[$n]['values'][$createResults[$n]['id']]['name']);
     }
     $this->assertAttachmentExistence(TRUE, $createResults['first']);
@@ -626,7 +629,7 @@ class api_v3_AttachmentTest extends CiviUnitTestCase {
    * Create an attachment and delete using its ID. Assert that the records are correctly created and destroyed
    * in the DB and the filesystem.
    */
-  public function testDeleteByEntity() {
+  public function testDeleteByEntity(): void {
     // create 2 entities (keepme,delme) -- each with 2 attachments (first,second)
     foreach (['keepme', 'delme'] as $e) {
       $entities[$e] = CRM_Core_DAO::createTestObject('CRM_Activity_DAO_Activity');
@@ -639,7 +642,7 @@ class api_v3_AttachmentTest extends CiviUnitTestCase {
           'entity_table' => 'civicrm_activity',
           'entity_id' => $entities[$e]->id,
         ]);
-        $this->assertTrue(is_numeric($createResults[$e][$n]['id']));
+        $this->assertIsNumeric($createResults[$e][$n]['id']);
       }
     }
     $this->assertAttachmentExistence(TRUE, $createResults['keepme']['first']);
@@ -660,9 +663,9 @@ class api_v3_AttachmentTest extends CiviUnitTestCase {
   /**
    * Ensure mime type is converted to appropriate icon.
    */
-  public function testGetIcon() {
+  public function testGetIcon(): void {
     $entity = CRM_Core_DAO::createTestObject('CRM_Activity_DAO_Activity');
-    $this->assertTrue(is_numeric($entity->id));
+    $this->assertIsNumeric($entity->id);
 
     $createResult = $this->callAPISuccess('Attachment', 'create', [
       'name' => self::getFilePrefix() . 'hasIcon.docx',
@@ -691,20 +694,20 @@ class api_v3_AttachmentTest extends CiviUnitTestCase {
    * @param $name
    * @return string
    */
-  protected function tmpFile($name) {
+  protected function tmpFile($name): string {
     $tmpDir = sys_get_temp_dir();
     $this->assertTrue($tmpDir && is_dir($tmpDir), 'Tmp dir must exist: ' . $tmpDir);
     return $tmpDir . '/' . self::getFilePrefix() . $name;
   }
 
-  protected function cleanupFiles() {
+  protected function cleanupFiles(): void {
     $config = CRM_Core_Config::singleton();
     $dirs = [
       sys_get_temp_dir(),
       $config->customFileUploadDir,
     ];
     foreach ($dirs as $dir) {
-      $files = (array) glob($dir . "/" . self::getFilePrefix() . "*");
+      $files = (array) glob($dir . '/' . self::getFilePrefix() . '*');
       foreach ($files as $file) {
         unlink($file);
       }