Fix e-notice & php8.2 issue on user dashboard #25545
Conversation
|
(Standard links)
|
| if ($userChecksum) { | ||
| $this->assign('userChecksum', $userChecksum); | ||
| $validUser = CRM_Contact_BAO_Contact_Utils::validChecksum($this->_contactId, $userChecksum); | ||
| $this->_isChecksumUser = $validUser; |
There was a problem hiding this comment.
@seamuslee001 this undeclared property would be php 8.2 incompatible
| @@ -154,7 +151,7 @@ public function buildUserDashBoard() { | |||
| $this->assign('pcpInfo', $pcpInfo); | |||
| } | |||
|
|
|||
| if (!empty($dashboardOptions['Assigned Activities']) && empty($this->_isChecksumUser)) { | |||
| if (!empty($dashboardOptions['Assigned Activities']) && !$this->getUserChecksum()) { | |||
There was a problem hiding this comment.
undeclared property replaced by function
| */ | ||
| protected function getUserChecksum() { | ||
| $userChecksum = CRM_Utils_Request::retrieve('cs', 'String', $this); | ||
| if (empty($userID) && $this->_contactId) { | ||
| if ($this->_contactId && CRM_Contact_BAO_Contact_Utils::validChecksum($this->_contactId, $userChecksum)) { |
There was a problem hiding this comment.
Related: https://lab.civicrm.org/dev/core/-/issues/2724 but I'd need to get my head back into it.
There was a problem hiding this comment.
Ah right - I assume that we could put back in a getLoggedInUser check there then
There was a problem hiding this comment.
@demeritcowboy I took a look at this again & I'm inclined to close https://lab.civicrm.org/dev/core/-/issues/2724 and keep this.
The usages of the return value are
- to determine it the contact can add themselve to groups - if they used a checksum link in the url then no.
- to decide whether to inclue a checksum in a url for the transact page - if checksum was used then yes.
The impact of this PR is
1 - unchanged. This does mean that if they authenticated by checksum AND are otherwise logged in they can't edit their groups. No-one noticed in the last year & my mind spins at what scenario this could matter in
2 - is slightly changed to 'if checksum was used & it is valid then yes by this pr
|
test this please |
Test Result (1 failure / +1)api_v3_JobProcessMailingTest.testConcurrency with data set #1Test Result (1 failure / +1) |
|
test this please - lets' get a fresh test run |
Overview
Fix e-notice & php8.2 issue on user dashboard
This addresses the first notice on the image below - but on looking I realised that there was also use of an undefined property. I removed it in favour of a function to be called.
Before
After
Technical Details
I also moved the validation into the
getCheckSumfunction - the checksum is only returned if_contactIdis set (I considered renaming the property tocontactIDbut sometimes onCRM_Core_Formthe 2 variables are used differently....) so it makes sense to validate before returning rather than remember in the calling code. The Contribution dash also calls this code but the extra check seems fine or even goodComments