Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deduper - Pass arrays rather than strings to construct URLs #11671

Merged
merged 1 commit into from
Feb 20, 2018
Merged

Deduper - Pass arrays rather than strings to construct URLs #11671

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 28 additions & 19 deletions CRM/Contact/Form/Merge.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,8 @@ public function preProcess() {
$this->_gid = $gid = CRM_Utils_Request::retrieve('gid', 'Positive', $this, FALSE);
$this->_mergeId = CRM_Utils_Request::retrieve('mergeId', 'Positive', $this, FALSE);
$this->limit = CRM_Utils_Request::retrieve('limit', 'Positive', $this, FALSE);
$urlParams = "reset=1&rgid={$this->_rgid}&gid={$this->_gid}&limit=" . $this->limit;

$urlParams = ['reset' => 1, 'rgid' => $this->_rgid, 'gid' => $this->_gid, 'limit' => $this->limit];

$this->bounceIfInvalid($this->_cid, $this->_oid);

Expand All @@ -83,7 +84,7 @@ public function preProcess() {
'return' => 'contact_type',
));

$browseUrl = CRM_Utils_System::url('civicrm/contact/dedupefind', $urlParams . '&action=browse');
$browseUrl = CRM_Utils_System::url('civicrm/contact/dedupefind', array_merge($urlParams, ['action' => 'browse']));

