Problem/Motivation

The list of all available tokens is a very useful resource but it is placed in the help text where users don't expect it.
Typically, the help text - as part of the documentation - is static and something users should only need to read once to figure out how a module works.
Other modules place their changing overviews of available or used items as pages in the report section; for example the lists of fields or views plugins provided by core modules. This is where I rather would expect the list of available tokens as well.

Proposed resolution

Place the table of available tokens as a page such as admin/reports/tokens and link to if from the help page.

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#35 token-move-list-of-available-tokens-2782605-34.patch2.69 KBgaurav.kapoor
#32 token-move-list-of-available-tokens-2782605-32.patch2.43 KBgaurav.kapoor
#30 token-move-list-of-tokens-2782605-29.patch2.46 KBgaurav.kapoor
#28 token-move-list-of-available-tokens-2782065-28.patch1.75 KBgaurav.kapoor
#26 token-move-list-of-available-tokens-2782065-26.patch1.75 KBgaurav.kapoor
#24 token-move-list-of-available-token-2782605-24.patch2.47 KBgaurav.kapoor
#22 token-move-list-of-available-tokens-2782605-22.patch2.47 KBgaurav.kapoor
#20 token-move-list-of-available-token-2782605-20.patch2.47 KBgaurav.kapoor
#15 2782605-13.patch2.46 KBrajeshwari10
#9 2782605-9.patch2.48 KBrajeshwari10
#6 2782605-6.patch3.28 KBYogesh Pawar
#3 2782605-2.patch3.32 KBrajeshwari10
Membership dollars fund testing for the Drupal project. Drupal Association Learn more

Comments

ifrik created an issue. See original summary.

rajeshwari10’s picture

Assigned: Unassigned » rajeshwari10
rajeshwari10’s picture

Done changes as per suggested.

Thanks,
Rajeshwari.

Status: Needs review » Needs work

The last submitted patch, 3: 2782605-2.patch, failed testing.

The last submitted patch, 3: 2782605-2.patch, failed testing.

Yogesh Pawar’s picture

Status: Needs work » Needs review
FileSize
3.28 KB

I have rerolled the patch to remove whitespace & new line warnings.

Status: Needs review » Needs work

The last submitted patch, 6: 2782605-6.patch, failed testing.

The last submitted patch, 6: 2782605-6.patch, failed testing.

rajeshwari10’s picture

Assigned: rajeshwari10 » Unassigned
Status: Needs work » Needs review
FileSize
2.48 KB

Status: Needs review » Needs work

The last submitted patch, 9: 2782605-9.patch, failed testing.

The last submitted patch, 9: 2782605-9.patch, failed testing.

rajeshwari10’s picture

dkynast’s picture

Is it just me or is the list of available tokens here /admin/help/token (or here admin/reports/tokens when relocated) outdated.

My usecase is:

Send an e-mail when a comment has been written (via rules module):

From: {{comment.name.value}} Subject: {{comment.subject.value}}

works.

Regarding the list of tokens, there should be something like {{comment:edit-url}} but there isn't. Seems the whole comment namespace has been changed from D7 to D8 and the documentation available is outdated.

Or did I get something wrong? Thanks for a quick reply!

ifrik’s picture

@dkynast,

your comment is a support request that has nothing to do with this issue.
You should make a separate issue for this (either as a bug or a support request). Alternatively you can ask for help at a more appropriate place such as on IRC.

rajeshwari10’s picture

Status: Needs work » Needs review
FileSize
2.46 KB

Please review.

rajeshwari10’s picture

@ifrik

I have done changes as per the proposed solution.

Please review.

Status: Needs review » Needs work

The last submitted patch, 15: 2782605-13.patch, failed testing.

The last submitted patch, 15: 2782605-13.patch, failed testing.

