-
-
Notifications
You must be signed in to change notification settings - Fork 825
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.
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
[Ref] Move id fetching to the classes #21075
Conversation
(Standard links)
|
CRM/Contact/Form/Task/EmailTrait.php
Outdated
* | ||
* This raises questions - | ||
* 1) can it be something other than an integer | ||
* 2) is the right class for this the activity email task? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For 1, it used to be - see https://lab.civicrm.org/dev/core/-/issues/2463
For 2, I don't have a thoughtful answer at the moment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@demeritcowboy so for clarity we could do
return $this->_caseId ? (int) $this->_caseId : NULL;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@demeritcowboy for 2 - maybe I can copy the function onto that class & in this class have
protected function getCaseID() {
if ($this->_caseId) {
CRM_Core_Error::deprecatedWarning('case Id should only be present for ctivity emails - how did you get here');
return (int) $this->_caseId;
}
return NULL
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've updated the PR to how I think it should look (as long as I'm right about case id being an activity only thing)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still thinking, but as a practical matter the new deprecation comes up if you just send an email from Manage Case (i.e. from within the roles section), so it's obviously currently going thru there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I thought that was the activity class - will track it down
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@demeritcowboy I toned it down to still keeping the case stuff on the Trait
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the contribution one I get some warnings/notices, but it's pre-existing (e.g. find contributions - actions - send email). Looks good.
c53bd0e
to
db4d540
Compare
db4d540
to
bc6e899
Compare
Overview
[Ref] Move id fetching to the classes
Before
Shared class tries to retrieve contributionIDs, caseId from properties - it shouldn't need to know about them
After
For contribution it's moved to the contribution class - but case leaves me with questions - so I though I'd open this & ask Dave - @demeritcowboy - do you think the handling for _caseId rightly belongs on
CRM_Activity_Form_Task_Email
Technical Details
Comments