advancedforms / advanced-forms

WordPress plugin to create forms using Advanced Custom Fields
75 stars 14 forks source link

Returning `false` from `af/form/email/recipients` filter doesn't stop email sending #96

Closed te-online closed 2 years ago

te-online commented 2 years ago

The documentation says that returning false from af/form/email/recipients filter stops email sending for that recipient. However, I experience this not to work. Maybe it's my implementation, but I've tried several different things and nothing seems to resolve the issue.

This is my simplified implementation

add_filter('af/form/email/recipient', 'form_email_resolve_recipient', 10, 4);
function form_email_resolve_recipient($recipient, $email, $form, $fields) {
  // ...
  if ($recipient === '{custom:contact_person}' && $some_condition) {
    return get_field('general_contact_email', 'option');
  } elseif ($recipient === '{custom:contact_person_2}' && $some_condition) {
    // Ignore second email in this case
    return false;
  }

  return $recipient;
}

I suspect it is this line that is causing the issue. If $recipient is false at this point, it should actually bail, right?

fabianlindfors commented 2 years ago

Hi!

Code wise maybe I should add an explicit bail at that point but if $recipient is false, then af_resolve_merge_tags( false ) will also return false which will form an array: array( false ) which shouldn't succeed in sending.

Have you been able to verify that the code path in question is being triggered and is returning false?

te-online commented 2 years ago

I have to admit that I didn't put in any logs, but I did try to return a working email address instead of false which then received the email. When using WP Offload SES, you can see that with returning false it attempts to send an email to a blank address, when returning an actual address it sends it to that address. Maybe wp_mail by default ignores false as a destination address and WP Offload SES doesn't?

fabianlindfors commented 2 years ago

Might be something about WP Offload SES! I've added an explicit early bail if the filter returns false now: https://github.com/advancedforms/advanced-forms/commit/819fdff0d2a47ecfe32cd004c6dc046e94bdd292. Could you update the plugin with the latest version from Github and see if that helps?

te-online commented 2 years ago

Thank you so much for your quick response. Please allow me a few more days to test this, since I need to log in to the staging server of this website and replace the plugin as I don't have a local copy right now.

te-online commented 2 years ago

Hey! I just tested https://github.com/advancedforms/advanced-forms/tree/819fdff0d2a47ecfe32cd004c6dc046e94bdd292 and it fixes the issue perfectly 🎉

fabianlindfors commented 2 years ago

So glad to hear that! Thanks for helping me improve AF :)

te-online commented 2 years ago

Hey there 👋 Any plans on when the next release that could include this fix will be available? 😊