Problem/Motivation

We have been discussing render caching for token tree in past few issues. It is very convenient to use render caching but Drupal's default render caching won't work for POST requests (which is used when loading the token tree browser in a dialog window via AJAX).

Further, there might be some rework required to effectively implement render caching. In the previous issues, we changed tree_table to take in an already processed tree for rendering (such as when generated by buildRenderable). Even if we implement render caching, the tree would still have to be processed before passing it on to renderer.

However, we should determine if render cache is indeed the best place for this. I suspect the basis for this idea is that the processing was initially part of theme_token_tree. As we observed, that theme hook was doing too much which was the reason it was not easily cacheable or testable. Generating a token tree from a list of token types should not really be a part of the render stack (such as a render element), and hence it is not ideal to use render caching for that.

The issues where this was discussed:

Proposed resolution

My suggestion would be to cache the generated tree ourselves based on the options. We can still use this to pass in appropriate tags to render cache to cache the table output as well.

Remaining tasks

  • Decide on the approach for caching
  • Patch
  • Review
  • Commit

User interface changes

None

API changes

As per above suggestion, the buildRenderable method would return a '#cache' array for render caching.

Data model changes

None

Comments

hussainweb created an issue.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented

I created #2653998: Allow to opt-in for render caching on POST requests, without that, this won't do much unless we forcefully implement caching ourself but then that would make this a rather problematic API because the returned render array would vary and it would be impossible to further alter it, like you do yourself in one of the tests.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented

@berdir pinged me on IRC to provide feedback here.

which is used when loading the token tree browser in a dialog window via AJAX

Interesting, why does that use POST?

From #2645740-8: Replace tree_table with a render element and simplify other theme functions:

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

From #2647532: Remove token_tree theme hook:

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

I think it'd be better to do instead:

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

+
TokenTreeTable::getInfo() would return a #lazy_builder.

The 'all' would then be passed on to that #lazy_builder callback, which means that it's only in the lazy builder that we need to call $treeBuilder->getTokenTree('all') (which sounds expensive).

I wonder what you mean by determine if render cache is indeed the best place for this? Are you saying that you need this "token tree" for other things than just this "token tree table"? If so, then yes, it might make sense to just cache that separately, and that'd already make things a whole lot faster for the token tree table too then.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented

Because ajax is still always POST? We (still) haven't fixed #956186: Allow AJAX to use GET requests. But yes, once that is in, we can switch our ajax call to GET which should make this much easier.

The token tree data itself is already cached as well.