Deduper - Pass arrays rather than strings to construct URLs #11671
Conversation
When we pass a query it is urlencoded and any quotes in the string are not subsequently htmlentity encoded plus I think the url construction is generally cleaner
| @@ -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)); | |||
There was a problem hiding this comment.
FALSE not needed if $urlQuery was passed as an array as keys etc are url encoded
There was a problem hiding this comment.
Just to understand a little here, why could be necessary the "FALSE, NULL; FALSE" attributes?
There was a problem hiding this comment.
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
totten
changed the title
| $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]); |
There was a problem hiding this comment.
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
|
I haven't run the code but the changes all look very safe. I have reviewed the changes and they all seem very sensible and the code looks correct for the changes. I am happy to see this merged |
|
thanks @seamuslee001 -merging based on your review. We are also deploying this live today -based on 4.7.31rc + a few patches including this one |
Overview
Minor code cleanup
Before
CRM_Utils_System::url() receiving $query as a string
After
CRM_Utils_System::url() receiving $query as an array
Technical Details
When we pass a query it is urlencoded and any quotes in the string are not subsequently htmlentity encoded plus I think the url construction code is generally cleaner when not constructing urls as strings