Remove token_tree theme hook

1. +++ b/src/TreeBuilderInterface.php
   @@ -44,4 +44,27 @@ interface TreeBuilderInterface {
   +   * @param string|array $token_types
   +   *   An array containing token types that should be shown in the tree, or
   +   *   'all' if all token types should be shown.
   

   Not a fan of mixed argument types like that. It prevents us from using proper type hinting, for example. My only idea so far is a separate method, buildRenderableAllTypes or something, that contains the all logic.

2. +++ b/token.module
   @@ -22,17 +22,14 @@ use Drupal\Core\Url;
   -    /** @var \Drupal\Core\Render\RendererInterface $renderer */
   -    $renderer = \Drupal::service('renderer');
   -    $output .= '<dd>' . $renderer->render($token_tree) . '</dd>';
   +    $output .= '<dd>' . \Drupal::service('renderer')->render($token_tree) . '</dd>';
   

   This seems like an unrelated change

3. +++ b/token.module
   @@ -69,7 +55,7 @@ function token_theme() {
   -    ] + $info['token_tree']['variables'],
   +    ],
   

   do we have all the options here covered from above?

4. +++ b/token.pages.inc
   +++ b/token.pages.inc
   @@ -23,13 +23,19 @@ function template_preprocess_token_tree_link(&$variables) {
   

   This is now the last function in this file, which is great. Lets do a small follow-up and move it into .module

I'm not sure we fully understand each other about the render caching yet and I still think we need to move #token_types to be processed in the element type. But lets continue that discussion in the next issue/step.

1. The problem is that we can't change the API later, we have to design it now in a way that works. This is our one and only chance to do it :) Empty array doesn't work, that means "only global types". Separate method should be easy enough? You just explicitly load and forward all types to the other method?
2. Hm. It doesn't really matter that much now that I already reviewed it, so lets keep it. Stuff like this makes reviewing a patch slightly harder because I had to figure out what/if something changed :)

+++ b/src/Controller/TokenTreeController.php
@@ -44,7 +44,12 @@ class TokenTreeController extends ControllerBase {
     $token_types = !empty($options['token_types']) ? $options['token_types'] : 'all';
...
-    $build = $this->treeBuilder->buildRenderable($token_types, $options);
+    if ($token_types == 'all') {
+      $build = $this->treeBuilder->buildAllRenderable($options);
+    }
+    else {
+      $build = $this->treeBuilder->buildRenderable($token_types, $options);
+    }

ha, so we still have the option here :)

Since we're moving around this code anyway, maybe clean this up a bit? You have two checks now, first you conditionally set $token_types and then you have an if/else on that. I think you can just use $options['token_types'] in the if check?

+++ b/src/TreeBuilder.php
@@ -111,6 +108,16 @@ class TreeBuilder implements TreeBuilderInterface {
+    $options['global_types'] = FALSE;
+    return $this->buildRenderable($token_types, $options);

Can you add a comment why we disable global types (because we already explicitly add them, but that might not be obvious from the code)

Other than that, looks good to me. While not perfect, I think the separate method like this is cleaner. Agreed?

(sorry for the multiple posts), this was a crosspost with your previous patch and I only noticed just now. You're too fast :)

Comment is perfect :)

Sorry found something else :)

+++ b/src/Tests/Tree/TreeTest.php
@@ -42,7 +42,7 @@ class TreeTest extends TokenTestBase {
   public function testAllTokens() {
-    $this->drupalGet($this->getTokenTreeUrl(['token_types' => 'all']));
+    $this->drupalGet($this->getTokenTreeUrl());
 

hm. now that I see this, I don't think we can change this. As I said above, this means that it's no longer possible to have the token tree with *only* the global tokens. If this isn't failing any tests, we should add test coverage for that, should be easy enough now :) possibly just an assertNoText() for a token that shouldn't be there.

So, you can't do empty() check, at least here, we need to keep the all special case.

+++ b/src/TreeBuilder.php
@@ -51,6 +51,77 @@ class TreeBuilder implements TreeBuilderInterface {
+      /*'#cache' => array(

Are this and the next bit part of the @TODO item?

Yes, the caching it like that is fine for now, makes sense to keep it. Looks good now to me, yay for more test coverage.

Would be nice to have some sort of change record about that.

Good point. I think we can do a single change record on all the related changes here:

1) That #dialog = TRUE ( which we already removed in previous issues) now must use the separate theme template token_tree_link and is in fact the recommended way of embedding a token tree.
2) That this theme function was removed in favor of the buildRenderable()
3) And that tree_table was converted to a #type.

There are still 1-2 changes outstanding until we're done here (caching and what that will require) but in 90+% of the cases, modules should use token_tree_link anyway and we should highlight that.

@hussainweb++

I added an example to the change record. @hussainweb, please verify that it makes sense: #2648394: Theme functions have been replaced with template preprocess, render elements, or service methods.

I simplified the example a bit, there's no need to add all the optional attributes and also used an explicit token type. Published.