Replace tree_table with a render element and simplify other theme functions

Patch as per IS.

This probably needs some cleanup and tweaking, not to mention documentation, but I will appreciate a review first and views on the direction. FYI, I tested this manually as well and the token tree behaviour works fine. I found something that we could add in our tests (asserting restricted elements) but that could be a separate issue.

This should fix it.

I did think of that, but then it seemed that this render element would be doing too much. The only functionality left in token_tree and devel controller are orthogonal and something which is suitable to a controller. That way, the render element is focused on doing only one thing - rendering the token tree and the responsibility of building the render array (and the tree) goes to the controller.

The bulk of building the tree is already handled by the tree builder service. The part which now remains in theme_token_tree can be moved to the tree builder service as a helper method, IMHO (in a follow-up). With that, we can remove the theme hook entirely and replace the current places it is called with the render element and the new helper method.

This helps us keep both things separate - the render element TokenTreeTable to pre-render the tree structure generated by buildTree or the new helper method analogous to theme_token_tree.

This is what the devel controller really does:

    $token_tree = [
      $token_type => [
        'tokens' => $this->treeBuilder->buildTree($token_type, $options),
      ],
    ];

The other calls would do something like this:
Before:

    $token_tree = array(
      '#theme' => 'token_tree',
      '#token_types' => 'all',
      '#click_insert' => FALSE,
      '#show_restricted' => TRUE,
    );

After:

    $token_tree = array(
      '#type' => 'token_tree_table',
      '#token_tree' => $treeBuilder->getTokenTree('all'),
      '#click_insert' => FALSE,
      '#show_restricted' => TRUE,
    );

This way, we keep two distinct pieces of functionality separate and clean.

As for the rest of your points, I agree. I eventually plan to remove the theme hook (like I mentioned before). I will also work on auto-generating #columns if not set. We can decide on whether to always show description as well. The new #columns property makes it very easy to add/remove columns. It will just be a one line change once we decide.

I was thinking of handling caching in a follow-up, TBH. The caching doesn't work at the moment anyway and I wanted to focus on getting to the same level of features first. Of course, it is important to plan ahead so that we can be sure of the design.

Also, I am trying to approach this one step at a time. I am not used to massive refactors and would like to split it so that it is easy to review at each checkpoint and move ahead. If you don't mind, I would rather close this and work on caching in a follow-up.

I like the idea of buildRenderable. I was thinking of something similar but the name is perfect and since we have precedent, it sounds great. That can actually replace the whole render array structures with just one call.

Lastly, I want to explore why we have to use POST at all here. We are not modifying the data anywhere in the token/tree route. I wanted to explore this direction in a follow-up as well, just to avoid confusion. What do you think?

I looked into the dialog and AJAX system and POST is hardcoded with no one to override it as far as I can see. I don't think it is worth writing the AJAX requests ourselves...

About buildRenderable(), I was thinking it would return a render array with a '#token_tree' and cache tags set accordingly. It would still go to the TokenTreeTable render element (that is something which is renderable, right?) I am thinking you are talking about this in combination with pre_render, but I don't see why we have to use that. Can you point me to an example where it is used for caching? I can't find any documentation in relation with caching.

Further, if we use getTokenTree(), we can still use it along with buildRenderable() with cache settings specified in render array. If we have to use our own caching, we can cache at the buildTree() level (skipping the cache if $options['values'] is TRUE). I think if we have to cache, it should be close to the logic where we do bulk of the processing (buildTree). If we are using render cache, buildRenderable is a better choice. In both cases, we can pass back the bubbleable metadata to the render array. We just discard the metadata right now. This could help in render caching.

I think I am still missing something with pre_render and lazy_builder and so I can't really recommend one way or another. I do wish that render cache dealt with it. Considering that the only time it won't be cached will be in the dialog, which is only accessible to admin users, and not all that often used, I feel it is better to use render cache. If and when core starts supporting GET requests for dialog, we will have performance boost even for dialogs.

Thanks for committing and your reviews.

I think I now finally see what you meant all along. I see your concerns and quite agree and for a moment I thought that would make it harder to cache. But I thought on this a bit and then realized that this way makes sense. What we are caching here is render logic, which is quite separate from the token tree we are trying to build in buildTree. Earlier, the two were in the same place and that made things complex but we can now split them as we should and cache separately.

I am still working out how all of this play together and will follow your advice and work on the next step. Thanks for all your help and explanations. I really appreciate it.

Created #2648608: Implement (render) caching for token tree to discuss caching.