admin_toolbar_tools module makes all pages uncacheable

Hum, strange indeed. Seems major to me.

Just tested on a fresh new install 8.3.5 and admin toolbar 1.19. Seems that the core toolbar make it uncachable, not admin toolbar.

You confirm that it was cachable with toolbar enabled ?

Indeed ! I close this issue then.

NB: the core issue can be solved by a patch in https://www.drupal.org/node/2899392.
When I apply that patch the page becomes cacheable.

But when I then enable admin_toolbar_tools the page becomes uncacheable again :-(

Well, then it is certainly the CSRF token for the cache flush. Hard to see how to solve the issue.

CSRF tokens should be placeholderable, but this is not yet fixed in core. However most 'unsafe' links can be turned off with permissions. A content editor role does not need to flush the cache.

Ah I found a token is also set for links to drupal.org. In admin_toolbar_tools.services.yml:
- admin_toolbar_tools.drupalorg
- admin_toolbar_tools.listchanges
- admin_toolbar_tools.doc
Are those necessary?

In fact why are these routed through a controller instead of direct links?

Here is a patch that removes _csrf_token: 'TRUE' for links in the admin toolbar, created by admin_toolbar_tools, that are safe without a token.

Admin toolbars will be cacheable by Dynamic Page Cache if these links are present. For the remaining links (cache flushing and cron) that require tokens a separate patch is needed that provides a lazybuilder for these menu items.

@finne thaks for the patch I think that we can create another issue the handle your patch (#2915778: Remove the _csrf_token from routes that don't need it), as the other can take more time.

@adriancid thanks. I'm working on a lazybuilder for the cache and cron items, so the whole toolbar tools is cacheable again :-)

@finne thanks, reviewing your patch I see the route:

admin_development:
  path: '/admin/development'
  defaults:
    _controller: '\Drupal\admin_toolbar_tools\Controller\ToolbarController::development'
    _title: 'Development'
  requirements:
    _permission: 'administer site configuration'

That is only a redirect to:

  /**
   * Displays the administration link Development.
   */
  public function development() {
    return new RedirectResponse('/admin/structure/menu/');
  }

Do you have idea why we need this? Maybe @eme can help us?

Re: #13
I have no idea, same goes for the controller calls that redirect to drupal.org. I think these links could be normal admin_toolbar_tools.links.menu.yml, and it would clean out the controller quite a bit.

@finne let see if @eme can say something about this and I can change the behavior if needed.

I do agree for the admin/development link. Couldn't remember where it comes from. It must be quite old. We should delete this routing & controller.

Thanks @eme I'' remove it in a few hours, and what about the redirects to drupal.org pages?

@eme: And what about the external links to drupal.org?
- ha oops double posts :-)

Oh, yes of course. That's way better.

Solved the problem with the route admin_development in #2916040: Remove unused route admin_development

Here is a patch that makes all pages cacheable again. The solution is to use a lazy builder for the menu items that have tokens in them.

Unfortunately core toolbar uses a complicated mechanism to determine cacheability of the toolbar menu, so you cannot just add/remove menu items, because the cachability is already saved at an earlier stage with the use of internal toolbar module functions :-( I worked around this by building a menu render array for the required menu items and merging them into the menu tree after it's build() function has run.

Please test & review.

@finne I will check later your patch.

the drupal.org links issue is #2916064: Use in drupal.org links url parameter and not route_name

New patch that fixes the failing test (by checking an html string that is present even without the lazybuilder additions).

@finne thanks for the patch, the only problem that I see is that the menu items don't have the same weight.

@finne and @eme I need that you test the patch to see if we can commit this soon.

Changes are OK. I checked the code and tested the patch on a clean 8.5.x install.
For reference I added the interdiff between 24 and 28.
I think this can be committed.

@finne thanks for the work.

And thanks to @all for made this possible ;-)

Hi @finne, can you take a look at #2923466: Inconsistent dropdown behaviour with admin menu please?

Patch has been reverted as it causes many errors. I'll re-organize this issue and list the problems raised by it. For now, it is unfortunately very erratic except the fact that in vertical mode the dropdown is broken.

I ran into the same problem. While I wanted to fix it, I figured the problem is due to CSRF tokens in the links. But they were already correctly placeholdered. What was wrong was toolbar module, see #2949457: Enhance Toolbar's subtree caching so that menu links with CSRF token do not need one subtree cache item per session. -> The patch over there fixes the problem.

So what you say is that the patch here + #2949457: Enhance Toolbar's subtree caching so that menu links with CSRF token do not need one subtree cache item per session works well ?

Issue is that when we applied the patch in this post, the result was very erratic...

If the Toolbar is fixed in Drupal core at #2949457: Enhance Toolbar's subtree caching so that menu links with CSRF token do not need one subtree cache item per session, this issue is solved without needing a patch.

Issue #2949457: Enhance Toolbar's subtree caching so that menu links with CSRF token do not need one subtree cache item per session is fixed!