dev/core#2463 - Adjust returned list of activity ids when sending emails since it changes in 5.36+

[demeritcowboy]

Overview

https://lab.civicrm.org/dev/core/-/issues/2463

Followup to #19826 and towards making the recorded activity ids available to hooks.

I'm a bit conflicted about changing the signature for a public static function. But at the same time any extensions that call this function and act on the single activity id it returns are now doing it wrong as of 5.36, since they're missing all the other activity ids. Universe suggests there's a small handful. An alternative would be to keep the signature and provide some other way to get at the ids, which doesn't seem hard, but does it really help the existing extensions?

Before

In 5.35 and earlier, only one activity was recorded when there were multiple recipients when sending a (non-bulk) email. In 5.36 it now records multiple activities, but the code still returns just the last activity id to its caller.

After

Returns all the ids.

Technical Details

One thing that came up during the original review of the multiple activities PR had to do with any scheduled followups. I've left that as-is so it still only links the followup to one of the activity ids. The original quote:

For scheduled followups (the section at the bottom of the email form where you can create a supplementary activity at the same time as sending the email), it doesn't quite match my understanding of the discussion and the "multiple" policy, but this doesn't have anything to do with tokens and seems fine to me. In the multiple policy, there'd be a separate followup activity for each To recipient, but I think it's fine that it creates just one with the combined To's. If you really want separate ones, you can still make more manually.

Comments

This doesn't do anything with the returned ids yet. Making it available to hooks would be a followup.

There's some coverage for multiple recipients at testSendEmailWillReplaceTokensUniquelyForEachContact. But I should probably add something to explicitly test the returned values.

[civibot]

(Standard links)

• If this is your first pull-request for CiviCRM, please browse CONTRIBUTING.md for information about the development and testing processes.
• If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

[eileenmcnaughton]

@demeritcowboy does this need to be against 5.36?

[demeritcowboy]

The two most straightforward choices seem to be:

1. Yes. In which case it would first need 19826 backported, and then after this PR a small handful of extensions may fail in 5.36 (if they attempt to use array as integer) unless the devs do something in the next 2 weeks.
2. No. In which case 5.36 would mean a small handful of extensions continue to work but don't completely act on all the resulting activities, until whenever either this PR or something else happens, and then they may fail.

[eileenmcnaughton]

@demeritcowboy doesn't sound like a compelling case for putting it against 5.36 if it increases the risk of breakage :-)

I'll take a look in a bit

[demeritcowboy]

They'll be broken either way in 5.36, just different kinds of broken. The fact that some extensions call sendEmail and act on the resulting activity was unnoticed/unforseen during the change to store the replacement values of tokens in separate activities per recipient.

[artfulrobot]

This comment is not clear, I think an improvement would be:

Suggested change

	* ( sent, activityIds) if any email is sent and the corresponding activity ids
	* [ bool sent, array activityIds ] if any email is sent and the corresponding activity ids

I'm also not sure it's correct. $sent is initialised to [] but then set FALSE and maybe TRUE in the loop; so it's only referring to the last loop iteration. Seems the same problem as for $activityId(s)

[demeritcowboy]

Good point.

[demeritcowboy]

I'm going to look at the $sent issue separately since it's been like that since 2009. Kudos for noticing.

[eileenmcnaughton]

@artfulrobot thanks for taking a look - I'm happy to merge this when you are happy with it

[demeritcowboy]

Addressed the other comments and added an explicit test for the returned ids.

[artfulrobot]

Looks good to me, thanks @demeritcowboy

[demeritcowboy]

Thanks for the review!

[eileenmcnaughton]

Thanks @artfulrobot & @demeritcowboy