Simplenews uses a token enclosed in square brackes for newsletter email title: [[simplenews:category-name]] [node:title]
The first token is not recognized properly. The token_scan() extracts [simplenews:category-name as token.

The attached patch can solve this by a small change in the regular expression in token_scan().

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Damien Tournoud’s picture

I think you failed to attach the patch.

Sutharsan’s picture

FileSize
697 bytes

hmm. Better now.

aspilicious’s picture

Status: Active » Needs review
Dave Reid’s picture

Status: Needs review » Needs work

Why doesn't simplenews use tokens with only one square bracket around it?

Sutharsan’s picture

Status: Needs work » Needs review

It is not the token which uses double brackets, it is the surrounding text. Simplenews uses (by default) square brackets in the email subject: [Drupal newsletter] Drupal 7 released. To achieve this tokens are used: [[simplenews-category:name]] [node:title]

Dave Reid’s picture

Status: Needs review » Needs work

Ah thanks. This will still need tests though however, so back to needs work.

Sutharsan’s picture

What testing is required for this issue? Ideally we should start with a definition of what characters are allowed in a token. I have not been able to find one. Based on a token definition I can write some unit tests.

Based on D7 core tokens and a quick scan of D6 contib the minimum is: [a-z\-]

ohnobinki’s picture

+1

Sutharsan’s picture

Assigned: Unassigned » Sutharsan

Assign to self for work at Core developer summit at drupalcon CPH.

Letharion’s picture

Assigned: Sutharsan » Unassigned
FileSize
673 bytes

I've added the opening bracket to the second part of the token aswell.

Sutharsan’s picture

Status: Needs work » Needs review
FileSize
2.94 KB

letharion and I discussed the approach with Gábor Hojtsy and made the attached patch. It tests if a token is correctly recognized when the token is enclosed in a piece of text. We use a pre-defined set of strings to wrap the token in. These sets focus mostly on special characters as used in the token itself '[', ']' and ':' because this is where we expect the problems. So we test test if tokens are recognized in strings like:
* 'this is the [site:name] site'
* '[[site:name]]' (the original use case)
* ':[:[site:name]--]'
* '[site:[site:name]:name]'

The patch contains the #10 patch and a added system test.

In the discussion with Gabor we decided not to go down the road of defining a syntax of the token. We leave that for a future occasion.

sfyn’s picture

Status: Needs review » Reviewed & tested by the community

I reviewed and installed the patch, checked the results with an action : here is the output:


Here are the results of your tokens test:
* 'this is the patches.aegir.local site'
* '[patches.aegir.local]' (the original use case)
* ':[:patches.aegir.local--]'
* '[site:patches.aegir.local:name]'

I note that line 42 in the patch:

variable_set('site_name', '<strong>Drupal<strong>');

Includes markup. So we are also testing in this test that token properly filters markup?

Finally line 65:

$expected = $test['prefix'] . variable_get('site_name', 'Drupal') . $test['suffix'];

Is it really necessary to do a database request to construct this string? You could just as easily use the static string 'Drupal' instead of the variable_get.

Neither of these issues are problems for me.

Sutharsan’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
2.87 KB

The <strong>Drupal<strong> was left over from copying the test code on an other token test. It should be removed here. The use of variable_get() came in via the same route and can be replace for a small test performance gain.
At the same time I cleared out a small remainder of testing in the assertion output string and knocked out a duplicate test case. All minors. New patch attached for the test bot.

Sutharsan’s picture

Status: Needs review » Reviewed & tested by the community

sfyn, thanks for your review. Changing status back to RTBC now the test has been passed.

sun’s picture

Status: Reviewed & tested by the community » Needs work
+++ modules/system/system.test	25 Aug 2010 21:42:46 -0000
@@ -1566,6 +1566,47 @@ class TokenReplaceTestCase extends Drupa
+  function testSystemTokenRecognition() { ¶

Trailing white-space here.

+++ modules/system/system.test	25 Aug 2010 21:42:46 -0000
@@ -1566,6 +1566,47 @@ class TokenReplaceTestCase extends Drupa
+      array('prefix' => 'this is the ',   'suffix' => ' site'),
+      array('prefix' => 'this is the',   'suffix' => 'site'),

Can we remove the needless white-space before 'suffix' in all of those?

+++ modules/system/system.test	25 Aug 2010 21:42:46 -0000
@@ -1566,6 +1566,47 @@ class TokenReplaceTestCase extends Drupa
+      $this->assertFalse(strcmp($output, $expected), t('Token recognized in string %string', array('%string' => $input)));

strcmp() along with an assertFalse() is most probably the most confusing combination that could have been chosen here. ;) Can we use strpos() !== FALSE or something?

Powered by Dreditor.

Sutharsan’s picture

Status: Needs work » Needs review
FileSize
2.85 KB

Revised patch attached.

miro_dietiker’s picture

Status: Needs review » Reviewed & tested by the community

This looks fine to me.

Dries’s picture

Status: Reviewed & tested by the community » Needs work
+++ modules/system/system.test	29 Sep 2010 22:03:51 -0000
@@ -1568,6 +1568,47 @@ class TokenReplaceTestCase extends Drupa
+    // Set a site variables for the token work with.

This is not proper English.

+++ modules/system/system.test	29 Sep 2010 22:03:51 -0000
@@ -1568,6 +1568,47 @@ class TokenReplaceTestCase extends Drupa
+    // Run the test

Can be removed.

Powered by Dreditor.

Sutharsan’s picture

Status: Needs work » Needs review
FileSize
2.76 KB

Tekst remarks processed. I also found a piece of unused code, a remainder of copy/paste.

clemens.tolboom’s picture

FileSize
3.99 KB

I added documentation to the preg_match_all by adding the /x option (http://php.net/manual/en/reference.pcre.pattern.modifiers.php)

I was puzzled by [node:created:since] used in testTokenReplacement so added that as an extra example in the token.inc @file doc too.

clemens.tolboom’s picture

I also renamed the textual [$type:$token] into [$type:$name] which is more in line with the @file naming.

Dave Reid’s picture

tsvenson’s picture

Subscribing, would be great if this would make it to 7.1.

miro_dietiker’s picture

clemens.tolboom’s picture

Someone needs to review this first I guess :-)

pillarsdotnet’s picture

Been working on my site since #16. Code looks good. Is that sufficient review?

clemens.tolboom’s picture

Someone need to apply #20 and set the status on 'reviews and tested'

pillarsdotnet’s picture

Status: Needs review » Reviewed & tested by the community
sun’s picture

Status: Reviewed & tested by the community » Needs work
diff -ur ./includes/token.inc ../../drupal-7.x-dev/includes/token.inc
--- ./includes/token.inc	2010-10-18 03:13:07.000000000 +0200
+++ ../../drupal-7.x-dev/includes/token.inc	2010-12-23 19:05:00.000000000 +0100

Odd patch file target; I wonder how the testbot was able to apply this patch.

+++ ../../drupal-7.x-dev/includes/token.inc	2010-12-23 19:05:00.000000000 +0100
@@ -104,9 +104,16 @@
+  // Matches tokens with the following pattern: [$type:$name]
+  // $type and $name may not contain any [ ] or white space.
...
+    ([^\s\[\]:]*)  # match $type not containing whitespace : [ or ]
+    :              # : - separator
+    ([^\s\[\]]*)   # match $name not containing whitespace [ or ] ¶
...
   $types = $matches[1];
   $tokens = $matches[2];

"$name" is confusing here - the resulting variable is $tokens, which also clarifies that it potentially contains multiple "names" separated by colons.

Also note the trailing white-space.

+++ ../../drupal-7.x-dev/modules/system/system.test	2010-12-23 18:09:24.000000000 +0100
@@ -1634,6 +1634,42 @@
   /**
+   * Test to see if tokens are recognized when contained in a string.
+   *
+   * We check if valid tokens are recognized as part of a text environment.
+   * We add various strings as prefix and suffix and assert if the token is
+   * replaced correctly.
+   */
+  function testSystemTokenRecognition() {

Do we need an entirely new test for this? IMHO, this tests the fundamental token scan operation (and I really hope we have dedicated test for that).

While the amount of code comments is good, most of the comments are merely repeating the code in human language, which isn't really useful. Code comments should mention the non-obvious, the actual problem space that made us add the assertions and what the expected behavior is.

Powered by Dreditor.

pillarsdotnet’s picture

Status: Needs work » Needs review
FileSize
3.15 KB

Improved patch based on comments in #29.

Does not address the objection in #15 but that is against original code not changed by this patch.

pillarsdotnet’s picture

FileSize
3.88 KB

Sorry; ignore previous patch.

pillarsdotnet’s picture

FileSize
1.69 KB

And just for completeness, patching the tests only should fail.

sun’s picture

Status: Needs review » Needs work
+++ includes/token.inc
@@ -15,9 +15,9 @@
- * Tokens follow the form: [$type:$name], where $type is a general class of
+ * Tokens follow the form: [$type:$token], where $type is a general class of

@@ -34,9 +34,9 @@
- * Some tokens may be chained in the form of [$type:$pointer:$name], where $type
+ * Some tokens may be chained in the form of [$type:$pointer:$token], where $type
...
- * $name is the name of a given placeholder. For example, [node:author:mail]. In
+ * $token is the name of a given placeholder. For example, [node:author:mail]. In

Let's revert these. This high-level description already makes sure to explain that $name might be prefixed with additional $pointers aso.; i.e., replacing $name with $token would be wrong here.

+++ includes/token.inc
@@ -104,8 +104,15 @@ function token_replace($text, array $data = array(), array $options = array()) {
+  // $type and $token may not contain any "[" "]" or white space characters.
+  // $type may not contain ":" characters, but $token may.

Let's remove the double-quotes around characters, and also the "any" in the first line.

+++ includes/token.inc
@@ -104,8 +104,15 @@ function token_replace($text, array $data = array(), array $options = array()) {
+    ([^\s\[\]]*)   # match $name not containing whitespace [ or ]

Stale $name

+++ modules/system/system.test
@@ -1577,9 +1577,29 @@ class TokenReplaceTestCase extends DrupalWebTestCase {
     $target .= '[bogus:token]';
+
+    // Begin mailing-list tests: see #733192: Tokens enclosed in [ ] are not recognized.
...
+    // End mailing-list tests.
+
     $result = token_replace($source, array('node' => $node), array('language' => $language));

1) It looks like $target belongs to the assertions following afterwards. Would probably be better to append the new assertions elsewhere (at the end?) and keep the usage of site:name instead of re-using the the node-related tokens.

2) We do not link to d.o issues in code comment, unless it is impossible to explain something in a short code comment (but in which case the code comment should still contain a meaningful summary). Additionally, I've no clue what "mailing-list tests" has to do with this issue/patch, so that needs to be replaced entirely with a valid summary of what is being asserted/verified here and what is expected.

3) No "End of ..." comments. They're usually an obvious sign that something went wrong.

Powered by Dreditor.

pillarsdotnet’s picture

Status: Needs work » Needs review
FileSize
1.66 KB

Trying again...

pillarsdotnet’s picture

FileSize
3.13 KB

And with the fixes...

Sutharsan’s picture

FileSize
3.7 KB

testTokenReplacement() is not the right place for this test and assertion of different kind of tests should not be combined in one. This patch breaks out the test into a new testSystemTokenRecognition() test function.

pillarsdotnet’s picture

FileSize
1.66 KB

Test should fail without corresponding change to includes/token.inc.

pillarsdotnet’s picture

FileSize
2.21 KB

Lemme try that again...

pillarsdotnet’s picture

Status: Needs review » Needs work

Soo... if the tests pass without the corresponding code change, does that mean that the code change is not necessary?

pillarsdotnet’s picture

Category: bug » task

Okay, I tested by reverting includes/token.inc to the git:7.x version, creating a test newsletter via Simplenews (the originally-reported test case), and sending it to the test address (myself). The generated email came through with the proper email subject line.

So the originally reported problem no longer exists.

We should still probably commit the patch in #38, to guard against future regressions.

EDIT: Double-checking revealed that I had replaced the original token-based subject with a hard-coded string. After restoring the original configuration, I was able to duplicate the problem.

Sutharsan’s picture

The original case still fails without the patch (newsletter title is "[[simplenews-category:name]] bla") and works correctly with the patch ("[drupal.skye newsletter] bla"). Need to dig into this, but not time now.

pillarsdotnet’s picture

FileSize
762 bytes

Simpler test.

pillarsdotnet’s picture

Status: Needs work » Needs review
tstoeckler’s picture

Category: task » bug
+++ modules/system/system.test
@@ -1596,6 +1596,17 @@ class TokenReplaceTestCase extends DrupalWebTestCase {
+    $this->assertFalse(strcmp($target, $result), 'Token recognized inside [ ] chars.');

This would be easier written as:
$this->assertEqual($target, $result, 'Token recognized inside [ ] chars.');
(Minor: chars should be characters, or better, simply 'brackets')

Powered by Dreditor.

pillarsdotnet’s picture

The logic was copied verbatim from the existing testcase for the token_replace() function. If the copy should be changed, then so should the original. Please express that change as a separate patch in a separate issue.

I'd like to see the test actually fail before we bikeshed how to express equivalent logic statements and related warning strings.

EDIT: I have opened up #1076394: Improved test code to address the suggested change in assertion code.

Sutharsan’s picture

Lets call it a "self correcting mechanism" which causes the test like #37 and #38 to return a false positive. The tested string contains potential failing strings (e.g. '[[site:name]]') and at least one correct string '[site:name]'. token_scan() returns both, but token_generate() will only return a replacement pair for the valid tokens. The collection of all results now contains at least one token/value pair for '[site:name]' this is used in str_replace() which will replace any matching token. Even the ones which were not detected by token_scan(). But reality is not always as forgiving as this test case.

You can falsify this theory by using the code below. The first test fails, the second passes.

  /**
   * Test whether token-replacement works inside [ ] characters.
   */
  function testSystemTokenRecognition() {
    global $language;
    $source = '[[site:name]] blah.';
    $target = '[Drupal] blah.';
    $result = token_replace($source, array(), array('language' => $language));
    $this->assertFalse(strcmp($target, $result), 'Token recognized inside [ ] chars.');

    $source = '[[site:name]] blah. [site:name]';
    $target = '[Drupal] blah. Drupal';
    $result = token_replace($source, array(), array('language' => $language));
    $this->assertFalse(strcmp($target, $result), 'Token recognized inside [ ] chars.');
  }
Sutharsan’s picture

Status: Needs review » Needs work
Sutharsan’s picture

Status: Needs work » Needs review
FileSize
3.04 KB

This patch brings back the individual assertions on the individual tokens. In my sanbox the patch fails without the change in includes/token.inc.

pillarsdotnet’s picture

Meaning no offense to your personal sandbox, I'd also like to see what the testbot says.

pillarsdotnet’s picture

pillarsdotnet’s picture

Status: Needs review » Reviewed & tested by the community

Okay, tests-only patch in #49 fails (good).

Tests-plus-fix patch in #48 succeeds. (good)

Code looks good; all reasonable objections have been addressed.

Unless there are further objections I'd call this RTBC.

clemens.tolboom’s picture

Status: Reviewed & tested by the community » Needs review

I don't see why #51 concludes both patches in #48 and #49 are ok.

So what patch should we commit?

Sutharsan’s picture

Status: Needs review » Reviewed & tested by the community

#48 is the RTBC patch.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 733192-48-tests-only-should-fail.patch, failed testing.

pillarsdotnet’s picture

Yay! Testbot started working again!

pillarsdotnet’s picture

FileSize
3.02 KB

Re-rolled with the --no-prefix option; I'm curious as to whether the testbot will accept or reject it.

pillarsdotnet’s picture

Status: Needs work » Needs review
Sutharsan’s picture

Status: Needs review » Reviewed & tested by the community

Passed the testbot. RTBC by humans as before.

dawehner’s picture

Version: 7.x-dev » 8.x-dev
Issue tags: +Needs backport to D7
Dave Reid’s picture

Issue tags: +token

Adding token tag...

pillarsdotnet’s picture

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Both patches looks good. Committed to 7.x and 8.x so we can mark this fixed.

Someone should probably notify the Simplenews maintainers. I couldn't find the Simplenews issue -- it was not linked to from this issue.

clemens.tolboom’s picture

Simon Georges’s picture

Issue tags: -Needs backport to D7

Thanks a lot ! (from one of Simplenews maintainers, the others have been notified too).
For reference: #1062914: email subject token replace failing.
(and it doesn't need backport to D7 any more, as it is committed to 7.x (at least I think)).

Sutharsan’s picture

Confirmed. The patch has been committed to both 7.x and 8.x branches.

Status: Fixed » Closed (fixed)
Issue tags: -token

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