aghstrategies / com.aghstrategies.airmail

Unified CiviCRM bounce event handler for SMTP services
Other
8 stars 13 forks source link

CRM_Airmail_Backend_Elastic::processMessages() issues missing index warnings #34

Open bugfolder opened 1 year ago

bugfolder commented 1 year ago

After running a mail job, I see a series of "missing index" warnings in the Backdrop CMS dblog. Some of them are like this:

Notice: Undefined index: postback in CRM_Airmail_Backend_Elastic->processMessages() (line 21 of /mysite/civicrm_extensions/com.aghstrategies.airmail/CRM/Airmail/Backend/Elastic.php).

A sanitized version of the postback is

https://mysite.com/civicrm/airmail/webhook?reset=1&secretcode=SECRET_CODE&transaction=TRANSACTION&to=someone%40theirsite.com&from=admin%40mysite.com&date=8%2f1%2f2023+8%3a43%3a55+PM&status=Error&channel=SMTP+API&account=website%40mysite.com&category=NotDelivered&subject=Opt-out+Confirmation&messageid=MESSAGE_ID

A second type of warning (which comes in groups of 3) is

Notice: Trying to access array offset on value of type null in CRM_Airmail_Backend_Elastic->processMessages() (line 23 of /mysite/civicrm_extensions/com.aghstrategies.airmail/CRM/Airmail/Backend/Elastic.php).

Warnings are issued for lines 23, 24, and 25. A sanitized version of the postback is

http://mysite.com/civicrm/airmail/webhook?reset=1&secretcode=SECRET_CODE&transaction=TRANSACTION&to=someone%40theirsite.com&from=admin%40mysite.com&date=8%2f1%2f2023+8%3a41%3a02+PM&status=Error&channel=SMTP+API&account=website%40mysite.com&category=NotDelivered&postback=admin%40mysite.com&subject=MY_EMAIL_SUBJECT&messageid=MESSAGE_ID

The solution would appear to be to simply check for the existence of the array elements in question and then return from the function if they're missing; but maybe there's additional actions that should be taken if they're missing?

Using Airmail 2.2, CiviCRM 5.63.1, and Backdrop CMS 1.25.1.

chrisgaraffa commented 1 year ago

Hi @bugfolder, thanks for the report.

Do you have "Allow custom headers" enabled on your Sending Settings screen? Airmail sets the X-ElasticEmail-Postback header here so we can attempt to properly attribute the bounce to the mailing. According to Elastic's Email's documentation (under the "Category" parameter values), postback does require having that setting enabled.

It would be ideal to have that setting turned on, but we can look at possibly working around these warnings.

bugfolder commented 1 year ago

Hi, thanks for replying and for the analysis. No, I didn't have that turned on in EE. I found it and turned it on now. I'll check the results when the next mailing goes out.

bugfolder commented 1 year ago

Hi @chrisgaraffa, I definitely have "Custom headers" turned on in my advance sending settings:

image

But I am still seeing PHP warnings of the sort given above.

Would it be sufficient to simply check for existence and return from the function if they're missing, or is there different logic that should be implemented?

elisseck commented 1 year ago

@bugfolder sorry to hear you're still having these issues with custom headers turned on.

The parameters job_id, event_queue_id, and hash do need to be passed thru from the original Civi mailing.

You could check for existence to suppress the notices, but they are required parameters for the api action processed here and bounces will not be correctly logged without them. I imagine you'd see these API errors like "Airmail API error (bounce)\n" . $e->getMessage() in the Civi log as well.

Unfortunately we no longer use Elastic Email so I don't have a great way to test this currently, but if you are able to figure it out whether CiviMail is failing to send these parameters or Elastic is dropping these parameters that could lead somewhere, OR if you're able to submit a PR with a fix for the issue I would be happy to review it.