Skip to content
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

Same email address when sending notification mails from the forms. This is not supported by many mail providers. #350

Open
creatiombe opened this issue Feb 18, 2023 · 7 comments

Comments

@creatiombe
Copy link

Q A
Bug? yes
New Feature? no
Bundle Version 2.5.1
Sulu Version Specific version or SHA of a commit
Browser Version Browser name and version

Actual Behavior

How does it behave at the moment?

It uses the same SULU_ADMIN_EMAIL for notifications and success mails.

Expected Behavior

What is the behavior you expect?

May mailing services block same domain mails, so notification system is broken if the main organisation mail is the same as the SULU_ADMIN_MAIL. For example i only want a [email protected] for environment variable for SULU_ADMIN_EMAIL. But if this is the same as the main organisation mail adres, there is no possibility to change this.

Steps to Reproduce

What are the steps to reproduce this bug? Please add code examples,
screenshots or links to GitHub repositories that reproduce the problem.

Possible Solutions

If you have already ideas how to solve the issue, add them here.
Like i thought and got suggested by chatgpt, i need to override the form handler of the form bundle.
A possible solution would be to have in the sulu_form configuration a notification_from email address override that handles same email addresses. Or if the from and to is the same in the form handler, override the same email adress with a noreply to avoid problems.

@alexander-schranz
Copy link
Member

You can already define whatever email address you want in your config/packages/sulu_form.yaml file: https://github.com/symfony/recipes-contrib/blob/4deb156de103e876c8742d7db0aa73f9d8541004/sulu/form-bundle/2.4/config/packages/sulu_form.yaml#L3-L6

The SULU_ADMIN_MAIL is just the default one used as from, to, sender email address. Even in the Form itself can a mail address be defined.

I'm not sure what the issue really is. What email providers are blocking here what correctly (which headers in which combination)? Can you provide more information how we can reproduce here, so we maybe can provide here better default settings.

@creatiombe
Copy link
Author

creatiombe commented Feb 18, 2023

I want to keep SULU_ADMIN_MAIL for user administration and such, which is [email protected]. Also i use the main organisation email address which is also [email protected]. But because i use a dynamic form with all fields left empty sender email/name
receiver email/name. It default to both the same email adresses for the notification mail. Mailgun, which i use as provider does not accept the same email with from and to. Is there another way to just make the notification come from a different email address then that i am not aware off?

@creatiombe
Copy link
Author

If i change the sender email it also keeps the same mail as the main organisation is the same email address. Sadly there is no option to just use for the dynamic form another notification FROM email address so i can use [email protected] instead.

@alexander-schranz
Copy link
Member

alexander-schranz commented Feb 20, 2023

If I understand you correctly this works in your case:

sulu_form:
    mail:
        from: "[email protected]"
        to: "[email protected]"
        sender: "[email protected]"

But this fails:

sulu_form:
    mail:
        from: "[email protected]"
        to: "[email protected]"
        sender: "[email protected]"

Think then the easiest fix would be check in the FormBundle to set the sender only if sender !== from. Do you want to create a pull request for it?

Pinging here also @lkeijmel as he implemented the Sender feature, maybe he has to add something here.

@lkeijmel
Copy link
Contributor

lkeijmel commented Feb 20, 2023

I need to take a deep dive into Sulu again as it's also a couple of years ago I've used it. At first glance the suggestion of @alexander-schranz (only add sender if it's different from from) makes sense. However the sender option isn't mandatory in the config so you could do without it and if it's not set, it isn't used.

@creatiombe can you remove the sender option from your config and try it again?

@creatiombe
Copy link
Author

Hey in a follow up, yes i did fix it with overriding the SuluMailHelper with another service. I reconfigured the sendMail function the following code:

public function sendMail(
        $subject,
        $body,
        $toMail = null,
        $fromMail = null,
        bool $html = true,
        $replyTo = null,
        array $attachments = [],
        $ccMail = [],
        $bccMail = [],
        $plainText = null
    ): int {
        $message = new Email();
        $fromMail = $fromMail ?: $this->fromMail;
        $toMail = $toMail ?: $this->toMail;

        if ($toMail instanceof \array && 1 === count($toMail)) {
            $toMail = $toMail[0];
        }
        if ($fromMail instanceof \array && 1 === count($fromMail)) {
            $fromMail = $fromMail[0];
        }

        // Check if from and to are the same
        if (
            !($toMail instanceof \array) &&
            !($fromMail instanceof \array) &&
            $toMail === $fromMail
        ) {
            // Replace the email address with a noreply address
            $fromMail = preg_replace('/^(.*?)@/', 'noreply@', $fromMail);
        }

        $this->setHeaders(
            $message,
            $subject ?: '',
            $this->parseToAddresses($fromMail),
            $this->parseToAddresses($toMail),
            $this->parseToAddresses($replyTo),
            $this->parseToAddresses($ccMail),
            $this->parseToAddresses($bccMail),
            $this->sender ? new Address($this->sender) : null
        );
        $this->setBody($message, $html, $body, $plainText);
        $this->setAttachments($message, $attachments);

        $this->logMessage(
            $fromMail ?: $this->fromMail,
            $toMail ?: $this->toMail,
            $replyTo,
            $subject ?: '',
            $ccMail,
            $bccMail,
            $plainText
        );

        $this->mailer->send($message);

        return 0;
    }

Also i found out there was a setting wrong with mailgun mailer from symfony i needed to set ?region=eu. But still then it was mailing from the same email address. I just tested your settings and indeed it seems to work. But i would advice to use a check just in case same email adresses are set. I will try to make time next week to make a pull request for it.

@lkeijmel
Copy link
Contributor

Good to hear that!

When adding the small documentation change I didn't think of it to make it more clear how and why the sender property is used and that it's optional.

I will take a look at it and make a PR for it as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants