Merge pull request #10068 from agh1/crm-20351

CRM-20351 Multiple pages repeat stuff in the page run() method, invoking hooks multiple times
civicrm · Jul 10, 2017 · cd57d10 · cd57d10
2 parents 7bfe1d0 + 28c6453
commit cd57d10
Show file tree

Hide file tree

Showing 14 changed files with 255 additions and 216 deletions.
diff --git a/CRM/ACL/Page/ACL.php b/CRM/ACL/Page/ACL.php
@@ -90,23 +90,9 @@ public function &links() {
   /**
    * Run the page.
    *
-   * This method is called after the page is created. It checks for the
-   * type of action and executes that action.
-   * Finally it calls the parent's run method.
+   * Set the breadcrumb before beginning the standard page run.
    */
   public function run() {
-    // get the requested action
-    $action = CRM_Utils_Request::retrieve('action', 'String',
-      // default to 'browse'
-      $this, FALSE, 'browse'
-    );
-
-    // assign vars to templates
-    $this->assign('action', $action);
-    $id = CRM_Utils_Request::retrieve('id', 'Positive',
-      $this, FALSE, 0
-    );
-
     // set breadcrumb to append to admin/access
     $breadCrumb = array(
       array(
@@ -117,24 +103,6 @@ public function run() {
       ),
     );
     CRM_Utils_System::appendBreadCrumb($breadCrumb);
-    // what action to take ?
-    if ($action & (CRM_Core_Action::ADD | CRM_Core_Action::DELETE)) {
-      $this->edit($action, $id);
-    }
-
-    if ($action & (CRM_Core_Action::UPDATE)) {
-      $this->edit($action, $id);
-
-      if (isset($id)) {
-        $aclName = CRM_Core_DAO::getFieldValue('CRM_ACL_DAO_ACL', $id);
-        CRM_Utils_System::setTitle(ts('Edit ACL -  %1', array(1 => $aclName)));
-      }
-    }
-
-    // finally browse the acl's
-    if ($action & CRM_Core_Action::BROWSE) {
-      $this->browse();
-    }
 
     // parent run
     return parent::run();
@@ -193,22 +161,22 @@ public function browse() {
 
       switch ($acl[$dao->id]['object_table']) {
         case 'civicrm_saved_search':
-          $acl[$dao->id]['object'] = $group[$acl[$dao->id]['object_id']];
+          $acl[$dao->id]['object'] = CRM_Utils_Array::value($acl[$dao->id]['object_id'], $group);
           $acl[$dao->id]['object_name'] = ts('Group');
           break;
 
         case 'civicrm_uf_group':
-          $acl[$dao->id]['object'] = $ufGroup[$acl[$dao->id]['object_id']];
+          $acl[$dao->id]['object'] = CRM_Utils_Array::value($acl[$dao->id]['object_id'], $ufGroup);
           $acl[$dao->id]['object_name'] = ts('Profile');
           break;
 
         case 'civicrm_custom_group':
-          $acl[$dao->id]['object'] = $customGroup[$acl[$dao->id]['object_id']];
+          $acl[$dao->id]['object'] = CRM_Utils_Array::value($acl[$dao->id]['object_id'], $customGroup);
           $acl[$dao->id]['object_name'] = ts('Custom Group');
           break;
 
         case 'civicrm_event':
-          $acl[$dao->id]['object'] = $event[$acl[$dao->id]['object_id']];
+          $acl[$dao->id]['object'] = CRM_Utils_Array::value($acl[$dao->id]['object_id'], $event);
           $acl[$dao->id]['object_name'] = ts('Event');
           break;
       }
@@ -269,4 +237,26 @@ public function userContext($mode = NULL) {
     return 'civicrm/acl';
   }
 
+  /**
+   * Edit an ACL.
+   *
+   * @param int $mode
+   *   What mode for the form ?.
+   * @param int $id
+   *   Id of the entity (for update, view operations).
+   * @param bool $imageUpload
+   *   Not used in this case, but extended from CRM_Core_Page_Basic.
+   * @param bool $pushUserContext
+   *   Not used in this case, but extended from CRM_Core_Page_Basic.
+   */
+  public function edit($mode, $id = NULL, $imageUpload = FALSE, $pushUserContext = TRUE) {
+    if ($mode & (CRM_Core_Action::UPDATE)) {
+      if (isset($id)) {
+        $aclName = CRM_Core_DAO::getFieldValue('CRM_ACL_DAO_ACL', $id);
+        CRM_Utils_System::setTitle(ts('Edit ACL &ndash; %1', array(1 => $aclName)));
+      }
+    }
+    parent::edit($mode, $id, $imageUpload, $pushUserContext);
+  }
+
 }
diff --git a/CRM/ACL/Page/ACLBasic.php b/CRM/ACL/Page/ACLBasic.php
@@ -83,17 +83,7 @@ public function &links() {
    * Finally it calls the parent's run method.
    */
   public function run() {
-    // get the requested action
-    $action = CRM_Utils_Request::retrieve('action', 'String',
-      // default to 'browse'
-      $this, FALSE, 'browse'
-    );
-
-    // assign vars to templates
-    $this->assign('action', $action);
-    $id = CRM_Utils_Request::retrieve('id', 'Positive',
-      $this, FALSE, 0
-    );
+    $id = $this->getIdAndAction();
 
     // set breadcrumb to append to admin/access
     $breadCrumb = array(
@@ -105,15 +95,15 @@ public function run() {
     CRM_Utils_System::appendBreadCrumb($breadCrumb);
 
     // what action to take ?
-    if ($action & (CRM_Core_Action::UPDATE | CRM_Core_Action::ADD | CRM_Core_Action::DELETE)) {
-      $this->edit($action, $id);
+    if ($this->_action & (CRM_Core_Action::UPDATE | CRM_Core_Action::ADD | CRM_Core_Action::DELETE)) {
+      $this->edit($this->_action, $id);
     }
 
     // finally browse the acl's
     $this->browse();
 
-    // parent run
-    return parent::run();
+    // This replaces parent run, but do parent's parent run
+    return CRM_Core_Page::run();
   }
 
   /**

diff --git a/CRM/ACL/Page/EntityRole.php b/CRM/ACL/Page/EntityRole.php
@@ -95,17 +95,7 @@ public function &links() {
    * Finally it calls the parent's run method.
    */
   public function run() {
-    // get the requested action
-    $action = CRM_Utils_Request::retrieve('action', 'String',
-      // default to 'browse'
-      $this, FALSE, 'browse'
-    );
-
-    // assign vars to templates
-    $this->assign('action', $action);
-    $id = CRM_Utils_Request::retrieve('id', 'Positive',
-      $this, FALSE, 0
-    );
+    $id = $this->getIdAndAction();
 
     // set breadcrumb to append to admin/access
     $breadCrumb = array(
@@ -120,21 +110,21 @@ public function run() {
     CRM_Utils_System::setTitle(ts('Assign Users to Roles'));
 
     // what action to take ?
-    if ($action & (CRM_Core_Action::UPDATE | CRM_Core_Action::ADD | CRM_Core_Action::DELETE)) {
-      $this->edit($action, $id);
+    if ($this->_action & (CRM_Core_Action::UPDATE | CRM_Core_Action::ADD | CRM_Core_Action::DELETE)) {
+      $this->edit($this->_action, $id);
     }
 
     // reset cache if enabled/disabled
-    if ($action & (CRM_Core_Action::DISABLE | CRM_Core_Action::ENABLE)) {
+    if ($this->_action & (CRM_Core_Action::DISABLE | CRM_Core_Action::ENABLE)) {
       CRM_ACL_BAO_Cache::resetCache();
     }
 
     // finally browse the acl's
-    if ($action & CRM_Core_Action::BROWSE) {
+    if ($this->_action & CRM_Core_Action::BROWSE) {
     }
 
-    // parent run
-    return parent::run();
+    // This replaces parent run, but do parent's parent run
+    return CRM_Core_Page::run();
   }
 
   /**

diff --git a/CRM/Contact/Page/DedupeFind.php b/CRM/Contact/Page/DedupeFind.php
@@ -61,7 +61,7 @@ public function run() {
     $action = CRM_Utils_Request::retrieve('action', 'String', $this, FALSE, 0);
     $context = CRM_Utils_Request::retrieve('context', 'String', $this);
     $limit = CRM_Utils_Request::retrieve('limit', 'Integer', $this);
-    $rgid = CRM_Utils_Request::retrieve('rgid', 'Positive');
+    $rgid = CRM_Utils_Request::retrieve('rgid', 'Positive', $this);
     $urlQry = array(
       'reset' => 1,
       'rgid' => $rgid,

diff --git a/CRM/Contact/Page/DedupeRules.php b/CRM/Contact/Page/DedupeRules.php
@@ -99,12 +99,7 @@ public function &links() {
    * method.
    */
   public function run() {
-    // get the requested action, default to 'browse'
-    $action = CRM_Utils_Request::retrieve('action', 'String', $this, FALSE, 'browse');
-
-    // assign vars to templates
-    $this->assign('action', $action);
-    $id = CRM_Utils_Request::retrieve('id', 'Positive', $this, FALSE, 0);
+    $id = $this->getIdAndAction();
 
     $context = CRM_Utils_Request::retrieve('context', 'String', $this, FALSE);
     if ($context == 'nonDupe') {
@@ -116,18 +111,18 @@ public function run() {
     $this->assign('hasperm_merge_duplicate_contacts', CRM_Core_Permission::check('merge duplicate contacts'));
 
     // which action to take?
-    if ($action & (CRM_Core_Action::UPDATE | CRM_Core_Action::ADD)) {
-      $this->edit($action, $id);
+    if ($this->_action & (CRM_Core_Action::UPDATE | CRM_Core_Action::ADD)) {
+      $this->edit($this->_action, $id);
     }
-    if ($action & CRM_Core_Action::DELETE) {
+    if ($this->_action & CRM_Core_Action::DELETE) {
       $this->delete($id);
     }
 
     // browse the rules
     $this->browse();
 
-    // parent run
-    return parent::run();
+    // This replaces parent run, but do parent's parent run
+    return CRM_Core_Page::run();
   }
 
   /**

diff --git a/CRM/Contribute/Page/ManagePremiums.php b/CRM/Contribute/Page/ManagePremiums.php
@@ -105,28 +105,17 @@ public function &links() {
    * Finally it calls the parent's run method.
    */
   public function run() {
-
-    // get the requested action
-    $action = CRM_Utils_Request::retrieve('action', 'String',
-      // default to 'browse'
-      $this, FALSE, 'browse'
-    );
-
-    // assign vars to templates
-    $this->assign('action', $action);
-    $id = CRM_Utils_Request::retrieve('id', 'Positive',
-      $this, FALSE, 0
-    );
+    $id = $this->getIdAndAction();
 
     // what action to take ?
-    if ($action & (CRM_Core_Action::UPDATE | CRM_Core_Action::ADD | CRM_Core_Action::PREVIEW)) {
-      $this->edit($action, $id, TRUE);
+    if ($this->_action & (CRM_Core_Action::UPDATE | CRM_Core_Action::ADD | CRM_Core_Action::PREVIEW)) {
+      $this->edit($this->_action, $id, TRUE);
     }
     // finally browse the custom groups
     $this->browse();
 
     // parent run
-    return parent::run();
+    return CRM_Core_Page::run();
   }
 
   /**

diff --git a/CRM/Core/Page/Basic.php b/CRM/Core/Page/Basic.php
@@ -141,19 +141,7 @@ public function run() {
     $sort = ($n > 2) ? func_get_arg(2) : NULL;
     // what action do we want to perform ? (store it for smarty too.. :)
 
-    $this->_action = CRM_Utils_Request::retrieve('action', 'String', $this, FALSE, 'browse');
-    $this->assign('action', $this->_action);
-
-    // get 'id' if present
-    $id = CRM_Utils_Request::retrieve('id', 'Positive', $this, FALSE, 0);
-
-    require_once str_replace('_', DIRECTORY_SEPARATOR, $this->getBAOName()) . ".php";
-
-    if ($id) {
-      if (!$this->checkPermission($id, NULL)) {
-        CRM_Core_Error::fatal(ts('You do not have permission to make changes to the record'));
-      }
-    }
+    $id = $this->getIdAndAction();
 
     if ($this->_action & (CRM_Core_Action::VIEW |
         CRM_Core_Action::ADD |
@@ -175,6 +163,33 @@ public function run() {
     return parent::run();
   }
 
+  /**
+   * Retrieve the action and ID from the request.
+   *
+   * Action is assigned to the template while we're at it.  This is pulled from
+   * the `run()` method above.
+   *
+   * @return int
+   *   The ID if present, or 0.
+   */
+  public function getIdAndAction() {
+    $this->_action = CRM_Utils_Request::retrieve('action', 'String', $this, FALSE, 'browse');
+    $this->assign('action', $this->_action);
+
+    // get 'id' if present
+    $id = CRM_Utils_Request::retrieve('id', 'Positive', $this, FALSE, 0);
+
+    require_once str_replace('_', DIRECTORY_SEPARATOR, $this->getBAOName()) . ".php";
+
+    if ($id) {
+      if (!$this->checkPermission($id, NULL)) {
+        CRM_Core_Error::fatal(ts('You do not have permission to make changes to the record'));
+      }
+    }
+
+    return $id;
+  }
+
   /**
    * @return string
    */
@@ -288,11 +303,10 @@ public function action(&$object, $action, &$values, &$links, $permission, $force
     $hasDelete = $hasDisable = TRUE;
 
     if (!empty($values['name']) && in_array($values['name'], array(
-        'encounter_medium',
-        'case_type',
-        'case_status',
-      ))
-    ) {
+      'encounter_medium',
+      'case_type',
+      'case_status',
+    ))) {
       static $caseCount = NULL;
       if (!isset($caseCount)) {
         $caseCount = CRM_Case_BAO_Case::caseCount(NULL, FALSE);

diff --git a/CRM/Event/BAO/Participant.php b/CRM/Event/BAO/Participant.php
@@ -2209,8 +2209,7 @@ public static function addActivityForSelection($participantId, $activityType) {
 
     $date = CRM_Utils_Date::currentDBDate();
     $event = CRM_Event_BAO_Event::getEvents(0, $eventId);
-    $eventTitle = $event[$eventId];
-    $subject = "Registration selections changed for $eventTitle";
+    $subject = sprintf("Registration selections changed for %s", CRM_Utils_Array::value($eventId, $event));
     $targetCid = $contactId;
     $srcRecId = $participantId;
 

diff --git a/CRM/Financial/Page/FinancialAccount.php b/CRM/Financial/Page/FinancialAccount.php
@@ -90,30 +90,6 @@ public function &links() {
     return self::$_links;
   }
 
-  /**
-   * Run the page.
-   *
-   * This method is called after the page is created. It checks for the
-   * type of action and executes that action.
-   * Finally it calls the parent's run method.
-   */
-  public function run() {
-    // get the requested action
-    $action = CRM_Utils_Request::retrieve('action', 'String', $this, FALSE, 'browse'); // default to 'browse'
-
-    // assign vars to templates
-    $this->assign('action', $action);
-    $id = CRM_Utils_Request::retrieve('id', 'Positive', $this, FALSE, 0);
-
-    // what action to take ?
-    if ($action & (CRM_Core_Action::UPDATE | CRM_Core_Action::ADD)) {
-      $this->edit($action, $id);
-    }
-
-    // parent run
-    return parent::run();
-  }
-
   /**
    * Browse all custom data groups.
    */