Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 UTC on 18 March 2024, to get $100 off your ticket.
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().
Comment | File | Size | Author |
---|---|---|---|
#62 | token_in_brackets-733192-62-d7.patch | 3.44 KB | pillarsdotnet |
#62 | token_in_brackets-733192-62-d8.patch | 3.44 KB | pillarsdotnet |
#56 | 733192-56.patch | 3.02 KB | pillarsdotnet |
#49 | 733192-48-tests-only-should-fail.patch | 1.56 KB | pillarsdotnet |
#48 | 733192-48.patch | 3.04 KB | Sutharsan |
Comments
Comment #1
Damien Tournoud CreditAttribution: Damien Tournoud commentedI think you failed to attach the patch.
Comment #2
Sutharsan CreditAttribution: Sutharsan commentedhmm. Better now.
Comment #3
aspilicious CreditAttribution: aspilicious commentedComment #4
Dave ReidWhy doesn't simplenews use tokens with only one square bracket around it?
Comment #5
Sutharsan CreditAttribution: Sutharsan commentedIt 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]
Comment #6
Dave ReidAh thanks. This will still need tests though however, so back to needs work.
Comment #7
Sutharsan CreditAttribution: Sutharsan commentedWhat 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\-]
Comment #8
ohnobinki CreditAttribution: ohnobinki commented+1
Comment #9
Sutharsan CreditAttribution: Sutharsan commentedAssign to self for work at Core developer summit at drupalcon CPH.
Comment #10
Letharion CreditAttribution: Letharion commentedI've added the opening bracket to the second part of the token aswell.
Comment #11
Sutharsan CreditAttribution: Sutharsan commentedletharion 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.
Comment #12
sfyn CreditAttribution: sfyn commentedI 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.
Comment #13
Sutharsan CreditAttribution: Sutharsan commentedThe
<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.
Comment #14
Sutharsan CreditAttribution: Sutharsan commentedsfyn, thanks for your review. Changing status back to RTBC now the test has been passed.
Comment #15
sunTrailing white-space here.
Can we remove the needless white-space before 'suffix' in all of those?
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.
Comment #16
Sutharsan CreditAttribution: Sutharsan commentedRevised patch attached.
Comment #17
miro_dietikerThis looks fine to me.
Comment #18
Dries CreditAttribution: Dries commentedThis is not proper English.
Can be removed.
Powered by Dreditor.
Comment #19
Sutharsan CreditAttribution: Sutharsan commentedTekst remarks processed. I also found a piece of unused code, a remainder of copy/paste.
Comment #20
clemens.tolboomI 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.
Comment #21
clemens.tolboomI also renamed the textual [$type:$token] into [$type:$name] which is more in line with the @file naming.
Comment #22
Dave ReidMarked #1037528: How to be able to use square brackets in output around a token? as a duplicate of this issue.
Comment #23
tsvenson CreditAttribution: tsvenson commentedSubscribing, would be great if this would make it to 7.1.
Comment #24
miro_dietikerMarket #1062914: email subject token replace failing as a duplicate.
Comment #25
clemens.tolboomSomeone needs to review this first I guess :-)
Comment #26
pillarsdotnet CreditAttribution: pillarsdotnet commentedBeen working on my site since #16. Code looks good. Is that sufficient review?
Comment #27
clemens.tolboomSomeone need to apply #20 and set the status on 'reviews and tested'
Comment #28
pillarsdotnet CreditAttribution: pillarsdotnet commentedComment #29
sunOdd patch file target; I wonder how the testbot was able to apply this patch.
"$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.
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.
Comment #30
pillarsdotnet CreditAttribution: pillarsdotnet commentedImproved patch based on comments in #29.
Does not address the objection in #15 but that is against original code not changed by this patch.
Comment #31
pillarsdotnet CreditAttribution: pillarsdotnet commentedSorry; ignore previous patch.
Comment #32
pillarsdotnet CreditAttribution: pillarsdotnet commentedAnd just for completeness, patching the tests only should fail.
Comment #33
sunLet'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.
Let's remove the double-quotes around characters, and also the "any" in the first line.
Stale $name
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.
Comment #34
pillarsdotnet CreditAttribution: pillarsdotnet commentedTrying again...
Comment #35
pillarsdotnet CreditAttribution: pillarsdotnet commentedAnd with the fixes...
Comment #36
Sutharsan CreditAttribution: Sutharsan commentedtestTokenReplacement()
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 newtestSystemTokenRecognition()
test function.Comment #37
pillarsdotnet CreditAttribution: pillarsdotnet commentedTest should fail without corresponding change to includes/token.inc.
Comment #38
pillarsdotnet CreditAttribution: pillarsdotnet commentedLemme try that again...
Comment #39
pillarsdotnet CreditAttribution: pillarsdotnet commentedSoo... if the tests pass without the corresponding code change, does that mean that the code change is not necessary?
Comment #40
pillarsdotnet CreditAttribution: pillarsdotnet commentedOkay, 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.
Comment #41
Sutharsan CreditAttribution: Sutharsan commentedThe 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.
Comment #42
pillarsdotnet CreditAttribution: pillarsdotnet commentedSimpler test.
Comment #43
pillarsdotnet CreditAttribution: pillarsdotnet commentedComment #44
tstoecklerThis 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.
Comment #45
pillarsdotnet CreditAttribution: pillarsdotnet commentedThe 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.
Comment #46
Sutharsan CreditAttribution: Sutharsan commentedLets 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 instr_replace()
which will replace any matching token. Even the ones which were not detected bytoken_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.
Comment #47
Sutharsan CreditAttribution: Sutharsan commentedComment #48
Sutharsan CreditAttribution: Sutharsan commentedThis patch brings back the individual assertions on the individual tokens. In my sanbox the patch fails without the change in includes/token.inc.
Comment #49
pillarsdotnet CreditAttribution: pillarsdotnet commentedMeaning no offense to your personal sandbox, I'd also like to see what the testbot says.
Comment #50
pillarsdotnet CreditAttribution: pillarsdotnet commentedFiled: #1076564: Testbot is not updating test status of patches in issue queue.
Comment #51
pillarsdotnet CreditAttribution: pillarsdotnet commentedOkay, 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.
Comment #52
clemens.tolboomI don't see why #51 concludes both patches in #48 and #49 are ok.
So what patch should we commit?
Comment #53
Sutharsan CreditAttribution: Sutharsan commented#48 is the RTBC patch.
Comment #55
pillarsdotnet CreditAttribution: pillarsdotnet commentedYay! Testbot started working again!
Comment #56
pillarsdotnet CreditAttribution: pillarsdotnet commentedRe-rolled with the --no-prefix option; I'm curious as to whether the testbot will accept or reject it.
Comment #57
pillarsdotnet CreditAttribution: pillarsdotnet commentedComment #58
Sutharsan CreditAttribution: Sutharsan commentedPassed the testbot. RTBC by humans as before.
Comment #59
dawehnerComment #60
Dave ReidAdding token tag...
Comment #62
pillarsdotnet CreditAttribution: pillarsdotnet commentedBackports...
Comment #63
Dries CreditAttribution: Dries commentedBoth 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.
Comment #64
clemens.tolboom(adding XRef)
This issue blocks #1062914: email subject token replace failing.
Powered by Dreditor (triage sandbox) and Triage transitions
Comment #65
Simon Georges CreditAttribution: Simon Georges commentedThanks 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)).
Comment #66
Sutharsan CreditAttribution: Sutharsan commentedConfirmed. The patch has been committed to both 7.x and 8.x branches.