-
-
Notifications
You must be signed in to change notification settings - Fork 824
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] Minor cleanup in CiviMail #31813
base: master
Are you sure you want to change the base?
[REF] Minor cleanup in CiviMail #31813
Conversation
🤖 Thank you for contributing to CiviCRM! ❤️ We will need to test and review this PR. 👷 Introduction for new contributors...
Quick links for reviewers...
|
* Return a list of group names for this mailing. Does not work with | ||
* prior-mailing targets. | ||
* | ||
* @deprecated since 5.71 will be removed around 5.77 |
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.
Looking for callers, I see:
getGroupNames
is called by...CRM_Utils_Token::getMailingTokenReplacement
, which is called by:CRM_Mailing_Tokens::evaluateToken()
, and...CRM_Utils_Token::replaceMailingTokens()
, which is called by:CRM_SMS_Form_Upload
, and...CRM_Mailing_Event_BAO_MailingEventResubscribe
(edited to use nested layout)
It might be that...
- The code-paths are dead for other/higher-level reason (in which case more can be cleaned), or....
- These specific callers lack test-coverage for the
{mailing.group}
token.
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.
This deprecation was suggested in f31f94b. @eileenmcnaughton, does this actually look safe to remove?
Overview
CRM_Mailing_BAO_Mailing::getGroupNames
(scheduled for removal: around 5.77)Civi::log()->error
instead ofCRM_Core_Error::debug_log_message