Problem/Motivation

I noticed that empty token placeholders are not being removed from the email subject and body text. Since it can be be difficult to predict when optional field values are set on an entity, it seems like removing the empty tokens is a good default.

Steps to reproduce

  1. Create a workbench email with a token in the subject and message that is known to NOT exist.
  2. The sent email will not have the [***] token placeholders removed.

Proposed resolution

Pass the ['clear' => TRUE] option to the $this->token->replace method.

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#8 3202910-8.patch6.09 KBpcate

Issue fork workbench_email-3202910

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

PCate created an issue. See original summary.

PCate opened merge request !4

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Issue tags: +Needs tests

Code changes look great, we've got some existing tests for tokens, are you able to add in a case with an obvious broken token and assert it is removed?

See \Drupal\Tests\workbench_email\Functional\WorkbenchEmailTestBase::testEndToEnd

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Issue tags: +Novice

PCate closed merge request !3

PCate closed merge request !4

PCate opened merge request !5

pcate’s picture

pcate commented
Status: Active » Needs review
StatusFileSize
new 3202910-8.patch6.09 KB

I updated the \Drupal\Tests\workbench_email\Functional\WorkbenchEmailTestBase::testEndToEnd by added to the subject and body a non-existent token. The tests fail without the ['clear' => TRUE] change.

I had trouble finding out how to get drupal ci to run on the latest merge request so I just created a regular patch file. Sorry for the GitLab comment noise.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests

Awesome will make a new release with this on Monday

  • larowlan committed 94b0256 on 2.x authored by PCate 
    Issue #3202910 by PCate: Remove empty tokens from email subject and...
larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Status: Reviewed & tested by the community » Fixed 
diff --git a/tests/src/Functional/WorkbenchEmailTestBase.php b/tests/src/Functional/WorkbenchEmailTestBase.php
index 9b014a4..7a3d7e0 100644
--- a/tests/src/Functional/WorkbenchEmailTestBase.php
+++ b/tests/src/Functional/WorkbenchEmailTestBase.php
@@ -256,8 +256,8 @@ abstract class WorkbenchEmailTestBase extends BrowserTestBase {
     $this->submitForm([
       'id' => 'approved',
       'label' => 'Content approved',
-      'body[value]' => 'Content with title [node:title] was approved. You can view it at [node:url].[node:field_does_not_exist]',
-      'subject' => 'Content approved: [node:title][node:field_does_not_exist]',
+      'body[value]' => 'Content with [node:field_does_not_exist]title [node:title] was approved. You can view it at [node:url].',
+      'subject' => 'Content [node:field_does_not_exist]approved: [node:title][node:field_does_not_exist]',
       'enabled_recipient_types[author]' => TRUE,
       'enabled_recipient_types[email]' => TRUE,
       'enabled_recipient_types[role]' => TRUE,
@@ -269,7 +269,7 @@ abstract class WorkbenchEmailTestBase extends BrowserTestBase {
     $this->submitForm([
       'id' => 'needs_review',
       'label' => 'Content needs review',
-      'body[value]' => 'Content with title [node:title] needs review. You can view it at [node:url].[node:field_does_not_exist]',
+      'body[value]' => 'Content with [node:field_does_not_exist]title [node:title] needs review. You can view it at [node:url].[node:field_does_not_exist]',
       'subject' => 'Content needs review',
       'replyTo' => '[node:author:mail]',
       'enabled_recipient_types[role]' => TRUE,
@@ -294,8 +294,8 @@ abstract class WorkbenchEmailTestBase extends BrowserTestBase {
     $assert->checkboxChecked('Approver', $page->find('css', '#edit-recipient-types-role-settings-roles--wrapper'));
     $this->submitForm([
       'label' => 'Content needs review',
-      'body[value]' => 'Content with title [node:title] needs review. You can view it at [node:url].[node:field_does_not_exist]',
-      'subject' => 'Content needs review: [node:title][node:field_does_not_exist]',
+      'body[value]' => 'Content with[node:field_does_not_exist] title [node:title] needs review. You can view it at [node:url].[node:field_does_not_exist]',
+      'subject' => 'Content needs[node:field_does_not_exist] review: [node:title][node:field_does_not_exist]',
       'replyTo' => '[node:author:mail]',
     ], t('Save'));
     $assert->pageTextContains('Saved the Content needs review Email Template');

Changed on commit to add another instance mid-line to ensure spacing is retained

This will go out as 2.1.1

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.