Merge branch 'improve_validation'

[#75] Improve profile validation

CRM/Twingle/Form/Profile.php

@@ -16,8 +16,9 @@
 declare(strict_types = 1);
 
 use CRM_Twingle_ExtensionUtil as E;
-use Civi\Twingle\Exceptions\ProfileException;
 use Civi\Twingle\Exceptions\BaseException;
+use Civi\Twingle\Exceptions\ProfileException;
+use Civi\Twingle\Exceptions\ProfileValidationError;
 
 /**
  * Form controller class
@@ -168,7 +169,7 @@ class CRM_Twingle_Form_Profile extends CRM_Core_Form {
 
     switch ($this->_op) {
       case 'delete':
-        if ($this->profile) {
+        if (isset($this->profile)) {
           CRM_Utils_System::setTitle(E::ts('Delete Twingle API profile <em>%1</em>', [1 => $this->profile->getName()]));
           $this->addButtons([
             [
@@ -185,23 +186,26 @@ class CRM_Twingle_Form_Profile extends CRM_Core_Form {
         // Retrieve the source profile name.
         $source_id = CRM_Utils_Request::retrieve('source_id', 'Int', $this);
         // When copying without a valid profile id, copy the default profile.
-        if (!$source_id) {
+        if (!is_int($source_id)) {
           $this->profile = CRM_Twingle_Profile::createDefaultProfile();
-        } else {
+        }
+        else {
           try {
             $source_profile = CRM_Twingle_Profile::getProfile($source_id);
             $this->profile = $source_profile->copy();
             $this->profile->validate();
-          } catch (ProfileValidationError $e) {
+          }
-            if ($e->getErrorCode() == ProfileValidationError::ERROR_CODE_PROFILE_VALIDATION_FAILED) {
+          catch (ProfileValidationError $exception) {
-              Civi::log()->error($e->getLogMessage());
+            if ($exception->getErrorCode() == ProfileValidationError::ERROR_CODE_PROFILE_VALIDATION_FAILED) {
+              Civi::log()->error($exception->getLogMessage());
               CRM_Core_Session::setStatus(E::ts('The profile is invalid and cannot be copied.'), E::ts('Error'));
               CRM_Utils_System::redirect(CRM_Utils_System::url('civicrm/admin/settings/twingle/profiles', 'reset=1'));
               return;
             }
-          } catch (ProfileException $e) {
+          }
-            if ($e->getErrorCode() == ProfileException::ERROR_CODE_PROFILE_NOT_FOUND) {
+          catch (ProfileException $exception) {
-              Civi::log()->error($e->getLogMessage());
+            if ($exception->getErrorCode() == ProfileException::ERROR_CODE_PROFILE_NOT_FOUND) {
+              Civi::log()->error($exception->getLogMessage());
               CRM_Core_Session::setStatus(E::ts('The profile to be copied could not be found.'), E::ts('Error'));
               CRM_Utils_System::redirect(CRM_Utils_System::url('civicrm/admin/settings/twingle/profiles', 'reset=1'));
               return;
@@ -209,7 +213,10 @@ class CRM_Twingle_Form_Profile extends CRM_Core_Form {
           }
           catch (Civi\Core\Exception\DBQueryException $e) {
             Civi::log()->error($e->getMessage());
-            CRM_Core_Session::setStatus(E::ts('A database error has occurred. See the log for details.'), E::ts('Error'));
+            CRM_Core_Session::setStatus(
+              E::ts('A database error has occurred. See the log for details.'),
+              E::ts('Error')
+            );
             CRM_Utils_System::redirect(CRM_Utils_System::url('civicrm/admin/settings/twingle/profiles', 'reset=1'));
             return;
           }
@@ -218,7 +225,9 @@ class CRM_Twingle_Form_Profile extends CRM_Core_Form {
         break;
 
       case 'edit':
-        CRM_Utils_System::setTitle(E::ts('Edit Twingle API profile <em>%1</em>', [1 => $this->profile->getName()]));
+        CRM_Utils_System::setTitle(
+          E::ts('Edit Twingle API profile <em>%1</em>', [1 => $this->profile->getName()])
+        );
         break;
 
       case 'create':
@@ -553,90 +562,31 @@ class CRM_Twingle_Form_Profile extends CRM_Core_Form {
    *   TRUE when the form was successfully validated.
    */
   public function validate() {
-    $values = $this->exportValues();
 
-    // Validate new profile names.
+    if (in_array($this->_op, ['create', 'edit', 'copy'], TRUE)) {
-    if (
+      // Create profile with new values.
-      isset($values['name'])
+      $profile_values = $this->exportValues();
-      && (!isset($this->profile) || $values['name'] != $this->profile->getName() || $this->_op != 'edit')
+      $profile = new CRM_Twingle_Profile(
-      && NULL !== CRM_Twingle_Profile::getProfile($values['name'])
+        $profile_values['name'],
-    ) {
+        $profile_values,
-      $this->_errors['name'] = E::ts('A profile with this name already exists.');
+        $this->profile_id
-    }
+      );
 
-    // Restrict profile names to alphanumeric characters and the underscore.
+      // Validate profile data
-    if (isset($values['name']) && 1 === preg_match('/[^A-Za-z0-9\_]/', $values['name'])) {
-      $this->_errors['name'] =
-        E::ts('Only alphanumeric characters and the underscore (_) are allowed for profile names.');
-    }
-
-    // Validate custom field mapping.
       try {
-      if (isset($values['custom_field_mapping'])) {
+        $profile->validate();
-        $custom_field_mapping = preg_split('/\r\n|\r|\n/', $values['custom_field_mapping'], -1, PREG_SPLIT_NO_EMPTY);
-        if (!is_array($custom_field_mapping)) {
-          throw new BaseException(
-            E::ts('Could not parse custom field mapping.')
-          );
       }
-        foreach ($custom_field_mapping as $custom_field_map) {
+      catch (ProfileValidationError $e) {
-          $custom_field_map = explode('=', $custom_field_map);
+        switch ($e->getErrorCode()) {
-          if (count($custom_field_map) !== 2) {
+          case ProfileValidationError::ERROR_CODE_PROFILE_VALIDATION_FAILED:
-            throw new BaseException(
+            $this->setElementError($e->getAffectedFieldName(), $e->getMessage());
-              E::ts('Could not parse custom field mapping.')
+            break;
-            );
-          }
-          [$twingle_field_name, $custom_field_name] = $custom_field_map;
-          $custom_field_id = substr($custom_field_name, strlen('custom_'));
 
-          // Check for custom field existence
+          case ProfileValidationError::ERROR_CODE_PROFILE_VALIDATION_WARNING:
-          try {
+            CRM_Core_Session::setStatus($e->getMessage(), E::ts('Warning'));
-            $custom_field = civicrm_api3('CustomField', 'getsingle', [
-              'id' => $custom_field_id,
-            ]);
-          }
-          catch (CRM_Core_Exception $exception) {
-            throw new BaseException(
-              E::ts(
-                'Custom field custom_%1 does not exist.',
-                [1 => $custom_field_id]
-              ),
-              NULL,
-              $exception
-            );
-          }
-
-          // Only allow custom fields on relevant entities.
-          try {
-            $custom_group = civicrm_api3('CustomGroup', 'getsingle', [
-              'id' => $custom_field['custom_group_id'],
-              'extends' => [
-                'IN' => [
-                  'Contact',
-                  'Individual',
-                  'Organization',
-                  'Contribution',
-                  'ContributionRecur',
-                ],
-              ],
-            ]);
-          }
-          catch (CRM_Core_Exception $exception) {
-            throw new BaseException(
-              E::ts(
-                'Custom field custom_%1 is not in a CustomGroup that extends one of the supported CiviCRM entities.',
-                [1 => $custom_field['id']]
-              ),
-              NULL,
-              $exception
-            );
         }
       }
     }
-    }
-    catch (BaseException $exception) {
-      $this->_errors['custom_field_mapping'] = $exception->getMessage();
-    }
 
     return parent::validate();
   }

CRM/Twingle/Profile.php

@@ -17,6 +17,7 @@ declare(strict_types = 1);
 
 use CRM_Twingle_ExtensionUtil as E;
 use Civi\Twingle\Exceptions\ProfileException as ProfileException;
+use Civi\Twingle\Exceptions\ProfileValidationError;
 
 /**
  * Profiles define how incoming submissions from the Twingle API are
@@ -25,16 +26,16 @@ use Civi\Twingle\Exceptions\ProfileException as ProfileException;
 class CRM_Twingle_Profile {
 
   /**
-   * @var int $id
+   * @var int
    *   The id of the profile.
    */
-  protected $id = NULL;
+  protected ?int $id;
 
   /**
-   * @var string $name
+   * @var string
    *   The name of the profile.
    */
-  protected $name;
+  protected string $name;
 
   /**
    * @var array<string, mixed>
@@ -99,14 +100,7 @@ class CRM_Twingle_Profile {
    * @return bool
    */
   public function matches($project_id) {
-    $selector = $this->getAttribute('selector');
+    return in_array($project_id, $this->getProjectIds(), TRUE);
-    $project_ids = array_map(
-      function($project_id) {
-        return trim($project_id);
-      },
-      explode(',', $selector)
-    );
-    return in_array($project_id, $project_ids, TRUE);
   }
 
   /**
@@ -148,7 +142,7 @@ class CRM_Twingle_Profile {
    *
    * @return int
    */
-  public function getId() {
+  public function getId(): ?int {
     return $this->id;
   }
 
@@ -157,7 +151,7 @@ class CRM_Twingle_Profile {
    *
    * @param int $id
    */
-  public function setId(int $id) {
+  public function setId(int $id): void {
     $this->id = $id;
   }
 
@@ -166,7 +160,7 @@ class CRM_Twingle_Profile {
    *
    * @return string
    */
-  public function getName() {
+  public function getName(): string {
     return $this->name;
   }
 
@@ -179,6 +173,29 @@ class CRM_Twingle_Profile {
     $this->name = $name;
   }
 
+  /**
+   * Is this the default profile?
+   *
+   * @return bool
+   */
+  public function is_default(): bool {
+    return $this->name === 'default';
+  }
+
+  /**
+   * Retrieves the profile's project IDs.
+   *
+   * @return array<string>
+   */
+  public function getProjectIds(): array {
+    return array_map(
+      function($project_id) {
+        return trim($project_id);
+      },
+      explode(',', $this->getAttribute('selector') ?? '')
+    );
+  }
+
   /**
    * Retrieves an attribute of the profile.
    *
@@ -225,11 +242,139 @@ class CRM_Twingle_Profile {
   /**
    * Verifies whether the profile is valid (i.e. consistent and not colliding
    * with other profiles).
+   *
+   * @throws \Civi\Twingle\Exceptions\ProfileValidationError
+   * @throws \Civi\Core\Exception\DBQueryException
+   *   When the profile could not be successfully validated.
    */
-  public function verifyProfile(): void {
+  public function validate(): void {
-    // TODO: check
+
-    //  data of this profile consistent?
+    // Name cannot be empty
-    //  conflicts with other profiles?
+    if ('' === $this->getName()) {
+      throw new ProfileValidationError(
+        'name',
+        E::ts('Profile name cannot be empty.'),
+        ProfileValidationError::ERROR_CODE_PROFILE_VALIDATION_FAILED
+      );
+    }
+
+    // Restrict profile names to alphanumeric characters, space and the underscore.
+    $contains_illegal_characters = (1 !== preg_match('/[^A-Za-z0-9_\s]/', $this->getName()));
+    if ($contains_illegal_characters) {
+      throw new ProfileValidationError(
+        'name',
+        E::ts('Only alphanumeric characters, space and the underscore (_) are allowed for profile names.'),
+        ProfileValidationError::ERROR_CODE_PROFILE_VALIDATION_FAILED
+      );
+    }
+
+    // Check if profile name is already used for other profile
+    $profile_name_duplicates = array_filter(
+      CRM_Twingle_Profile::getProfiles(),
+      function($profile) {
+        return $profile->getName() == $this->getName() && $this->getId() != $profile->getId();
+      });
+    if ([] !== $profile_name_duplicates) {
+      throw new ProfileValidationError(
+        'name',
+        E::ts("A profile with the name '%1' already exists.", [1 => $this->getName()]),
+        ProfileValidationError::ERROR_CODE_PROFILE_VALIDATION_FAILED
+      );
+    }
+
+    // Check if project_id is already used in other profile
+    $profiles = $this::getProfiles();
+    foreach ($profiles as $profile) {
+      if ($profile->getId() == $this->getId() || $profile->is_default()) {
+        continue;
+      };
+      $project_ids = $this->getProjectIds();
+      $id_duplicates = array_intersect($profile->getProjectIds(), $project_ids);
+      if ([] !== $id_duplicates) {
+        throw new ProfileValidationError(
+          'selector',
+          E::ts(
+            "Project ID(s) [%1] already used in profile '%2'.",
+            [
+              1 => implode(', ', $id_duplicates),
+              2 => $profile->getName(),
+            ]
+          ),
+          ProfileValidationError::ERROR_CODE_PROFILE_VALIDATION_WARNING
+        );
+      }
+    }
+
+    // Validate custom field mapping.
+    $custom_field_mapping = $this->getAttribute('custom_field_mapping');
+    if (is_string($custom_field_mapping)) {
+      $custom_field_mapping = preg_split('/\r\n|\r|\n/', $custom_field_mapping, -1, PREG_SPLIT_NO_EMPTY);
+      $parsing_error = new ProfileValidationError(
+        'custom_field_mapping',
+        E::ts('Could not parse custom field mapping.'),
+        ProfileValidationError::ERROR_CODE_PROFILE_VALIDATION_FAILED
+      );
+      if (!is_array($custom_field_mapping)) {
+        throw $parsing_error;
+      }
+      foreach ($custom_field_mapping as $custom_field_map) {
+        $custom_field_map = explode('=', $custom_field_map);
+        if (count($custom_field_map) !== 2) {
+          throw $parsing_error;
+        }
+        [$twingle_field_name, $custom_field_name] = $custom_field_map;
+        $custom_field_id = substr($custom_field_name, strlen('custom_'));
+
+        // Check for custom field existence
+        try {
+          /**
+           * @phpstan-var array<string, mixed> $custom_field
+           */
+          $custom_field = civicrm_api3(
+            'CustomField', 'getsingle', ['id' => $custom_field_id]
+          );
+        }
+        catch (CRM_Core_Exception $exception) {
+          throw new ProfileValidationError(
+            'custom_field_mapping',
+            E::ts(
+              'Custom field custom_%1 does not exist.',
+              [1 => $custom_field_id]
+            ),
+            ProfileValidationError::ERROR_CODE_PROFILE_VALIDATION_FAILED,
+            $exception
+          );
+        }
+
+        // Only allow custom fields on relevant entities.
+        try {
+          civicrm_api3('CustomGroup', 'getsingle',
+            [
+              'id' => $custom_field['custom_group_id'],
+              'extends' => [
+                'IN' => [
+                  'Contact',
+                  'Individual',
+                  'Organization',
+                  'Contribution',
+                  'ContributionRecur',
+                ],
+              ],
+            ]);
+        }
+        catch (CRM_Core_Exception $exception) {
+          throw new ProfileValidationError(
+            'custom_field_mapping',
+            E::ts(
+              'Custom field custom_%1 is not in a CustomGroup that extends one of the supported CiviCRM entities.',
+              [1 => $custom_field['id']]
+            ),
+            ProfileValidationError::ERROR_CODE_PROFILE_VALIDATION_FAILED,
+            $exception
+          );
+        }
+      }
+    }
   }
 
   /**
@@ -238,14 +383,11 @@ class CRM_Twingle_Profile {
    * @throws \Civi\Twingle\Exceptions\ProfileException
    */
   public function saveProfile(): void {
-    // make sure it's valid
-    $this->verifyProfile();
-
     try {
-      if ($this->id !== NULL) {
+      if (isset($this->id)) {
         // existing profile -> just update the config
         CRM_Core_DAO::executeQuery(
-          "UPDATE civicrm_twingle_profile SET config = %2, name = %3 WHERE id = %1",
+          'UPDATE civicrm_twingle_profile SET config = %2, name = %3 WHERE id = %1',
           [
             1 => [$this->id, 'String'],
             2 => [json_encode($this->data), 'String'],
@@ -255,17 +397,20 @@ class CRM_Twingle_Profile {
       else {
         // new profile -> add new entry to the DB
         CRM_Core_DAO::executeQuery(
-          "INSERT IGNORE INTO civicrm_twingle_profile(name,config,last_access,access_counter) VALUES (%1, %2, null, 0)",
+          <<<SQL
+          INSERT IGNORE INTO civicrm_twingle_profile(name,config,last_access,access_counter) VALUES (%1, %2, null, 0)
+          SQL,
           [
             1 => [$this->name, 'String'],
             2 => [json_encode($this->data), 'String'],
           ]);
       }
     }
-    catch (Exception $e) {
+    catch (Exception $exception) {
       throw new ProfileException(
-        E::ts("Could not save/update profile: %1", [1 => $e->getMessage()]),
+        E::ts('Could not save/update profile: %1', [1 => $exception->getMessage()]),
-        ProfileException::ERROR_CODE_COULD_NOT_SAVE_PROFILE
+        ProfileException::ERROR_CODE_COULD_NOT_SAVE_PROFILE,
+        $exception
       );
     }
   }
@@ -275,7 +420,7 @@ class CRM_Twingle_Profile {
    *
    * @throws \Civi\Twingle\Exceptions\ProfileException
    */
-  public function deleteProfile() {
+  public function deleteProfile(): void {
     // Do only reset default profile
     if ($this->getName() == 'default') {
       try {
@@ -284,31 +429,31 @@ class CRM_Twingle_Profile {
         $default_profile->saveProfile();
 
         // Reset counter
-        CRM_Core_DAO::executeQuery("UPDATE civicrm_twingle_profile SET access_counter = 0, last_access = NULL WHERE id = %1", [
+        CRM_Core_DAO::executeQuery(
-          1 => [
+          'UPDATE civicrm_twingle_profile SET access_counter = 0, last_access = NULL WHERE id = %1',
-            $this->id,
+          [1 => [$this->id, 'Integer']]
-            'Integer'
+        );
-          ]
+      }
-        ]);
+      catch (Exception $exception) {
-      } catch (Exception $e) {
         throw new ProfileException(
-          E::ts("Could not reset default profile: %1", [1 => $e->getMessage()]),
+          E::ts('Could not reset default profile: %1', [1 => $exception->getMessage()]),
-          ProfileException::ERROR_CODE_COULD_NOT_RESET_PROFILE
+          ProfileException::ERROR_CODE_COULD_NOT_RESET_PROFILE,
+          $exception
         );
       }
     }
     else {
       try {
-        CRM_Core_DAO::executeQuery("DELETE FROM civicrm_twingle_profile WHERE id = %1", [
+        CRM_Core_DAO::executeQuery(
-          1 => [
+          'DELETE FROM civicrm_twingle_profile WHERE id = %1',
-            $this->id,
+          [1 => [$this->id, 'Integer']]
-            'Integer'
+        );
-          ]
+      }
-        ]);
+      catch (Exception $exception) {
-      } catch (Exception $e) {
         throw new ProfileException(
-          E::ts("Could not delete profile: %1", [1 => $e->getMessage()]),
+          E::ts('Could not delete profile: %1', [1 => $exception->getMessage()]),
-          ProfileException::ERROR_CODE_COULD_NOT_DELETE_PROFILE
+          ProfileException::ERROR_CODE_COULD_NOT_DELETE_PROFILE,
+          $exception
         );
       }
     }
@@ -487,11 +632,20 @@ class CRM_Twingle_Profile {
    * @throws \Civi\Twingle\Exceptions\ProfileException
    */
   public static function getProfile(int $id = NULL) {
-    if (!empty($id)) {
+    if (isset($id)) {
-      $profile_data = CRM_Core_DAO::executeQuery("SELECT id, name, config FROM civicrm_twingle_profile WHERE id = %1",
+      /**
-        [1 => [$id, 'Integer']]);
+       * @var CRM_Core_DAO $profile_data
+       */
+      $profile_data = CRM_Core_DAO::executeQuery(
+        'SELECT id, name, config FROM civicrm_twingle_profile WHERE id = %1',
+        [1 => [$id, 'Integer']]
+      );
       if ($profile_data->fetch()) {
-        return new CRM_Twingle_Profile($profile_data->name, json_decode($profile_data->config, 1), (int) $profile_data->id);
+        return new CRM_Twingle_Profile(
+          $profile_data->name,
+          json_decode($profile_data->config, TRUE),
+          (int) $profile_data->id
+        );
       }
     }
     throw new ProfileException('Profile not found.', ProfileException::ERROR_CODE_PROFILE_NOT_FOUND);
@@ -501,16 +655,23 @@ class CRM_Twingle_Profile {
    * Retrieves the list of all profiles persisted within the current CiviCRM
    * settings, including the default profile.
    *
-   * @return array<string, \CRM_Twingle_Profile>
+   * @return array<int, \CRM_Twingle_Profile>
-   *   profile_name => CRM_Twingle_Profile
+   *   An array of profiles with profile IDs as keys and profile objects as values.
    * @throws \Civi\Core\Exception\DBQueryException
    */
-  public static function getProfiles() {
+  public static function getProfiles(): array {
     // todo: cache?
     $profiles = [];
-    $profile_data = CRM_Core_DAO::executeQuery("SELECT id, name, config FROM civicrm_twingle_profile");
+    /**
+     * @var CRM_Core_DAO $profile_data
+     */
+    $profile_data = CRM_Core_DAO::executeQuery('SELECT id, name, config FROM civicrm_twingle_profile');
     while ($profile_data->fetch()) {
-      $profiles[$profile_data->id] = new CRM_Twingle_Profile($profile_data->name, json_decode($profile_data->config, 1), (int) $profile_data->id);
+      $profiles[(int) $profile_data->id] = new CRM_Twingle_Profile(
+        $profile_data->name,
+        json_decode($profile_data->config, TRUE),
+        (int) $profile_data->id
+      );
     }
     return $profiles;
   }
@@ -518,17 +679,20 @@ class CRM_Twingle_Profile {
   /**
    * Get the stats (access_count, last_access) for all twingle profiles
    *
-   * @return CRM_Twingle_Profile[]
+   * @return array<string, array<string, mixed>>
    * @throws \Civi\Core\Exception\DBQueryException
    */
   public static function getProfileStats() {
     $stats = [];
+    /**
+     * @var CRM_Core_DAO $profile_data
+     */
     $profile_data = CRM_Core_DAO::executeQuery(
       'SELECT name, last_access, access_counter FROM civicrm_twingle_profile'
     );
     while ($profile_data->fetch()) {
       // phpcs:disable Drupal.Arrays.Array.ArrayIndentation
-      $stats[$profile_data->name] = [
+      $stats[(string) $profile_data->name] = [
         'name' => $profile_data->name,
         'last_access' => $profile_data->last_access,
         'last_access_txt' => $profile_data->last_access

Civi/Twingle/Exceptions/BaseException.php

@@ -28,21 +28,21 @@ use CRM_Twingle_ExtensionUtil as E;
 class BaseException extends \Exception {
 
   /**
-   * @var int|string
+   * @var string
    */
   protected $code;
   protected string $log_message;
 
   /**
    * BaseException Constructor
-   * @param string|null $message
+   * @param string $message
    *   Error message
-   * @param string|null $error_code
+   * @param string $error_code
    *   A meaningful error code
-   * @param \Throwable|null $previous
+   * @param \Throwable $previous
    *   A previously thrown exception to include.
    */
-  public function __construct(?string $message = '', ?string $error_code = '', ?\Throwable $previous = NULL) {
+  public function __construct(string $message = '', string $error_code = '', \Throwable $previous = NULL) {
     parent::__construct($message, 1, $previous);
     $this->log_message = '' !== $message ? E::LONG_NAME . ': ' . $message : '';
     $this->code = $error_code;
@@ -61,7 +61,7 @@ class BaseException extends \Exception {
    * @return string
    */
   public function getErrorCode() {
-    return (string) $this->code;
+    return $this->code;
   }
 
 }

Civi/Twingle/Exceptions/ProfileValidationError.php

@@ -38,8 +38,13 @@ class ProfileValidationError extends BaseException {
    * @param string $error_code
    *  A meaningful error code
    */
-  public function __construct(string $affected_field_name, string $message = '', string $error_code = '') {
+  public function __construct(
-    parent::__construct($message, $error_code);
+    string $affected_field_name,
+    string $message = '',
+    string $error_code = '',
+    ?\Throwable $previous = NULL
+  ) {
+    parent::__construct($message, $error_code, $previous);
     $this->affected_field_name = $affected_field_name;
   }