Tokens enclosed in [ ] are not recognized

I think you failed to attach the patch.

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

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

+1

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

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.

+++ 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.

This looks fine to me.

+++ 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.

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.

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

Marked #1037528: How to be able to use square brackets in output around a token? as a duplicate of this issue.

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

Market #1062914: email subject token replace failing as a duplicate.

Someone needs to review this first I guess :-)

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

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

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.

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.

Sorry; ignore previous patch.

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

+++ 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.

Trying again...

And with the fixes...

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

Lemme try that again...

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

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.

Simpler test.

+++ 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.

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.

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

Filed: #1076564: Testbot is not updating test status of patches in issue queue.

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.

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

So what patch should we commit?

Yay! Testbot started working again!

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

Adding token tag...

Backports...

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.

(adding XRef)
This issue blocks #1062914: email subject token replace failing.

Powered by Dreditor (triage sandbox) and Triage transitions

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)).