pkp / pkp-lib

The library used by PKP's applications OJS, OMP and OPS, open source software for scholarly publishing.
https://pkp.sfu.ca
GNU General Public License v3.0
297 stars 443 forks source link

Mailables can fail if email template missing subject in one language #8026

Closed NateWr closed 1 year ago

NateWr commented 2 years ago

Describe the bug When a mailable gets the email subject from an email template, it's possible the subject will be empty in the requested language. In such cases, the mailable will fail with a fatal error:

Exception: Subject isn't specified in PKP\mail\mailables\EditorialReminder in /home/nate/Projects/pkp/ojs-main/lib/pkp/classes/mail/Mailable.inc.php:263

To Reproduce Reproducing this is a little complicated, but an example is in the editorial reminder task.

  1. Delete any rows in the scheduled_tasks table that include the EditorialReminder class.
  2. Delete the subject for the editorial reminder email template in a secondary language, like fr_CA.
  3. Login as an editor, go to their profile, and change their working languages so they only work in that secondary language.
  4. Run php tools/runScheduledTasks.php and then process the jobs queue.
  5. You should see the error above when it tries to send an email in fr_CA.

The following should work, too, if you edit the ANNOUNCEMENT email template to be missing the subject. But I ran into odd problems. Anyway, this should give a hint about how to handle it:

$mailable = new Mailable();
$mailable->recipients([$request->getUser()]);
$mailable->sender($request->getUser());
$mailable->subject(Repo::emailTemplate()->getByKey($request->getContext()->getId(), 'ANNOUNCEMENT'));
Mail::send($mailable);

What application are you using? main (3.4 pre-release)

Additional information @Vitaliy-1 I've assigned this to you if you have a chance to look at it. Even though it's a configuration problem (they are missing information in their configurable templates), I think this is going to cause a lot of problems.

I don't know if it's better for us to force an email template to have a subject in every language, or use a fallback subject when an email gets through to sending without a subject... Technically, it should be possible to send an email without a subject. But in practice we probably don't want to do it.

Vitaliy-1 commented 1 year ago

I'm unable to reproduce the problem with EditorialReminder. It uses getLocalizedData() method to retrieve subject from mailable, not getData('key', $locale), meaning that it retrieves subject from available locale. This indeed causes problems:

You are completely right regarding Announcement email. Another thing to consider is when the email body is also missing. Basically, Laravel approach for the email subject is to substitute it with a default one if not set. But that's a different situation.

NateWr commented 1 year ago

email subject and body can be in different locales

This is not great, but it's probably better than an empty subject or a generic fallback subject like "an email from ".

email template variables might be compiled in a different locale than subject/body itself.

Will this only happen when a template's subject or body is missing in a certain locale? If so, I think that's ok... again not great but better than a catastrophic break.

Vitaliy-1 commented 1 year ago

PRs pkp-lib: https://github.com/pkp/pkp-lib/pull/8295 OJS: https://github.com/pkp/ojs/pull/3555 OMP: https://github.com/pkp/omp/pull/1211

NateWr commented 1 year ago

@Vitaliy-1 the code looks good. Please go ahead and merge once the tests have passed.