joachim’s picture

  1. +++ b/token.module
    @@ -29,13 +30,13 @@ function token_help($route_name, RouteMatchInterface $route_match) {
    +    $output .= '<dt>' . t('The list of the currently available tokens on this site are shown ' . Link::createFromRoute(t('here.'), 'token.reports')->toString()) . '</dt>';
    

    That's not the right way to create a link in text. The 'here' needs to be within the text, so the whole sentence can be translated, and the URL added in as a t() placeholder.

  2. +++ b/token.routing.yml
    @@ -20,3 +20,10 @@ token.flush_cache:
    +    _permission: 'access content'
    

    Is this the right permission? That means pretty much any visitor to the site can see this, including anon users who typically will have this permission.

    IIRC, generating a token list is reasonably performance-heavy, so that would be a way to DDOS a site...

PS. Also, the link text should be descriptive, so 'here' shouldn't be the link. Something like 'You can also see [LINK]the list of all currently available tokens[LINK] on the site.

gaurav.kapoor’s picture

I guess this should be the correct way

joachim’s picture

Status: Needs review » Needs work
+++ b/token.module
@@ -31,13 +32,13 @@ function token_help($route_name, RouteMatchInterface $route_match) {
+    $output .= '<dt>' . t('You can also see ' . Link::createFromRoute('Tokens', 'token.reports')->toString().' ,The list of all currently available tokens on the site') . '</dt>';

Nope, that's not right either I'm afraid. That will also break translation!

You need to use a placeholder for the url, so the string passed to t() is completely hardcoded, eg

 t('This is a <href=":url">link</a>.')

See the docs for https://api.drupal.org/api/drupal/core%21includes%21bootstrap.inc/functi... for more details. (I can't find anything about this in the developer docs on d.o, which is either a big omission, or a google fail...)

gaurav.kapoor’s picture

joachim’s picture

Status: Needs review » Needs work
+++ b/token.module
@@ -31,13 +32,13 @@ function token_help($route_name, RouteMatchInterface $route_match) {
+    $output .= '<dt>' . t('You can also see :url ,The list of all currently available tokens on the site',array(':url' => Link::createFromRoute('Tokens', 'token.reports'))) . '</dt>';

Sorry, but we're now back to the problem with the first patch!

The *complete sentence* needs to be within the call to t().

To make a link, put the actual A element inside the t(), like in my example. Use a placeholder for the value of the href property.

This means also that the placeholder value should be a URL, not a link, so use the Url rather than Link class.

Finally, make sure the sentence is correctly punctuated and capitalized, and also to put a space after a parameter separating comma.

gaurav.kapoor’s picture

joachim’s picture

Status: Needs review » Needs work

Thanks for persevering! :)
That's now the right approach for the use of the url in the translated string. A few things still need cleaning up though:

  1. +++ b/token.module
    @@ -5,6 +5,7 @@
    +use Drupal\Core\Link;
    

    You don't need this import any more.

  2. +++ b/token.module
    @@ -31,13 +32,13 @@ function token_help($route_name, RouteMatchInterface $route_match) {
    +
    

    Stray space.

  3. +++ b/token.module
    @@ -31,13 +32,13 @@ function token_help($route_name, RouteMatchInterface $route_match) {
    +    $output .= '<dt>' . t('You can also see :<a href=":url">Tokens</a> ,The list of all currently available tokens on the site', array(':url' => url('token.reports'))) . '</dt>';
    

    url() doesn't exist on D8. You want the Url class, and in particular https://api.drupal.org/api/drupal/core!lib!Drupal!Core!Url.php/function/....

    Also, the text should form a complete sentence. Currently, it will read:

    > You can also see :Tokens ,The list of all currently available tokens on the site

    What's wrong with this:

    - there is a stray ':' before the word Tokens
    - Tokens should be lower case
    - there is a stray space after the word Tokens
    - there is no full stop at the end
    - the sentence doesn't make sense really. It should read "You can also see the list of all currently available tokens on the site."

gaurav.kapoor’s picture

Status: Needs review » Needs work

The last submitted patch, 26: token-move-list-of-available-tokens-2782065-26.patch, failed testing.

gaurav.kapoor’s picture

Status: Needs review » Needs work

The last submitted patch, 28: token-move-list-of-available-tokens-2782065-28.patch, failed testing.

gaurav.kapoor’s picture

I have added the required controller.Just need the Tokens info which should be displayed in the link.

Status: Needs review » Needs work

The last submitted patch, 30: token-move-list-of-tokens-2782605-29.patch, failed testing.

gaurav.kapoor’s picture

Status: Needs review » Needs work

The last submitted patch, 32: token-move-list-of-available-tokens-2782605-32.patch, failed testing.

joachim’s picture

Getting there! :)

  1. +++ b/src/Controller/TokenAvailableTokensController.php
    @@ -0,0 +1,23 @@
    +      '#type' => 'markup',
    +      '#markup' => t('List of Tokens'),
    

    The list of tokens isn't here! See patch #3, which has it.

  2. +++ b/src/Controller/TokenAvailableTokensController.php
    @@ -0,0 +1,23 @@
    \ No newline at end of file
    

    Missing newline.

  3. +++ b/token.module
    @@ -36,8 +37,7 @@ function token_help($route_name, RouteMatchInterface $route_match) {
    +    $output .= '<dt>' . t('You can also see the list of currently available <a href=":url">tokens</a> on the site.', array(':url' => Url::fromRoute('token.reports'))) . '</dt>';
    

    This looks good!
    Though this is causing a test failure, I think. The problem is that Url::fromRoute() returns an object, and the placeholder must be a string.

    There should be a method on the Url class that outputs the url as a string.

gaurav.kapoor’s picture

Status: Needs review » Needs work

The last submitted patch, 35: token-move-list-of-available-tokens-2782605-34.patch, failed testing.

joachim’s picture

Looking good!

The test failures the latest patch is getting are all because the tests are expecting the help page to have the tokens list:

> fail: [Other] Line 40 of modules/contrib/token/src/Tests/Tree/HelpPageTest.php:
"The list of the currently available tokens on this site are shown below." found

In other words, it looks like the test needs to be updated too. The testing of the token list needs to be moved out of HelpPageTest, and into a new test case for the reports page.

gaurav.kapoor’s picture

Thanks @joachim. But i am not able to understand what should be the exact logic to be used in new test. We have to modify the previous tests or create a new file with code for test case of report page??

joachim’s picture

The existing, failing test is called HelpPageTest. That makes it sound like it is testing just the help page. So adding a test for the new reports page here would be wrong, as the name of the test class would no longer be accurate.

So I would say:

- remove the parts of HelpPageTest that test the tokens list
- add a new test (ReportsPageTest?)
- adapt the removed parts and put them into the new test

Berdir’s picture

  1. +++ b/src/Controller/TokenAvailableTokensController.php
    @@ -0,0 +1,27 @@
    +  /**
    +   * {@inheritdoc}
    +   */
    

    This is not inheriting from anything, so it needs documentation.

  2. +++ b/src/Controller/TokenAvailableTokensController.php
    @@ -0,0 +1,27 @@
    +    $token_tree = \Drupal::service('token.tree_builder')->buildAllRenderable([
    

    this should be injected as a service and assigned to $build['token_tree'] or so.

  3. +++ b/src/Controller/TokenAvailableTokensController.php
    @@ -0,0 +1,27 @@
    +    $output .= t('The list of the currently available tokens on this site are shown below');
    +    $output .= \Drupal::service('renderer')->render($token_tree);
    +    return array(
    +      '#markup' => $output,
    +    );
    

    Then you can make the text as $build['info']['#markup'] or anything like that and drop the render call.

  4. +++ b/token.routing.yml
    @@ -20,3 +20,10 @@ token.flush_cache:
    +
    +token.reports:
    +  path: '/admin/reports/tokens'
    +  defaults:
    +    _controller: '\Drupal\token\Controller\TokenAvailableTokensController::content'
    

    this also a needs a menu link so it shows up on /admin/reports.

I have no problem with just changing the help test, we can easily rename it to TokenHelpAndReportsTest or something.