if (!$this->_rgid) {
// Unset browse URL as we have come from the search screen.
Expand Down Expand Up @@ -124,13 +125,13 @@ public function preProcess() {
$this->assign('mainUfId', $mainUfId);
$this->assign('mainUfName', $mainUser ? $mainUser->name : NULL);
}

$flipUrl = CRM_Utils_System::url('civicrm/contact/merge',
"reset=1&action=update&cid={$this->_oid}&oid={$this->_cid}&rgid={$this->_rgid}&gid={$gid}"
);
$flipParams = array_merge($urlParams, ['action' => 'update', 'cid' => $this->_oid, 'oid' => $this->_cid]);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I note that in the original there is no limit but using array_merge suggests we will get a limit. probably not the worst thing

if (!$flip) {
$flipUrl .= '&flip=1';
$flipParams['flip'] = '1';
}
$flipUrl = CRM_Utils_System::url('civicrm/contact/merge',
$flipParams
);
$this->assign('flip', $flipUrl);

$this->prev = $this->next = NULL;
Expand All @@ -140,8 +141,13 @@ public function preProcess() {
) as $position) {
if (!empty($pos[$position])) {
if ($pos[$position]['id1'] && $pos[$position]['id2']) {
$urlParams .= "&cid={$pos[$position]['id1']}&oid={$pos[$position]['id2']}&mergeId={$pos[$position]['mergeId']}&action=update";
$this->$position = CRM_Utils_System::url('civicrm/contact/merge', $urlParams);
$rowParams = array_merge($urlParams, [
'action' => 'update',
'cid' => $pos[$position]['id1'],
'oid' => $pos[$position]['id2'],
'mergeId' => $pos[$position]['mergeId'],
]);
$this->$position = CRM_Utils_System::url('civicrm/contact/merge', $rowParams);
$this->assign($position, $this->$position);
}
}
Expand Down Expand Up @@ -294,18 +300,17 @@ public function postProcess() {
$message = '<ul><li>' . ts('%1 has been updated.', array(1 => $name)) . '</li><li>' . ts('Contact ID %1 has been deleted.', array(1 => $this->_oid)) . '</li></ul>';
CRM_Core_Session::setStatus($message, ts('Contacts Merged'), 'success');

$url = CRM_Utils_System::url('civicrm/contact/view', "reset=1&cid={$this->_cid}");
$urlParams = "reset=1&gid={$this->_gid}&rgid={$this->_rgid}&limit={$this->limit}";
$urlParams = ['reset' => 1, 'cid' => $this->_cid, 'rgid' => $this->_rgid, 'gid' => $this->_gid, 'limit' => $this->limit];
$contactViewUrl = CRM_Utils_System::url('civicrm/contact/view', ['reset' => 1, 'cid' => $this->_cid]);

if (!empty($formValues['_qf_Merge_submit'])) {
$urlParams .= "&action=update";
$lisitingURL = CRM_Utils_System::url('civicrm/contact/dedupefind',
$urlParams['action'] = "update";
CRM_Utils_System::redirect(CRM_Utils_System::url('civicrm/contact/dedupefind',
$urlParams
);
CRM_Utils_System::redirect($lisitingURL);
));
}
if (!empty($formValues['_qf_Merge_done'])) {
CRM_Utils_System::redirect($url);
CRM_Utils_System::redirect($contactViewUrl);
}

if ($this->next && $this->_mergeId) {
Expand All @@ -321,12 +326,16 @@ public function postProcess() {
$pos['next']['id2']
) {

$urlParams .= "&cid={$pos['next']['id1']}&oid={$pos['next']['id2']}&mergeId={$pos['next']['mergeId']}&action=update";
$url = CRM_Utils_System::url('civicrm/contact/merge', $urlParams);
$urlParams['cid'] = $pos['next']['id1'];
$urlParams['oid'] = $pos['next']['id2'];
$urlParams['mergeId'] = $pos['next']['mergeId'];
$urlParams['action'] = 'update';
CRM_Utils_System::redirect(CRM_Utils_System::url('civicrm/contact/merge', $urlParams));
}
}

CRM_Utils_System::redirect($url);
// Perhaps never reached.
CRM_Utils_System::redirect($contactViewUrl);
}

/**
Expand Down
11 changes: 9 additions & 2 deletions CRM/Contact/Page/AJAX.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -808,9 +808,16 @@ public static function getDedupes() {
$searchRows[$count]['weight'] = CRM_Utils_Array::value('weight', $pair);

if (!empty($pairInfo['data']['canMerge'])) {
$mergeParams = "reset=1&cid={$pairInfo['entity_id1']}&oid={$pairInfo['entity_id2']}&action=update&rgid={$rgid}&limit=" . CRM_Utils_Request::retrieve('limit', 'Integer');
$mergeParams = [
'reset' => 1,
'cid' => $pairInfo['entity_id1'],
'oid' => $pairInfo['entity_id2'],
'action' => 'update',
'rgid' => $rgid,
'limit' => CRM_Utils_Request::retrieve('limit', 'Integer'),
];
if ($gid) {
$mergeParams .= "&gid={$gid}";
$mergeParams['gid'] = $gid;
}

$searchRows[$count]['actions'] = "<a class='crm-dedupe-flip' href='#' data-pnid={$pairInfo['prevnext_id']}>" . ts('flip') . "</a>&nbsp;|&nbsp;";
Expand Down
2 changes: 1 addition & 1 deletion CRM/Contact/Page/DedupeFind.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,7 @@ public function run() {
$urlQry['selected'] = 1;
}

$this->assign('sourceUrl', CRM_Utils_System::url('civicrm/ajax/dedupefind', $urlQry, FALSE, NULL, FALSE));
$this->assign('sourceUrl', CRM_Utils_System::url('civicrm/ajax/dedupefind', $urlQry));
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FALSE not needed if $urlQuery was passed as an array as keys etc are url encoded

Copy link
Contributor

@Elennio Elennio Feb 20, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to understand a little here, why could be necessary the "FALSE, NULL; FALSE" attributes?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the first 2 params - FALSE & NULL are the defaults for CRM_Utils_System::url() but the 3rd one (FALSE) is for $htmlize and we see

if ($htmlize) {
  $url = htmlentities($url);
}

I'm pretty confident that FALSE was added to compensate for a time in the past when $urlQuery was being passed as a string. When passing $urlQuery as a string any special characters are preserved & then they get kinda mangled (from a url POV) later when htmlentities happens.

However, when passed as an array both the key & the value are passed through url_encode - which means that something odd like a " is already converted to %22 when it hits the htmlentities function & not further converted.

In general url_encode makes sense for handling urls & html_entities for things to be displayed via html. The presence of htmlentities in that function is a bit odd really


//reload from cache table
$cacheKeyString = CRM_Dedupe_Merger::getMergeCacheKeyString($rgid, $gid, $criteria);
Expand Down