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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Dave Reid’s picture

Status: Active » Needs review
Issue tags: +token

Hrm, that's interesting since it should work. This will need some tests to back it up though.

Dave Reid’s picture

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

I'd love to test this on D7...

Dave Reid’s picture

Issue tags: +Needs tests, +Novice

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

Désiré’s picture

Here is some tests, and actualized bug fix for D7 and D8.
(test only patch should fail)

Désiré’s picture

xjm’s picture

Status: Needs review » Needs work

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

xjm’s picture

+++ b/modules/system/system.testundefined
@@ -1781,6 +1781,36 @@ class QueueTestCase extends DrupalWebTestCase {
+ * Test token scanning in strings.
...
+   * Scan dummy text, then tests the output.

Oh, 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.

Désiré’s picture

casey’s picture

wizonesolutions’s picture

Here's a reroll against head. This is affecting Fill PDF (via Token itself...) and I would love to see this get in.

Status: Needs review » Needs work
Issue tags: -Needs tests, -Novice, -token, -Needs backport to D7

The last submitted patch, 1035292-dynamic_tokens_cant_have_spaces-9-11.patch, failed testing.

wizonesolutions’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work
Issue tags: +Needs tests, +Novice, +token, +Needs backport to D7

The last submitted patch, 1035292-dynamic_tokens_cant_have_spaces-9-11.patch, failed testing.

wizonesolutions’s picture

Status: Needs work » Needs review
FileSize
2.49 KB

DrupalWebTestCase has been renamed to WebTestBase. Fixed; new patch.

Also, removed unintentional bad reference from patch name.

xjm’s picture

Thanks @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:

  1. +++ b/core/modules/system/system.testundefined
    @@ -1935,6 +1935,36 @@ class SystemThemeFunctionalTest extends WebTestBase {
    +class TokenScanTestCase extends WebTestBase {
    +  public static function getInfo() {
    ...
    +  }
    +}
    

    There should be blank lines between the opening and closing of the class, and its members. Reference: http://drupal.org/node/1354#classes

  2. +++ b/core/modules/system/system.testundefined
    @@ -1935,6 +1935,36 @@ class SystemThemeFunctionalTest extends WebTestBase {
    +    $this->assertTrue(isset($token_wannabes['valid']['simple']), t('Simple valid token matched.'));
    +    $this->assertTrue(isset($token_wannabes['valid']['token with: spaces']), t('Valid token with space characters in the token name matched.'));
    +    $this->assertFalse(isset($token_wannabes['not valid']), t('Not valid token with spaces in the token type not matched.'));
    +    $this->assertTrue(isset($token_wannabes['node']), t('Existing valid token matched.'));
    

    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!

bomoko’s picture

Status: Needs review » Needs work
Issue tags: +Novice, +token, +Needs manual testing, +Needs backport to D7

The last submitted patch, 1035292-dynamic_tokens_cant_have_spaces-15.patch, failed testing.

star-szr’s picture

Issue tags: +Needs reroll

Tagging for reroll.

oriol_e9g’s picture

Status: Needs work » Needs review
FileSize
2.86 KB
1.73 KB
oriol_e9g’s picture

Ops! Wrong file :S

jthorson’s picture

#21: token-spaces-1035292-21.patch queued for re-testing.

jthorson’s picture

Issue tags: -Needs reroll

Adjusting tags

Michsk’s picture

This patch fixed it for me in D7.

Summit’s picture

For me too. Is it ok to set it to RTBC?
Greetings, Martijn

jthorson’s picture

Martijn,

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.

brephraim’s picture

Which patch works for D7 now?

achton’s picture

Here'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:


function token_test_menu() {
  $items['token-test'] = array(
    'title' => 'Token test',
    'page callback' => '_token_test_admin_view',
    'access callback' => TRUE,
  );
  return $items;
}

function _token_test_admin_view() {
  return token_replace('Test string: [date:custom:Y m]') . PHP_EOL;
}

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.

drupalEx1234’s picture

Version: 8.x-dev » 7.17

I can't get the D7 version to work. Can anybody help?

jthorson’s picture

Version: 7.17 » 8.x-dev

drupalEx1234,

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.

jthorson’s picture

Status: Needs review » Needs work
+ * Definition of Drupal\system\Tests\System\TokenScanTest.

Minor nitpick ... should be:

Contains Drupal\system\Tests\System\TokenScanTest.'

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

drupalEx1234’s picture

Any news on the development of this. I still can't get it to work on D7.

jthorson’s picture

Someone 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!

ACF’s picture

Status: Needs work » Needs review
Issue tags: -Needs manual testing
FileSize
2.62 KB

Re rolled with small change to doc block.

star-szr’s picture

Thanks for the reroll, @ACF!

I have one minor suggestion, otherwise this looks good to me.

+++ b/core/modules/system/lib/Drupal/system/Tests/System/TokenScanTest.phpundefined
@@ -0,0 +1,42 @@
+      'description' => 'Scan token-like patterns in a dummy text to check token scanning.',
...
+    // Define text with valid and not valid, fake and existing token like
+    // strings.

I think the inline comment should use "token-like" as well, it's less ambiguous.

ACF’s picture

Added the change to the comment.

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

Tested, reviewed, works for me. => RTBC (also as per #33)

webchick’s picture

Version: 8.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Committed and pushed to 8.x. Thanks! Moving back to D7 for backport.

vijaycs85’s picture

Status: Patch (to be ported) » Needs review
FileSize
2.39 KB

patch to D7...

Status: Needs review » Needs work

The last submitted patch, 1035292-token-space-39.patch, failed testing.

star-szr’s picture

Thanks @vijaycs85! The test class for Drupal 7 will need to extend DrupalWebTestCase, see http://drupal.org/node/1543796.

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
440 bytes
2.4 KB

Updating patch...

realityloop’s picture

The 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?

star-szr’s picture

@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!

vijaycs85’s picture

oriol_e9g’s picture

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

Status: Reviewed & tested by the community » Fixed
Issue tags: +7.22 release notes

Status: Fixed » Closed (fixed)
Issue tags: -Novice, -token, -Needs backport to D7, -7.22 release notes

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