flexmailer - Update [CiviMail Draft] prefix to match BAO behavior
#21715
Conversation
|
(Standard links)
|
|
@totten Stacktrace |
| @@ -41,11 +41,23 @@ public static function preview($apiRequest) { | |||
| $contactID = \CRM_Utils_Array::value('contact_id', $params, | |||
| \CRM_Core_Session::singleton()->get('userID')); | |||
|
|
|||
| $job = new \CRM_Mailing_BAO_MailingJob(); | |||
| $job = new class extends \CRM_Mailing_BAO_MailingJob { | |||
There was a problem hiding this comment.
Non - blocking - but I definitely find it much easier to read / search for when we define classes in their own file rather than this on-the-fly way
There was a problem hiding this comment.
Yeah, I do see the slippery-slope argument where some class may be misjudged as "anonymous/strictly-hands-off to others" -- and then create some cost around discovering that this anonymous class is actually a supported interface.
At the same time, suppose we did a class CRM_Mailing_DAO_MailingJob_SaveProhibited. Future-person would be just as likely to read too much into it -- eg taking it as an exemplar to emulate in new CRM_*_DAO_*_SaveProhibited classes, and/or rightly objecting that SaveProhibited hierarchy conflicts with the BAO hierarchy, and/or feeling an obligation to support some external evil like if ($dao instance of CRM_Mailing_DAO_MailingJob_SaveProhibited) { activateThirdPartyBits(); }.
It seems to me - if one wants to be a good precedent - then that's undertaking a bigger task: systemically adding a freeze-mode toCRM_Core_DAO. Maybe that's a good feature? But it seems a bit by-the-by?
There was a problem hiding this comment.
@totten to be honest I don't really understand this code - I merged based on the tests - I find the cognitve load of classes declared on the fly to be really high.
If you had declared an actual class in a file like
class CRM_Mailing_BAO_Mailing_Job_PreviewMode extends \CRM_Mailing_BAO_MailingJob
it would have been easy to read.
I've always thought stuff like this was 'complicated stuff that Tim understands' but I actually had to debug some code like this the other day & I found that if I re-wrote constructs like this before trying to read it & also renaming all the abbreviations to words I could make sense of it.
|
Merging - this seems to 'do the trick' & per my comment the readability issue is non-blocking |
Overview
When drafting a mail-blast, one may use in-app previews and/or email test-messages. This refines the way in which the
[CiviMail Draft]prefix is applied - and updates Flexmailer to more precisely match BAO.Before
After
Technical Details
The behavior of
[CiviMail Draft]prefix is keyed off the propertycivicrm_mailing_job.is_test. The functional change here comes from twiddlingis_test=1=>is_test=0forpreviewaction.In the case of previewing, there is no persistent
civicrm_mailing_job, but there is a placeholder/mock-job (allowing the preview to more completely exercise representative code-paths). It doesn't matter much what data we put in this mock job - as long as it isn't saved. Theis_test=1was a slight mitigation in case it was accidentally saved. This patch replaces that with a more forthright mitigation to prevent accidental saving of the mock-job.