I was trying to write my own dynamic tokens just like the [date:custom:?] token and noticed that the tokens can't have spaces in them. In the token_scan() function it does say that both the type and token can't have spaces in it and the regexp clearly indicates that too.
I guess I'm a bit confused on how it is supposed to operate. It seems like the [date:custom:?] token takes arguments or parameters but really that is not the case at all. It just means the actual name of the token after "custom" can be dynamic.
If you try and use this token [date:custom:Y m] it will not work because there is a space in it. If you use [date:custom:Y-m] it works fine.
I'm not sure what the ramifications are in having tokens that can have spaces but I've attached a patch here that does allow spaces in tokens.
If tokens should not have spaces in them is there a better way to have a token like interface that allows parameters? I suppose one could make a custom filter but it just seems redundant to me.
Comment | File | Size | Author |
---|---|---|---|
#45 | 1035292-token-space-45-test-only.patch | 1.48 KB | vijaycs85 |
#45 | 1035292-token-space-45.patch | 2.4 KB | vijaycs85 |
#42 | 1035292-token-space-42.patch | 2.4 KB | vijaycs85 |
#42 | 1035292-diff-39-42.txt | 440 bytes | vijaycs85 |
#39 | 1035292-token-space-39.patch | 2.39 KB | vijaycs85 |
Comments
Comment #1
Dave ReidHrm, that's interesting since it should work. This will need some tests to back it up though.
Comment #2
Dave ReidComment #3
wernercd CreditAttribution: wernercd commentedI'd love to test this on D7...
Comment #4
Dave ReidWe needs some simple test functions to assert that token_scan() works for tokens like [one: two three ] match but not [one two:three]. Would also be good to test a real token like [date:custom:...]. I think this seems like a good novice task as well to help get involved with core and writing tests. Ping me on IRC if you need any help with this.
Comment #5
Désiré CreditAttribution: Désiré commentedHere is some tests, and actualized bug fix for D7 and D8.
(test only patch should fail)
Comment #6
Désiré CreditAttribution: Désiré commentedgrammar corrections
Comment #7
xjmThe patches in #6 look good to me. Thanks for your work on this.
Note that the D8 version of this patch will need to be rerolled, because the core directory structure for Drupal 8 has now changed. (For more information, see #22336: Move all core Drupal files under a /core folder to improve usability and upgrades).
If you need help rerolling this patch, you can come to core office hours or ask in #drupal-gitsupport on IRC.
Comment #8
xjmOh, just a minor note. These should begin with verbs in the third-person present indicative. (It's true that won't be consistent with the rest of the file, but we should use the correct tense in the docblocks we're adding. The others are being cleaned up as part of #1310084: [meta] API documentation cleanup sprint.)
For more information, see:
http://drupal.org/node/1354#functions
29 days to next Drupal core point release.
Comment #9
Désiré CreditAttribution: Désiré commentedComment #10
casey CreditAttribution: casey commented#9: 1035292-dynamic_tokens_cant_have_spaces-9.patch queued for re-testing.
Comment #11
wizonesolutionsHere's a reroll against head. This is affecting Fill PDF (via Token itself...) and I would love to see this get in.
Comment #13
wizonesolutions#11: 1035292-dynamic_tokens_cant_have_spaces-9-11.patch queued for re-testing.
Comment #15
wizonesolutionsDrupalWebTestCase
has been renamed toWebTestBase
. Fixed; new patch.Also, removed unintentional bad reference from patch name.
Comment #16
xjmThanks @wizonesolutions. The current patch looks pretty good.
Let's do some manual testing to confirm the functionality. Please document the exact steps you used for testing it manually here in the issue.
Also, a couple of super-minor code style points we can clean up in the meanwhile:
There should be blank lines between the opening and closing of the class, and its members. Reference: http://drupal.org/node/1354#classes
We could make these assertion messages a little less terse (use articles and make them complete English sentences). Also, let's remove t() from these assertion messages. Reference: http://drupal.org/simpletest-tutorial-drupal7#t
When posting a new patch, please again upload the test-only patch followed by the combined patch. Thanks!
Comment #17
bomoko CreditAttribution: bomoko commented#15: 1035292-dynamic_tokens_cant_have_spaces-15.patch queued for re-testing.
Comment #19
star-szrTagging for reroll.
Comment #20
oriol_e9gComment #21
oriol_e9gOps! Wrong file :S
Comment #22
jthorson CreditAttribution: jthorson commented#21: token-spaces-1035292-21.patch queued for re-testing.
Comment #23
jthorson CreditAttribution: jthorson commentedAdjusting tags
Comment #24
Michsk CreditAttribution: Michsk commentedThis patch fixed it for me in D7.
Comment #25
Summit CreditAttribution: Summit commentedFor me too. Is it ok to set it to RTBC?
Greetings, Martijn
Comment #26
jthorson CreditAttribution: jthorson commentedMartijn,
The preferred process is to test and verify the D8 version of the patch, ensure it gets committed to the Drupal 8 codebase, and then set it back to 'needs review' for the Drupal 7 version of the patch. This approach ensures that any items we fix in Drupal 7 don't re-appear as bugs fin the Drupal 8 code.
If it weren't for this minor 'process' hurdle, it would definately be okay ... anyone can set the RTBC status, given that they've verified that i) the patch solves the issue in question, and ii) it complies with the Drupal coding standards.
Thanks for helping verify the patch on D7 ... this will help speed up the process once the D8 version of the patch is finally verified and marked RTBC.
Comment #27
brephraim CreditAttribution: brephraim commentedWhich patch works for D7 now?
Comment #28
achtonHere's a reroll for D8 based on #21 and xjm's comments in #16.
I am unsure how to perform a manual test of this, but my attempt involved creating a tiny module that called token_replace() like so:
Without the patch, the token fails to parse. With the patch, it displays "Test string: 2012 10".
Because I'm not sure if this is what signifies a "manual test", I am not RTBC'ing it.
Comment #29
drupalEx1234 CreditAttribution: drupalEx1234 commentedI can't get the D7 version to work. Can anybody help?
Comment #30
jthorson CreditAttribution: jthorson commenteddrupalEx1234,
Please do not change the version of the issue ... as per my explanation in #26, that will actually slow down any progress on the D7 version of the patch, as it needs to be applied to D8 before the patch will be backported.
This doesn't preclude from asking if the D7 patch is still valid ... but leave the issue version at 8.x until it's committed to D8; and it will be bumped down to 7.x for the backport after that.
Comment #31
jthorson CreditAttribution: jthorson commentedMinor nitpick ... should be:
... as per the 'documenting files' section of the doxygen and comment formatting conventions page.
Otherwise, I think the description in #28 should suffice as a manual test, and after the docblock change, this should be ready to go.
Comment #32
drupalEx1234 CreditAttribution: drupalEx1234 commentedAny news on the development of this. I still can't get it to work on D7.
Comment #33
jthorson CreditAttribution: jthorson commentedSomeone needs to re-roll the patch from #28 to make sure it applies against the current 8.x branch of core, and incorporate the docblock change from #31 ... at which point it will likely be marked RTBC for 8.x. Once committed, the issue number gets switched to 7.x, and the patch is re-rolled again.
Since the re-roll and comment change are fairly trivial tasks, and are blocking the resolution you need for Drupal 7.x, I'd definitely encourage you to take a stab at it!
Comment #34
ACF CreditAttribution: ACF commentedRe rolled with small change to doc block.
Comment #35
star-szrThanks for the reroll, @ACF!
I have one minor suggestion, otherwise this looks good to me.
I think the inline comment should use "token-like" as well, it's less ambiguous.
Comment #36
ACF CreditAttribution: ACF commentedAdded the change to the comment.
Comment #37
Fabianx CreditAttribution: Fabianx commentedTested, reviewed, works for me. => RTBC (also as per #33)
Comment #38
webchickCommitted and pushed to 8.x. Thanks! Moving back to D7 for backport.
Comment #39
vijaycs85patch to D7...
Comment #41
star-szrThanks @vijaycs85! The test class for Drupal 7 will need to extend DrupalWebTestCase, see http://drupal.org/node/1543796.
Comment #42
vijaycs85Updating patch...
Comment #43
realityloopThe patch in #36 didn't have a tests only patch to show, so if no functionality has changed since the testsonly patch in #25 I think it might be ok that #42 doesn't include a tests only patch?
Comment #44
star-szr@realityloop - Well, that test only patch was against 8.x, so I think that's a good idea.
@vijaycs85 - Could you please post a new comment with a test-only patch as well as the complete patch from #42 that you already posted? See #28 for an example and http://drupal.org/node/1468170 for a bit more information. Thanks!
Comment #45
vijaycs85Here you go...
Comment #46
oriol_e9gComment #47
David_Rothstein CreditAttribution: David_Rothstein commentedCommitted to 7.x - thanks! http://drupalcode.org/project/drupal.git/commit/fc104c9