In the 7.x branch some of the devel menu items (Clear cache, Rebuild menus, Reinstall modules and Run cron) have the querystring 'destination' parameter, which allows a quick back navigation.
Would be very useful re-introduce the destination parameter in the Devel menu items also in the 8.x branch

CommentFileSizeAuthor
#10 re_introduce_the-2579907-10.patch7.31 KBwillzyx
#6 re_introduce_the-2579907-6.patch6.23 KBwillzyx
#2 re_introduce_the-2579907-2.patch6.11 KBwillzyx

Comments

willzyx created an issue. See original summary.

willzyx’s picture

willzyx commented
StatusFileSize
new re_introduce_the-2579907-2.patch6.11 KB

The attached patch contains the following changes:

- Re-introduce the destination parameter in the Devel menu items
- Rename DevelCsrfLinksTest in DevelMenuLinksTest so we can group the menu related tests in this class
- Add tests for the destination parameter in the Devel menu items
- Remove the unexistent hooks devel_menu_link_alter() devel_translated_menu_link_alter() and devel_menu_need_destination() since currently this is dead code

moshe weitzman’s picture

moshe weitzman commented

Are we giving up on this block being cacheable? I guess thats reasonable. Otherwise looks good.

willzyx’s picture

willzyx commented

Are we giving up on this block being cacheable?

mmm no.. currently the block IS cacheable, it is cached and the destination parameter is added correcly and varied depending on the context (try the patch).
The "dirty" work of manage the cache context for the menu is done by the core. If we want to be more explicit (and probably we sholud) we should implement ::getCacheContexts() in the DestinationMenuLink class

moshe weitzman’s picture

moshe weitzman commented

Cacheable with how much granularity? If an item varies by path, it can be not worth caching, especially as this block is for admins only.

willzyx’s picture

willzyx commented
StatusFileSize
new re_introduce_the-2579907-6.patch6.23 KB

We're mixing a little the things; this issue is not about devel menu block cacheability nor want change the caching logic of this block.
It simply add the destination parameter in the Devel menu items.
If we want to discuss if/how to change the cache logic of the menu block I'm ok but I thinnk that is out of scope for this issue

anyway.. (correct me if I'm wrong, I'm still learning the d8 cache system)
Currently we use the default caching logic implemented by core for the menu blocks. This is the cache context used by default for menu block:
- languages:language_interface
- user.permissions
- route.menu_active_trails:menu_name
- theme

This is the cache context used currently for devel menu:
- user.permissions
- route (with attached patch but it also used by a bunch of other blocks eg local tasks)

willzyx’s picture

willzyx commented

@moshe if you have no objection, I'm going to commit this patch in a couple of days

moshe weitzman’s picture

moshe weitzman commented
Assigned: Unassigned » moshe weitzman

I'd like to test this before it goes in. Assigning to self. I've got a similar need at #2582797: [Regression] login link has no destination=drupalSettings.path, so dumps you on the profile and I'm pretty sure that we can't append the destination via PHP there because we deal with a menu link that appears on every page.

moshe weitzman’s picture

moshe weitzman commented
Status: Needs review » Postponed

Lets reopen once that issue gets in. We will reuse its javascript to append the destination querystring param.

willzyx’s picture

willzyx commented
Status: Postponed » Needs review
StatusFileSize
new re_introduce_the-2579907-10.patch7.31 KB

@moshe
I modified the patch by removing the route cache context and adding cache max-age = 0. In this way the menu links with destination (and the block that contains them) become uncachable (and this should solve the doubts about caching that you express in #5). The performance issues related to this change are minimal especially considering that the devel menu block is only for admins

IMHO we can commit this patch and make the menu block uncachable and when #2582797: [Regression] login link has no destination=drupalSettings.path, so dumps you on the profile lands we can easily switch to the js-based solution changing only a couple of lines of code

Let me know if is possible to commit this patch or you want to wait #2582797: [Regression] login link has no destination=drupalSettings.path, so dumps you on the profile

moshe weitzman’s picture

moshe weitzman commented
Status: Needs review » Reviewed & tested by the community

This fix works for devel.

  • willzyx committed 693b243 on 8.x-1.x 
    Issue #2579907 by willzyx: Re-introduce the destination parameter in the...
willzyx’s picture

willzyx commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. Thanks

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.