Problem/Motivation

Contextual links break the render cache: they are only added if the user has the "access contextual links" permission and can be different for each user.

Proposed resolution

Client-side solution

catch has indicated a long time ago, and in another issue, that the solution here is to indeed do something analogous to what Edit module is doing: only annotate the "thing" (Entity/View/Block/…) with some sort of identifier (probably in a data-contextual-id attribute, that a piece of JS can look for, submit to the server to know if the user can access the contextual links for each thing, and what those actions are.
The JS would then only be added if the current user has the access contextual links permission.

Consequences

For users that don't have permission to use contextual links, the overhead is limited to just <div data-contextual-id="<id>"></div> (in the location where the contextual links would appear), and class="contextual-region". If the user has access to contextual links, the JS will be loaded and will fill in the div[data-contextual-id]s.
These (<div data-contextual-id="<id>"></div> & class="contextual-region" end up in the render cache, and are served to everybody; hence allowing for full render cache compatibility.
Contextual Links before this patch worked when JS was disabled. Well, for an odd definition of "working": the links would appear, but they would be mostly-inappropriate pieces of unordered links. Now, JS is a hard requirement.

Server-side solution

Some people are arguing that the solution shouldn't require JavaScript. It should be possible to have more granular caching (possibly even per-user — though this effectively amounts to still breaking the render cache) and/or post-processing (e.g. retrieve rendered block from render cache and post-process it to insert the contextual links for the current user, if any).

On post-processing, catch said in #120:

If we end up establishing js-replacement as the standard approach to this (which I've been trying to encourage for well over a year now), adding a post-process layer to the render cache, explicitly limited to internal caching, would then allow for a PHP implementation of the replacement for sites that don't use *SI and don't want the js dependency.

On more granular caching, sdboyer said in #134 & #141:

frankly, when it comes to just throwing stuff on the page, we need to get more comfortable with saying, "don't do it that way, there's a right way to do things." that having been said, from a SCOTCH perspective, it's at least as useful if we have an explicit way of knowing that someone is lazily injecting HTML all over the place, rather than (re)writing a new block that works they way they actually want it to.

most important, though, is that injecting HTML all over is only a problem IF some aspect of that injection is made conditionally on something from unencapsulated global state. only at that point does it become cache-busty. it's perfectly fine for things to vary, so long as they vary in a manner we can predict from the outside by inspecting the data that's injected. as long as those data represent the entire range of variation, we're more or less in the clear.

i haven't just quite gotten to the caching mechanism yet, so i unfortunately can't point you to code (though i took a break from writing the contextual integration that supports it to write this response). it's rooted in the idea that by knowing what we inject into a block, we know its caching dimensions. blocks will be able to say "i'm uncacheable," or that the granularity level is less than that which the injected data might seem to imply (e.g., they require a user object, but vary merely by role, not by uid), but no more of the grand constants like CACHE_PER_ROLE, CACHE_PER_USER.

Both

It's also possible to have both the server-side and client-side solution, because both make more sense in different scenarios.

Remaining tasks

  • Choose solution: client-side (implemented, almost RTBC'able), server-side (TBD), or both. An example of both could be the client-side approach implemented so far in this issue, plus a boolean cache split based on #1961884: Add support for 'permissions' and/or 'contexts' to render cache system. We can also choose to do the client-side portion in this issue, and the server-side portion as a follow up in that one.

User interface changes

None.

API changes

To be determined.

Original report by di3h3rd3r

The "Configure block" link of the "Active Forum Topics" and "New Forum Topics" blocks are visible to anonymous and authenticated users. Works as expected for admin user. I have tried the block in different regions with the links still showing up.

CommentFileSizeAuthor
#184 contextual-links-render-cache-914382-184-TESTS-ONLY.patch2.21 KBDavid_Rothstein
#184 contextual-links-render-cache-914382-184.patch4.45 KBDavid_Rothstein
#176 contextual_links_render_cache-914382-176.patch44.23 KBWim Leers
#176 interdiff.txt704 bytesWim Leers
#172 contextual_links_render_cache-914382-172.patch44.26 KBWim Leers
#172 interdiff.txt3.18 KBWim Leers
#170 contextual_links_render_cache-914382-170.patch45.13 KBWim Leers
#170 interdiff.txt10.21 KBWim Leers
#166 contextual_links_render_cache-914382-166.patch42.44 KBeffulgentsia
#165 contextual_links_render_cache-914382-165.patch43.52 KBWim Leers
#160 contextual_links_render_cache-914382-160.patch43.5 KBWim Leers
#158 contextual_links_render_cache-914382-155.patch42.21 KBfranz
#155 contextual_links_render_cache-914382-155.patch42.21 KBWim Leers
#155 interdiff.txt2.3 KBWim Leers
#153 interdiff.txt4.12 KBGábor Hojtsy
#152 interdiff.txt720 bytesGábor Hojtsy
#152 contextual_links_render_cache-914382-152.patch39.82 KBGábor Hojtsy
#150 contextual_links_render_cache-914382-150.patch37.61 KBGábor Hojtsy
#147 contextual_links_render_cache-914382-146.patch39.01 KBWim Leers
#147 interdiff.txt5.53 KBWim Leers
#132 contextual_links_render_cache-914382-132.patch36.57 KBWim Leers
#132 interdiff.txt1.24 KBWim Leers
#127 contextual_links_render_cache-914382-127.patch36.51 KBWim Leers
#127 interdiff.txt2.82 KBWim Leers
#126 contextual_links_render_cache-914382-126.patch34.21 KBWim Leers
#126 interdiff.txt3.25 KBWim Leers
#123 contextual_links_render_cache-914382-123.patch34.11 KBWim Leers
#121 worksforme.png298.97 KBWim Leers
#121 contextual_links_render_cache-914382-121.patch22.26 KBWim Leers
#121 interdiff-step-ONE-tests_and_feedback.txt16.74 KBWim Leers
#121 interdiff-step-TWO-views_support_metadata.txt9.76 KBWim Leers
#118 view-edit.png108.23 KBdawehner
#118 views.view_.contextual.yml_.txt2.89 KBdawehner
#113 contextual_links_render_cache-914382-113.patch18.69 KBWim Leers
#113 contextual_links_render_cache-914382-113-just_contextual.patch13.21 KBWim Leers
#103 914382-test_9763ca9-103.patch2.43 KBstar-szr
#95 914382-95.patch2.23 KBstar-szr
#95 interdiff.txt1.22 KBstar-szr
#93 914382-93.patch2.25 KBstar-szr
#93 interdiff.txt2.07 KBstar-szr
#85 914382-85.patch2.31 KBstar-szr
#85 interdiff.txt1.12 KBstar-szr
#84 914382-84-testonly.patch2.21 KBstar-szr
#84 914382-84.patch4.64 KBstar-szr
#80 914382-80-test-D7.patch2.21 KBstar-szr
#78 914382-78.patch2.22 KBstar-szr
#55 drupal8.block-contextual-links-cache.55.patch2.44 KBsun
#54 drupal8.block-contextual-links-cache.54.patch1.64 KBsun
#53 block_cache_contextual.patch692 bytescatch
#49 914382-contextual_forum_block_cache.patch2.18 KBfranz
#37 914382-contextual_block_cache.patch937 bytesfranz
#29 914382-forum-d7.patch633 bytesNiklas Fiekas
#19 1 logged in as admin.jpg254.71 KBpaul_constantine
#19 2 logged out.jpg255.68 KBpaul_constantine
#19 3 logged in a reg user.jpg245.63 KBpaul_constantine
#8 914382-forum.patch625 bytesdroplet
#4 914382-forum.patch629 bytesdroplet
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

furamag’s picture

I'm able to recreate this issue. I'm not sure where problem is. I think something wrong in hook_menu inside block.module. Currently we are using following code:

  $items['admin/structure/block/manage/%/%/configure'] = array(
    'title' => 'Configure block',
    'type' => MENU_DEFAULT_LOCAL_TASK,
    'context' => MENU_CONTEXT_INLINE,
  );

Per my understanding we should use 'access arguments' for menu item. We need something like this:

  $items['admin/structure/block/manage/%/%/configure'] = array(
    'title' => 'Configure block',
    'type' => MENU_DEFAULT_LOCAL_TASK,
    'context' => MENU_CONTEXT_INLINE,
    'access arguments' => array('administer blocks'),
  );

But I can see 'Configure block' link even if I'm using 'access arguments'. What I did wrong?

tsvenson’s picture

Version: 7.0-alpha7 » 7.x-dev

There still seems to be some problems with this. I added the "Recent forum topics" to a live site and the configure block link cam up above the topic links. It did disappear when I cleaned the cache though, but something still looks to be wrong with it.

droplet’s picture

at forum.module cached the block forever.....

  $block['content'] = drupal_render_cache_by_query($query, 'forum_block_view');
droplet’s picture

Assigned: Unassigned » droplet
Status: Active » Needs review
Issue tags: +Quick fix
FileSize
629 bytes

super simple solution for it

montesq’s picture

Status: Needs review » Reviewed & tested by the community

I confirm the patch #4 fixes the issue on my side for the blocks "Active Forum topics" and "New forum topics"

Step to reproduce the bug:
- make a new install
- active forum module
- active the blocks "Active Forum Topics" & "New Forum Topics"
- as administrator on the home page, when your cursor is over the blocks, the contextual links are displayed and you can see the link "configure block"
- log out
Current result: as anonymous user you can see on the home page "configure block"
After applying the patch, the anonymous user is no more able to see the link "configure block"

ajdeeley’s picture

It works for me too. Is this patch a permanent fix? I mean, when I update the core next time will I have to apply this patch again? Thanks

droplet’s picture

@ajdeeley,
hoping this patch will commit into Drupal 7.1, then save own time to repatch again & again.

droplet’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
625 bytes

PATCH UPDATED. CACHE_TEMPORARY, DRUPAL_CACHE_PER_ROLE are constants :)
don't know why I do that before.

montesq’s picture

I don't understand what happened when I try the previous patch!? (maybe the cache was expired when I refresh the page after applying the patch?)
The last one is the good one? I repeat the steps to reproduce:
- make a new install
- active forum module
- active the blocks "Active Forum Topics" & "New Forum Topics"
- as administrator on the home page, when your cursor is over the blocks, the contextual links are displayed and you can see the link "configure block"
- log out
Current result: as anonymous user you can see on the home page "configure block"
After applying the patch, the anonymous user is no more able to see the link "configure block"

I'm waiting for another test before marking as RTBC...

droplet’s picture

@montesq & all,
#4 patch in never caches forum block, so you always see the fresh result
#8 patch is fixed and cached block per role and it will be rebuild until next cache wipe.
(it used default caches settings, copy from API docs:
CACHE_TEMPORARY: Indicates that the item should be removed at the next general cache wipe.
)

content now are serve from forum_index, it will be very fast with 1000000 nodes & without caches, so it's hard to notice patch #4 has caches problem

kalebheitzman’s picture

Tested Patch in #8 and it works fine. Did notice that from a clean install with the Bartik theme, I had to clear my cache (before installing the patch) to get contextual links to show up for New Forum Topics and Active Forum Topics. I then replicated the issue, applied the patch, and everything worked fine.

montesq’s picture

Status: Needs review » Reviewed & tested by the community

@droplet: thanks a lot for your accurancy
As kalebheitzman confirms this patch is the good one, we can set the case as RTBC

webchick’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

Nice catch.

Is it possible to add a test for this?

montesq’s picture

How can we test that?
According to me, the content of the blocks "Active Forum Topics" & "New Forum Topics" is the same for all users except when the "contextual links" is activated. Do we need to activate "contextual links" in the Forum test?
Advice are welcome...

David_Rothstein’s picture

The simplest way would be to have the test turn on the contextual links module, yes.

You could make it more generic by instead having the test module specifically inject some content into one of the template variables before rendering it for certain users (because after all that's what contextual_preprocess() basically does for the $title_suffix variable) and then looking for that content when the block is viewed, but not really sure that's worth the effort.

droplet’s picture

hmm.. It can be not same when no forum posts at beginning..

(assume no forum topics exists)
enable forum blocks
Admin post a forum topic => reload page => Block is empty (Expected, cached empty block content after it enabled)
Login to UserA, block has new forum topic (Expected) => post a new forum topic
Logout (Ann user now), Block has new forum topic (both admin & UserA posts) (Expected)

paul_constantine’s picture

Tried patch number 8 and it fixed the problem for me as far as "unregistered users" and "administrators" are concerned.

Unfortunately the "registered user" and the "moderator" still see the "configure Block" option on the "Newest Forum posts" block.

The only role that should be able to see that is the "administrator".

So maybe that patch still needs some work (if you are using more than two roles on your system). And I tried to deactivate contextual links in the permissions for all but that did not change anything.

droplet’s picture

@paul_constantine

can you try to do same test:
1. apply patch
2. log admin
3. clean all caches
4. reload page, block has conf link
5. logout
6. anon user (no conf link)
7. login common user account (no conf link)

may we need add trigger to somewhere to clean all caches when block enable

for theory, admin is "admin + registered user role", so should be diff. if not, there is caches bug i think.

(*** always remember to clean caches after it's enable)

paul_constantine’s picture

Did what you suggested, same result.

- When looged is as admin I get the configure icon (per mouse-over). (Image 1.)
- Cleared cache and logged out.
- As anon user I get no icon and no text link. (Image 2.)
- When logged in as registered user I get a text link to configure the block. (Image 3.)

Funny that it is a link for the reg users and an icon for the admin.

droplet’s picture

anyone can reproduce the bug ??

@paul_constantine
can you test it with a fresh install ?? thanks.

paul_constantine’s picture

Sorry it's on a live server.

Fresh install would mean too much downtime.

I may just remove the block and wait for the next D7 core update.

But thank you very much for your patch ... it solved the problem with the anon users :-)

Kind regards,
Paul

montesq’s picture

@paul_constantine
I don't reproduce this issue :
With anonymous or registered user, the "configure block" item is not displayed, except if you give the permissions "Use contextual links" and "Administer blocks".
It must be the case in your case...
What happens if you click on the link as registered (non admin) user?

paul_constantine’s picture

If I click as a registered user I get the "access denied" page (as I should since only the admin is allowed to configure the blocks).

paul_constantine’s picture

Problem solved:

Unchecking the permission "use contextual links" AND clearing the cache afterwards solved the problem. Appearently the cache was keeping the link visible. Note: the configure block icon for the admin is also gone.

Update:

After changing some minor details in the text formats (integrating a video filter) the problem is back again. Even though "contextual links" are deactivated in permissions the ugly "configure block" textlinks are back for registered users.

droplet’s picture

@Paul,

Im confused with your issues. super ROOT admin account always has full-permission. Is that only happen to FORUM BLOCK?
can you disable contextual links module and test.

also, can you do a fresh installation and test again (you can install in local machine, sub dir, new domain......etc) because there only you have this problem. and post steps like #18 that can help us solving problems

paul_constantine’s picture

It turned out to be a cache problem after all. I did clear the Drupal cache but I did not clear the cache of my web browser. After a couple of hours the links disappeard again - all by themself - without me chaning anything.

So all in all it must have been a cache problem for that particular block.

Hopelfully the problem is solved now.
Thanks for your input :-)

Niklas Fiekas’s picture

Subscribe.

catch’s picture

Version: 7.x-dev » 8.x-dev
Issue tags: +Needs backport to D7

Subscribing. The patch looks correct, it will need to be applied to 8.x first then D7 (same patch should apply to both at the moment).

Niklas Fiekas’s picture

FileSize
633 bytes

Straight forward D7 Port of droplets patch.

Kisugi Ai’s picture

why you have this issues still in actual dev release?
every time if i update my drupal release i have to applay this patch.

Niklas Fiekas’s picture

A testcase is missing. After that it should be good to go.

willkaxu’s picture

Status: Needs work » Needs review

#8: 914382-forum.patch queued for re-testing.

NPC’s picture

Patch in #8 solved it for me, thank you very much!

chx’s picture

Assigned: droplet » chx

Tests incoming this week.

wranvaud’s picture

Title: Contextual links on blocks should change DRUPAL_CACHE_GLOBAL to DRUPAL_CACHE_PER_ROLE » Configure block link visible to all users.
Component: contextual.module » forum.module
Priority: Major » Normal
Issue tags: +Quick fix

As suggested above, I checked permissions but everything was ok, only admin could see contextual menus. So I rebuilt the permissions and cleared the browser cache. Seems to be working without the patch.
EDIT: no it doesn't work for very long....

catch’s picture

Title: Configure block link visible to all users. » Contextual links on blocks should change DRUPAL_CACHE_GLOBAL to DRUPAL_CACHE_PER_ROLE
Component: forum.module » contextual.module
Priority: Normal » Major
Status: Needs review » Needs work
Issue tags: -Quick fix

There's actually a more serious problem here than just forum module, see #1223324: Contextual links on blocks should change DRUPAL_CACHE_GLOBAL to DRUPAL_CACHE_PER_ROLE which I've just marked as duplicate.

#1223324-22: Contextual links on blocks should change DRUPAL_CACHE_GLOBAL to DRUPAL_CACHE_PER_ROLE contains a more comprehensive patch.

I'm not sure if that can be made to work for forum module's case - might have to still add the per role manually, but either way that will need documenting in the D6-D7 upgrade guide.

What should really happen when blocks are handling their own caching is for the contextual links to not be cached along with the block itself (i.e. a level higher in the render array), haven't looked at that though.

franz’s picture

Status: Needs work » Needs review
FileSize
937 bytes

Is block cache that critical?

I re-rolled the patch from http://drupal.org/node/1223324#comment-4777872 . The logic on it is to override cache of all blocks changing them to a PER_ROLE setting if contextual is enabled.

Marking needs review just to test it

sun’s picture

After resolving the first hook invocation order @todo, I think we could go ahead with #37. I.e., leaving the edge-case of the second node access @todo to contrib...

I hope that the context initiative takes this lack of a more granular context into account though. @catch's idea would basically translate into introducing a new hook_block_post_view(), which runs after hook_block_view(), but is not yet an alter hook, and which allows to add further stuff to the potentially-cached-in-whatever-way block... or similar.

franz’s picture

Should we change contextual weight?

catch’s picture

Title: Configure block link visible to all users. » Contextual links on blocks should change DRUPAL_CACHE_GLOBAL to DRUPAL_CACHE_PER_ROLE
Component: forum.module » contextual.module
Priority: Normal » Major
Issue tags: -Quick fix

Block caching is still disabled for {node_access} in D7 so that is not really a @todo.

Unless we're able to mess with the forum block structure (not impossible) so that it does not end up caching the contextual links, we may also need to add the additional one-off patch here - but if we do that, we need to document heavily somewhere that this is the case since it is going to break all over the place.

Kisugi Ai’s picture

Hi,

patch 914382-contextual_block_cache.patch dont resolve

only 914382-forum.patch will work for me

franz’s picture

Alyx, did you clear cache?

The straight-forward fix in here is the same I wrote in the other ticket. However, as was pointed by others, this may be a bug for every custom cache block implemented, as well as globally or per page cached blocks. I don't think we should fix only for the forum block.

droplet’s picture

Title: Contextual links on blocks should change DRUPAL_CACHE_GLOBAL to DRUPAL_CACHE_PER_ROLE » Forum Configure block link visible to all users. (block cache error)
Component: contextual.module » forum.module

not the same isseue.

forum block content cached with drupal_render_cache_by_query(), not the block caches.

#8 is the right patch, please review it, thanks

droplet’s picture

Title: Forum Configure block link visible to all users. (block cache error) » Wrong caches in forum block content (and configure block link visible to all users)

Re-title.

(keeps "configure block link visible to all users" because not everyone notice the forum block contents are cached the wrong content. make it searchable. ^_^)

catch’s picture

Status: Needs review » Needs work

It's the same root cause because contextual module is adding content indiscriminately to stuff that get's cached without taking account of it. The whole point of cache by query is that it handles caching automatically without any need for cache by role. Additionally forum module has no mention of contextual module.

droplet’s picture

I'm not understand contextual module well. If contextual has caches issue, all blocks will show the links, right ?? so you think #37 is a more right patch or we needs work on other patches or merge them?
(#37 has typo error "|=")

The whole point of cache by query is that it handles caching automatically without any need for cache by role.

No ?? disabled contextual module / disable block caches, same thing still happen to forum block CONTENTS.

catch’s picture

Forum block content ought to be cached, I didn't see any reports of issues apart from the contextual links in terms of issues with this?

If the best fix we can do for D7 is having every module that provides a block and does its own caching add cache per role just in case contextual module is enabled then yeah we should merge the two patches but it needs inline comments and documentation as an api change.

franz’s picture

I see now why #37 doesn't fix the issue. Yes, we need both patches together. However, it has to be properly documented on hook_block_info() that it is the custom cache implementer's responsibility to use CACHE_PER_ROLE or CACHE_PER_USER when caching.

@droplet, that's not a typo, it's a bitwise operation. It is advised use it on cache constants.

franz’s picture

Status: Needs work » Needs review
FileSize
2.18 KB

I've implemented module weight change, added comments.

I thought about adding a doc on hook_block_info() for the CACHE_CUSTOM, but I see it's not there at all.

Marking needs review for testing.

xjm’s picture

Tagging issues not yet using summary template.

Kisugi Ai’s picture

@franz
jeah i will try later
but i have had no other block problems

HongPong’s picture

subscribe

catch’s picture

FileSize
692 bytes

Since block module adds contextual links, we should probably try to do this there.

It ought to be possible to do that for cacheable render arrays too, although bit more complex.

sun’s picture

#53 unconditionally manipulates block caching on all blocks, regardless of whether contextual module is actually installed/enabled.

I'd be fine with just simply going ahead with attached patch and leaving the remaining nightmare to figure out for contrib.

sun’s picture

whoopsie, forgot one hunk.

franz’s picture

Status: Needs review » Needs work

@sun, this is almost what I posted on #49, except it doesn't guarantee contextual module will be run after all - core, at least - modules.

catch’s picture

@sun. In that case why is block module adding the contextual links itself at all ? I hate module_exists() but this seems like somewhere to use that rather than have this logic in two different places, either that or move everything into contextual.

Not keen on punting this to contrib, the problem was introduced by core adding arbitrary crap to blocks, so core can fix that. The idea of hundreds of contrib and custom modules checking for contextual module does not sound appealing.

AlexWebDesigner’s picture

I confirm! I solved the "configure block" link visible to anonymus user simply with a "clear Cache" ..

David_Rothstein’s picture

I looked through this issue and I don't understand the logic in the latest patches. What does all that code to force DRUPAL_CACHE_PER_ROLE in the Contextual Links module (or alternatively in the Block module) actually accomplish?

In Drupal 7, blocks that are cached via the normal mechanism (the 'cache_block' table) cannot have any caching problems related to contextual links, because the thing that gets cached is a renderable array, not the final HTML. And in a renderable array, the #contextual_links property should be completely safe to cache.

In Drupal 8, things are different, since as a result of #918808: Standardize block cache as a drupal_render() #cache rendered HTML is now getting cached in the 'cache_block' table for all blocks. However, that change only happened about a week and a half ago, so it would be very impressive if the patches from two months ago here were prescient enough to be prepared for that :)

Since block contextual links are pretty broken in Drupal 8 anyway (see #1304470: Add tests for module-provided contextual links on blocks) and the fix for that might have some ramifications for caching, I think we should not try to solve the complete, just-introduced-a-week-and-a-half-ago problem here. Let's instead just fix the forum module here, which (unless someone can prove me wrong with a specific example?) is the only fix that is actually needed for Drupal 7.

Since the issue is marked 'Needs tests', though, it might be a good idea to start with tests and to make sure that we test both kinds of scenarios. If there is code that proves my above points wrong, it would certainly be good to have that in the form of a test... and even if it proves me right, it would still be equally useful to have that as a test :)

catch’s picture

@David, well HTML is cached within the renderable array, and if that includes rendered contextual links then that explains the bug report. Agreed that tests would be a good start here (and useful for the other bug you found as well).

franz’s picture

@David, the bug was confirmed and reproduceable on D7 - see #1223324: Contextual links on blocks should change DRUPAL_CACHE_GLOBAL to DRUPAL_CACHE_PER_ROLE

It was particular to the way Forum rendered the block, AFAIR...

David_Rothstein’s picture

It was particular to the way Forum rendered the block, AFAIR...

Yes, exactly. The Forum module has this bug (and any other module that uses similar custom caching will too), but I am so far unaware of any other conditions where it can occur in D7, which, if true, means that the rest of the code in the patch isn't needed.

franz’s picture

OK, so please re-open the issue in #61, it has a specific fix for it.

catch’s picture

The problem with the forum hunk though is that it only fixes it for forum module.

I don't think it's acceptable for Drupal 7 modules doing custom caching to have to take into account the per-role cache requirement of contextual links - so really contextual should be responsible for adding more granular caching (if that's even possible which it may not be).

David_Rothstein’s picture

@franz, it looks to me like the specific fix in #1223324: Contextual links on blocks should change DRUPAL_CACHE_GLOBAL to DRUPAL_CACHE_PER_ROLE was essentially the same code that had already been posted in the early patches in this issue? (I think that's why it was marked as a duplicate.)

@catch, I assumed based on the fact that even the more recent patches in this issue included the forum.module change also, that there was no way to fix that custom caching bug generically. Looking at it now, though, perhaps there is a way to do it in hook_block_view_alter() (rather than hook_block_info_alter()), although it looks like it might involve some minor refactoring of the drupal_render() cache code. If that works, I definitely agree it's a better way to do it.

franz’s picture

David, yes, I forgot there was this parallel patching...

catch’s picture

Title: Wrong caches in forum block content (and configure block link visible to all users) » Contextual links incompatible with render cache
Component: forum.module » contextual.module

More background on #1304470: Add tests for module-provided contextual links on blocks where this was also run into. This might end up being a feature addition to drupal_render(), but moving it over to contextual module for now.

moshe weitzman’s picture

Assigned: chx » Unassigned

I had an idea this morning to change drupal_render() such that it caches the the main theme() call for an element but does not cache the calls to #theme_wrappers (and #post_render). Since block.tpl.php is a theme_wrapper, contextual_preprocess() will dynamically stuff its junk into #title_suffix and the links will print out properly. This proposal has minimal performance impact, since theme_wrappers can just prepend and append HTML. Not caching #post_render could be considered a feature improvement - core does not use it anywhere ... I wonder if this proposal is what David meant by in #65 by "minor refactoring of the drupal_render() cache code"

cache_tags would be a more robust solution, since having #contextual_links makes render caching at upper levels invalid. But I think the above idea is a near term improvement.

snoopy77’s picture

Droplet (post #4) , thank you. Your simple patch fixed the abnormal behavior on my 7.10 installation. Cheers :-)

moshe weitzman’s picture

I'm going to move over to #636454: Cache tag support and see if we can get that in. Interim solutions like my #68 are not too satisfying.

David may work on a solution for D7. I think #4 would be an OK solution for now. Perhaps we add the additional keys only if contextual module is enabled. It has been 1 year since that patch was posted, so lets fix the bleeding.

catch’s picture

I don't think cache tags specifically will help here, that covers invalidation but not cid generation/granularity.

What's needed here I think is a way for contextual module to alter the cid for blocks as well as altering the HTML. However even that is tricky because contextual links aren't per role necessarily, they're based on whatever arbitrary rules are in the access callback for menu_get_item().

Where the cache tags issue (or really the follow-up from that) could help is in passing access/granularity requirements up the stack of renderable thingies. For example if you have a block caching a block and the child one has contextual links, it doesn't help if only the nested block has a more granular cid. So this really comes down to the mess of having infinite options to vary HTML output via multiple different methods based on any arbitrary criteria with no introspection into what happened from other parts of the system.

Sms2luv’s picture

How to apply patch

Niklas Fiekas’s picture

@Sms2luv: There's this: http://drupal.org/node/620014.

star-szr’s picture

Issue summary added.

star-szr’s picture

On another note, I just tested with the steps to reproduce (up in the issue summary) and was unable to reproduce this behaviour in 8.x-dev, and contextual links worked fine.

Kisugi Ai’s picture

in 7.14 to
what ever thez have made

star-szr’s picture

From what I can tell this was fixed in 8.x via #918808: Standardize block cache as a drupal_render() #cache. If someone else can confirm, we can at least push this back to 7.x.

Edit: Of course the issue summary would need to be updated as well if that's the case.

star-szr’s picture

Assigned: Unassigned » star-szr
Status: Needs work » Needs review
FileSize
2.22 KB

Here's a test case for 8.x, I'll work on backporting this test to 7.x later.

star-szr’s picture

To correct myself in #75 - contextual links (i.e. "Edit menu", "List links" for menu blocks) are currently broken in 8.x as per #1304470: Add tests for module-provided contextual links on blocks, but the "Configure block" links work. I will be updating the issue summary after I backport the test to 7.x.

star-szr’s picture

Version: 8.x-dev » 7.x-dev
Assigned: star-szr » Unassigned
Issue tags: -Needs tests
FileSize
2.21 KB

Here's a test for 7.x. Once this comes back from testbot I'll set the issue back to 8.x, so that the 8.x test can get reviewed/committed.

star-szr’s picture

Issue summary: View changes

Adding issue summary.

Status: Needs review » Needs work

The last submitted patch, 914382-80-test-D7.patch, failed testing.

star-szr’s picture

Version: 7.x-dev » 8.x-dev
Status: Needs work » Needs review

The 7.x test demonstrates the bug, setting back to 8.x and NR to get the test reviewed/committed. The issue summary has been updated as well.

star-szr’s picture

Issue summary: View changes

Noting that the issue is fixed in 8.x, updating remaining tasks, other tweaks

droplet’s picture

Status: Needs review » Needs work

anyone able to merge fixes and tests patch?

star-szr’s picture

Version: 8.x-dev » 7.x-dev
Status: Needs work » Needs review
FileSize
4.64 KB
2.21 KB

Here's #55 along with the test in #80. Setting to 7.x temporarily for testbot.

star-szr’s picture

Version: 7.x-dev » 8.x-dev
FileSize
1.12 KB
2.31 KB

And back to 8.x with an updated version of the test from #78 per http://drupal.org/node/1710766.

star-szr’s picture

Issue summary: View changes

Linkifying comment number, adding note about test for 7.x, updating remaining tasks.

dags’s picture

Assigned: Unassigned » dags
dags’s picture

Assigned: Unassigned » dags

The 8.x test in #85 is good to go.

I'm confused by the issue summary though. Under remaining tasks it says, "A fix for caching in contextual.module needs to be written for Drupal 7, and can be tested against #84." I tested the patch in #84 against a fresh D7 install and it fixes the issue. Are we looking for a different solution or can this be RTBC?

dags’s picture

Assigned: dags » Unassigned
star-szr’s picture

Assigned: dags » Unassigned

@dags - Thanks! The consensus in this issue seemed to be that the patch from #55 (the "fix" part of the test+fix patch in #84) was not the proper solution for D7.

See comments #57, #64, #68, #71 for a bit more background. Please feel free to update the issue summary if you can make more sense of it.

star-szr’s picture

Issue tags: -Needs backport to D7

#85: 914382-85.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Needs backport to D7

The last submitted patch, 914382-85.patch, failed testing.

star-szr’s picture

Assigned: Unassigned » star-szr

The test will need to be updated, I'll take a look in the next few days.

star-szr’s picture

Assigned: star-szr » Unassigned
Status: Needs work » Needs review
FileSize
2.07 KB
2.25 KB

Updated the Drupal 8 test per #1535868: Convert all blocks into plugins and #1872540: Provide a test helper method for creating block instances. I'll update the issue summary as well.

star-szr’s picture

Issue summary: View changes

Updating patch links

tim.plunkett’s picture

Issue tags: +Blocks-Layouts
+++ b/core/modules/contextual/lib/Drupal/contextual/Tests/ContextualBlockCacheTest.phpundefined
@@ -0,0 +1,57 @@
+ * Definition of Drupal\contextual\Tests\ContextualBlockCacheTest.

Contains \Drupal\...

+++ b/core/modules/contextual/lib/Drupal/contextual/Tests/ContextualBlockCacheTest.phpundefined
@@ -0,0 +1,57 @@
+  function setUp() {

protected function setUp

+++ b/core/modules/contextual/lib/Drupal/contextual/Tests/ContextualBlockCacheTest.phpundefined
@@ -0,0 +1,57 @@
+    $web_user = $this->drupalCreateUser(array('access content', 'access contextual links', 'administer blocks'));
+    $this->drupalLogin($web_user);

This can be combined into one line

+++ b/core/modules/contextual/lib/Drupal/contextual/Tests/ContextualBlockCacheTest.phpundefined
@@ -0,0 +1,57 @@
+    $this->assertNoLinkByHref($configure_link, 0, 'Configure block link for Active forum topics block is not displayed for logged out user.');
+  }

Missing blank line after method before end of class

---

Note that this will need tweaks if #1871696: Convert block instances to configuration entities to resolve architectural issues gets in first, or else that will have to convert this ($block['config_id'] to $block->id(), among others).

star-szr’s picture

FileSize
1.22 KB
2.23 KB

Thanks for the review, @tim.plunkett! Here's a revised patch with those changes.

star-szr’s picture

Issue summary: View changes

Updated issue summary.

star-szr’s picture

Issue summary: View changes

Updated issue summary.

catch’s picture

Why is the test passing?

star-szr’s picture

This was a bug in D7, fixed in D8 but there are no tests for it in D8 yet. The D7 version of the test is still failing (#84).

Wim Leers’s picture

Issue tags: -Needs backport to D7

#96: I was wondering the same thing.
#97: How is it possible that this was committed without tests? If it were new functionality, I'd understand.

It seems that it was concluded at #918808: Standardize block cache as a drupal_render() #cache that extra tests were unnecessary, so it was committed at #918808-67: Standardize block cache as a drupal_render() #cache without additional tests?

Wim Leers’s picture

Issue tags: +Needs backport to D7

Sigh, d.o.

catch’s picture

@Wim, #918808: Standardize block cache as a drupal_render() #cache didn't affect this afaik, or not intentionally. The forum block was already cached via the render cache, not the block cache, so should be unaffected. Am I missing something?

Wim Leers’s picture

#100: the issue summary of this issue claims so, maybe it's wrong?

This was originally fixed in 8.x via #918808: Standardize block cache as a drupal_render() #cache.

star-szr’s picture

Back in #77 I tested this bug before and after #918808: Standardize block cache as a drupal_render() #cache, and found that it was fixed by #918808: Standardize block cache as a drupal_render() #cache, whether intentionally or not. I'll re-test that tonight to confirm.

star-szr’s picture

Just (re-)confirmed that this bug was originally fixed in 8.x via #918808: Standardize block cache as a drupal_render() #cache. I've attached the patch I used to test with, it's the same test as in #80 but #918808: Standardize block cache as a drupal_render() #cache was before we had any tests for contextual.module.

git checkout 9763ca9
git apply 914382-test_9763ca9-103.patch
drush cc all
drush test-run --uri=d8.dev ContextualBlockCacheTestCase

Contextual link block caching 25 passes, 1 fail, 0 exceptions, and 8 debug messages
Test ContextualBlockCacheTestCase->testForumBlock() failed: Configure block link for Active forum topics block is not displayed for logged out user.

git checkout 08b9bd4
drush test-run --uri=d8.dev ContextualBlockCacheTestCase

Contextual link block caching 26 passes, 0 fails, 0 exceptions, and 8 debug messages

Status: Needs review » Needs work

The last submitted patch, 914382-test_9763ca9-103.patch, failed testing.

star-szr’s picture

Status: Needs work » Needs review

I really should have named that with -do-not-test.patch.

catch’s picture

OK I can't check from my phone but it looks to me like the block render array structure changed so that contextual links are a level up from the caching. That'd be enough for the forum block but not the issue title. At this point I think we should get the test committed, move this back to 7.x and open a new issue for the root cause.

Wim Leers’s picture

Indeed, the actual problem is not solved; the HTML that contextual.module inserts automatically is still specific to the current set of permissions that the current user has access to. Meaning that not the exact same block can be reused for e.g. anonymous and admin users. This was solved in edit.module by only the thing that you want to provide actions for in some way, so that we can then have some JS that does looks up the available actions for that thing and for the current user via AJAX. That's the only way to not reduce the render cache hit ratio.

effulgentsia’s picture

@catch pointed out in #1874664-114: Introduce toolbar level "Edit" mode that shows all contextual links that now that that issue landed, this issue becomes even more important.

Wim Leers’s picture

Assigned: Unassigned » Wim Leers
Issue tags: +in-place editing

I'll work on this. I most likely won't work on this this week though, so feel free to unassign if you want to work on this sooner. I'll comment when I start working on it.

effulgentsia’s picture

Issue tags: +caching

Adding the caching tag to issues that need to be fixed for blocks to be cacheable (for now with render cache, eventually with *SI).

Wim Leers’s picture

I likely will be able to start working on this tomorrow.

xjm’s picture

Issue tags: -caching +D8 cacheability
Wim Leers’s picture

The proposed solution

catch has indicated a long time ago, and in another issue, that the solution here is to indeed do something analogous to what Edit module is doing: only annotate the "thing" (Entity/View/Block/…) with some sort of identifier (probably in a data-contextual-id attribute, that a piece of JS can look for, submit to the server to know if the user can access the contextual links for each thing, and what those actions are.
The JS would then only be added if the current user has the access contextual links permission.

If you disagree with this approach, now is the time to speak up.

Consequences

For users that don't have permission to use contextual links, the overhead is limited to just <div data-contextual-id="<id>"></div> (in the location where the contextual links would appear), and class="contextual-region". If the user has access to contextual links, the JS will be loaded and will fill in the div[data-contextual-id]s.
These (<div data-contextual-id="<id>"></div> & class="contextual-region" end up in the render cache, and are served to everybody; hence allowing for full render cache compatibility.
Contextual Links before this patch worked when JS was disabled. Well, for an odd definition of "working": the links would appear, but they would be mostly-inappropriate pieces of unordered links. Now, JS is a hard requirement.

Potential next steps

The attached patch is the simplest possible first iteration of this patch, which changes as little as possible yet still no longer breaks the render cache.

Views has to do very crazy hacks to make Contextual Links work well, see Drupal.behaviors.viewsContextualLinks & views_preprocess_html(). I updated this to work in an analogous (yet slightly simpler) way, but it'd be nice to simplify that for Views.

Edit module's live modification of contextual links also is updated to work with the latest code.

Also attached: a "just_contextual" version of the patch, which excludes the updates to the Views & Edit modules for continued compatibility.


Related: #1914966: Nested contextual links triggers don't work well.
jessebeach’s picture

Issue tags: -Needs tests, -sprint, -Spark
+++ b/core/modules/contextual/contextual.jsundefined
@@ -11,19 +11,62 @@ var contextuals = [];
+    $.ajax({
+      url: Drupal.url('contextual/render'),
+      type: 'POST',
+      data: { 'ids[]' : ids },
+      dataType: 'json',
+      success: function(results) {

Great, my first thought upon hearing of this was "How many AJAX calls are we making?" But all the contextual links are rolled into one call. Nice.

+++ b/core/modules/contextual/contextual.jsundefined
@@ -11,19 +11,62 @@ var contextuals = [];
+        for (var id in results) if (results.hasOwnProperty(id)) {
+          var $contextual = $context
+            // Find the location for the current rendered contextual link.
+            .find('[data-contextual-id="' + id + '"]')
+            // Move it into the DOM.
+            .html(results[id]);
+          // Create a Drupal.contextual object and notify listeners of a new
+          // contextual link.
+          $(document).trigger('drupalContextualLinkAdded', {
+            contextual: new Drupal.contextual($contextual, $contextual.closest('.contextual-region'))
+          });

This if statement should be on its own line.

Otherwise it looks good.

Status: Needs review » Needs work

The last submitted patch, contextual_links_render_cache-914382-113-just_contextual.patch, failed testing.

David_Rothstein’s picture

Are some of the earlier tests missing from the patch?

Requiring JavaScript for contextual links is probably OK but we can't do this every time someone wants to add dynamic HTML to a bunch of blocks for any purpose. Maybe we make a special exception for contextual links, though, since they're very common?

In general, I would say the issue is that the drupal_render() cache gives you no opportunity to modify the HTML after it is pulled from the cache. If it did, you could do something with tokens - e.g. <some html /> ... !some-dynamic-token ... <more html /> could be what's stored in the render cache and then after it's pulled out of the cache, Contextual Links or other modules would get an opportunity to modify it and substitute in either an empty string or some actual HTML as appropriate.

I guess this is fundamentaly incompatible with ESI, but if lots of blocks on your page are highly dynamic then it already is. So that is a possible reason to make an exception for contextual links and use JavaScript for those instead (to preserve the compatibility).

At this point I think we should get the test committed, move this back to 7.x and open a new issue for the root cause.

Makes sense to me too. I think it would be better if this new approach did not interfere with getting the simpler parts committed and backported.

moshe weitzman’s picture

The proposed approach (and patch!) looks solid to me as well. This had been broken for a long time; its great to have a fresh new direction.

dawehner’s picture

+++ b/core/modules/contextual/lib/Drupal/contextual/ContextualController.phpundefined
@@ -0,0 +1,63 @@
+        $args = explode(':', $context);
+        $provider = array_shift($args);
+        $pos = strpos($provider, '[');
+        $module = drupal_substr($provider, 0, $pos);
+        $parent_path = drupal_substr($provider, $pos + 1, drupal_strlen($provider) - $pos - 2);

These lines certainly need some documentation :)

Manual testing showed the following problems:

  • There is no views edit link anymore shown on the page, even if it's actual part of the html.
  • The contextual links field in views doesn't work anymore, see provided yml file.

I'm happy to help here, but I'm just wondering first whether your approach supports this in general?

Gábor Hojtsy’s picture

Issue tags: +Performance

@David: loading them later/on demand also helps with performance issues. The more contextual links avoided, the speedier the page. Replacing tokens in the HTML before output would not mitigate the performance concerns and not let us hit two birds with one stone here :)

catch’s picture

@David's #116 that's been discussed a few times in relation to the js dependency.

If we end up establishing js-replacement as the standard approach to this (which I've been trying to encourage for well over a year now), adding a post-process layer to the render cache, explicitly limited to internal caching, would then allow for a PHP implementation of the replacement for sites that don't use *SI and don't want the js dependency. I don't think we need to support that feature in core but would be very happy to add what we need so that contrib could. Also things like allowing cache ID granularity to be pulled out in the same way as cache tags/#attached are, and for allowing elements to disable render caching (including on their parents) altogether - but these are all follow-ups.

I don't think any of the previous patches on this issue are viable even for Drupal 7 - they change one specific cache ID for forums but don't deal with the problem anywhere else at all.

Wim Leers’s picture

#114: RE: the if-test being on its own line. The Drupal JS coding standards said differently, but after checking with nod_, that's an outdated aspect of the coding standards: http://drupal.org/node/172169#forin. Fixed: http://drupal.org/node/172169/revisions/view/2343526/2620684. Fixed in rerolled patch, too.


#116:

Are some of the earlier tests missing from the patch?

Yes: they made assumptions that are no longer true, and they don't provide complete enough test coverage.
In this reroll, I've completely revamped the tests.

Requiring JavaScript for contextual links is probably OK but we can't do this every time someone wants to add dynamic HTML to a bunch of blocks for any purpose. Maybe we make a special exception for contextual links, though, since they're very common?

[… and further reasoning]

It seems you agree, but you just have reservations. Or do you lean towards disagreeing with the approach taken?
Note that (besides Gábor's point in #119):

  1. an AJAX request will only be made if there is a data-contextual-id attribute in the "dynamic HTML"
  2. a follow-up issue could be to devise fancy caching strategies to reduce the need to do this. E.g. for each "contextual ID", cache the results in localStorage for X seconds. (Advanced access control modules could then disable this client-side caching.) This would reduce the cost.

#118:

These lines certainly need some documentation :)

Hah, they sure do :) I've pulled that code out of there, into _contextual_links_to_id() and _contextual_id_to_links() (the inverse), documented those, and wrote a unit test for them.
Note that the JS never has to parse these; it just finds the ids, and passes them to the server.

Manual testing showed the following problems:

Hm… all I can say is that I tested with the default Drupal 8 frontpage view, and that worked fine.

Your screenshots is for the frontpage view, and your .yml file is for a new contextual view. Both work fine. See attached screenshot (worksforme.png). Screenshots were made with the patch in #113.

That being said, the contextual links in #113 do not do the deeplinking that Views used to do before — the #views_contextual_links_info data is lost. I realize that was necessary because contextual links equate to local tasks, and that doesn't allow for linking even deeper (the display ID that the user would start editing).
The solution to this problem I propose is simple: get rid of Views' special handling (i.e. all of #views_contextual_links_info), and allow some metadata to be associated with a contextual link. Views can then continue to use views_ui_contextual_links_view_alter() to cleanly make dynamic link alterations. Everything else remains the same.
What this really means is that instead of only associating a "parent path" and "dynamic path arguments", we now allow a third thing to be set for each contextual link: metadata. That's it.

What's left to do there is to clean up all references in Views' code comments to #views_contextual_links_info :)


Also fixed a problem I'd discovered with my patch: all contextual links had ?destination=contextual/render, which is of course not quite right.


Two interdiffs:

  1. "step ONE": addresses feedback + introduces full test coverage (all possible permission cases + unit tests for generating the "contextual IDs" and going back to $element['#contextual_links']
  2. "step TWO": addresses the Views edge case (and fixes the test failure there) by adding the ability for contextual links to ship with some metadata; consequently updates the format used for the "contextual IDs".

Status: Needs review » Needs work

The last submitted patch, contextual_links_render_cache-914382-121.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
34.11 KB

Oops.

Status: Needs review » Needs work

The last submitted patch, contextual_links_render_cache-914382-123.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review

@Wim Leers
It's not about the edit view links, it's a totally separate feature in views to display contextual links as a field,
see ContextualLinks.php
If we have this approach to load the links via javascript one contextual per row seems not really perfect?

Wim Leers’s picture

Stupid, stupid, I hardcoded my own base path (REALLY! n00b!) and forgot to update the main contextual links test after doing the whole "step two" thing described in #121.

Tests fixed. This should pass.

EDIT: #125: I see your problem now. Reproduced. Working on addressing it.

Wim Leers’s picture

Regarding the Views field handler plugin ContextualLinks.php:

  1. It doesn't have test coverage.
  2. It duplicates parts of contextual.module!
  3. It does this because it's not relying on local menu tasks… but instead on *any* kind of View Field that is a link. You could even add a custom field that links to foobar.com and then have that appear as a contextual link!
  4. (It seems this was originally added at #1019486: Display fields in a contextual links block?)

Fortunately, even this sort of insane use case (from a contextual.module POV) can easily be supported by the measures taken in step two of #121: just use the metadata that you can associate with a contextual link!

However, if you configure the View to have many contextually linking fields, then you're also going to end up with quite large contextual IDs in your HTML. But, since this whole feature already goes against (or rather, circumvents) the architecture of contextual links in the first place, I think this is only fair.

This patch fixes Views' ContextualLinks.php to be compatible with the approach I've followed in my patches, but doesn't add test coverage (that's not my responsibility).

dawehner’s picture

Thanks for fixing it!

It doesn't have test coverage.
It duplicates parts of contextual.module!
It does this because it's not relying on local menu tasks… but instead on *any* kind of View Field that is a link. You could even add a custom field that links to foobar.com and then have that appear as a contextual link!
(It seems this was originally added at #1019486: Display fields in a contextual links block?)

I wouldn't have assumed a test coverage for that :) The reason why it duplicates parts of contextual module is simply as you said, because contextual is partly not really flexible. I think this though is a great tool for sitebuilder. Added a follow up for the test: #1954804: Add a test for the contextual link views field

David_Rothstein’s picture

It seems you agree, but you just have reservations. Or do you lean towards disagreeing with the approach taken?

I agree; I think for contextual links in particular this Ajax approach makes sense and looks good. I'm just worried about setting too much of a precedent. Drupal is a dynamic content management system; we can't tell developers they're doing it wrong any time they want to put lots of dynamic HTML directly on a page...

A quick and cursory patch review:

  1. It's claimed that this is a performance improvement but I don't see how; the server is doing the same work, right? Maybe the initial page load will happen a split second faster for admins (with the contextual links following a split second later).

    However, on the opposite side I see what looks like it could be a performance regression for the site overall:

     function contextual_preprocess(&$variables, $hook) {
    -  // Nothing to do here if the user is not permitted to access contextual links.
    -  if (!user_access('access contextual links')) {
    -    return;
    -  }
    -
    

    This means the rest of the function will run for every request... and particularly on pages with lots of potential contextual links perhaps this could add up and be significant?

  2. +function contextual_custom_theme() {
    +  if (substr(current_path(), 0, 11) === 'contextual/') {
    

    No need to parse paths in hook_custom_theme(); just use 'theme callback' in hook_menu() instead.

  3. @@ -479,10 +484,11 @@ function views_preprocess_html(&$variables) {
       // page.tpl.php, so we can only find it using JavaScript. We therefore remove
       // the "contextual-region" class from the <body> tag here and add
       // JavaScript that will insert it back in the correct place.
    -  if (!empty($variables['page']['#views_contextual_links_info'])) {
    +  if (!empty($variables['page']['#contextual_links']['views_ui'])) {
    

    What happens if another module (rather than Views UI) is attaching contextual links to the view?

Wim Leers’s picture

#128: Great :)


#129:

Drupal is a dynamic content management system; we can't tell developers they're doing it wrong any time they want to put lots of dynamic HTML directly on a page...

This issue is not about making that particular aspect harder, or to make dynamically generated HTML a bad practice.

It's only about making sure that for content that does have a certain cacheability TTL does not get broken by unnecessarily generating unrelated dynamic HTML, which is precisely what contextual.module was doing.

Your 3 patch review points addressed:

  1. It's claimed that this is a performance improvement but I don't see how; the server is doing the same work, right?

    Yes, and because it's first generating placeholders, it's actually doing more work: all pageloads only get the placeholder. Post-pageload a piece of JS will cause the server to render the actual contextual links like we had them before.
    The point is — again — that we don't break cacheability of (very expensive to render) things.

    This means the rest of the function will run for every request... and particularly on pages with lots of potential contextual links perhaps this could add up and be significant?

    This is where you're wrong. It is precisely this user access check that broke the render cache. This is what caused the rendering to happen for every page load, whereas now it has become cacheable. This is the primary performance improvement. The secondary performance improvement is that instead of rendering contextual links, we now only render a placeholder, which is cheaper (less steps, less bytes on the wire).

  2. No, contextual.module uses Drupal 8's new routing system (which is mandatory). This is how routing system developers said we should do this for now, until we figure out how to subsume this into the routing system itself. It's a temporary thing, hence the @todo comment.
  3. Before and after, the code is Views UI-specific. I don't think this is an actual problem.
David_Rothstein’s picture

  1. I was actually referring to the potential performance hit for sites not using a render cache.
  2. How will anyone find that @todo later? They will definitely find a theme callback though (since that's what the rest of core uses) and be able to convert it along with the rest of them. And you don't have to give up on using the new routing system to do this; I believe you just need a dummy hook_menu() entry that has a 'route_name' key plus the theme callback. That will link it to the route.
  3. I wrote the original version of that code and I am close to 100% positive it's not Views-UI specific :) Any module which wants to attach any kind of contextual links to a view display can do so; the processing for this happens in views_add_contextual_links() and is module-agnostic. Views UI happens to do it via views_ui_views_plugins_display_alter() but there is nothing particularly special about that implementation.
Wim Leers’s picture

  1. In that case, the cost on the initial page load is only very slightly more (assuming it's more expensive to generate and set the contextual ID than checking permissions, which probably is true). However, there's a subsequent AJAX request for users that have access to contextual links, so in that case, performance indeed gets slightly worse.
    Isn't this easily fixable, by opting in more (as many as possible) render arrays to use #cache? AFAICT it's up to the render array, not up to the site, to use "a render cache"?
  2. That @todo is in several locations already. I don't know why we don't have a dummy hook_menu() entry; I just followed the example set by people far more familiar with the routing system than I am :)
  3. Aha, I see your point now :) Does the attached interdiff satisfy your concerns?
effulgentsia’s picture

I believe you just need a dummy hook_menu() entry that has a 'route_name' key plus the theme callback.

I don't think that's true. If the route exists in the new routing system, it takes precedence, and nothing from the old hook_menu() is merged with it at the time of request processing. We do now have #1954892: Replace 'theme callback' and hook_custom_theme() with a clean theme negotiation system to figure the theme callback problem out, so the todos can now link to that.

sdboyer’s picture

@effulgentsia just pointed me to this - thanks.

way too much for me to absorb here right now, but i've read back about 20 comments. from that:

#113: only annotate the "thing" (Entity/View/Block/…) with some sort of identifier (probably in a data-contextual-id attribute, that a piece of JS can look for, submit to the server to know if the user can access the contextual links for each thing, and what those actions are.

this is good, and SCOTCH will be fine with this approach.

#120: If we end up establishing js-replacement as the standard approach to this (which I've been trying to encourage for well over a year now), adding a post-process layer to the render cache, explicitly limited to internal caching, would then allow for a PHP implementation of the replacement for sites that don't use *SI and don't want the js dependency.

for blocks, at least, it sounds feasible. we're oriented towards a slightly different caching system there, of course, but there's (already, in the princess branch) a couple points i can think of where such a post-processing layer could be injected.

#129Drupal is a dynamic content management system; we can't tell developers they're doing it wrong any time they want to put lots of dynamic HTML directly on a page...

and

#116Requiring JavaScript for contextual links is probably OK but we can't do this every time someone wants to add dynamic HTML to a bunch of blocks for any purpose. Maybe we make a special exception for contextual links, though, since they're very common?

frankly, when it comes to just throwing stuff on the page, we need to get more comfortable with saying, "don't do it that way, there's a right way to do things." that having been said, from a SCOTCH perspective, it's at least as useful if we have an explicit way of knowing that someone is lazily injecting HTML all over the place, rather than (re)writing a new block that works they way they actually want it to.

most important, though, is that injecting HTML all over is only a problem IF some aspect of that injection is made conditionally on something from unencapsulated global state. only at that point does it become cache-busty. it's perfectly fine for things to vary, so long as they vary in a manner we can predict from the outside by inspecting the data that's injected. as long as those data represent the entire range of variation, we're more or less in the clear.

effulgentsia’s picture

Status: Needs review » Needs work

The issue summary hasn't been updated yet for the new direction, so adding that tag.

To address #131.1, it would also be good to get benchmarks here for anonymous users not using page or block caches for a page with a good representative number of contextual regions and links. I don't think we need xhprof; apache benchmark (ab) averages should be sufficient. We know where the hit is: it's in contextual_preprocess() querying the theme registry and generating all those long ids. The question is whether it's a tiny addition to the page (both CPU and bandwidth) or a significant one. We may certainly make the decision that what we gain in caching offsets the cost, but we can only make that decision after knowing what the cost is.

I'm also a little concerned about all those ids (containing quite a bit of information) being disclosed to all users. I haven't yet though of a specific case where such disclosure is an obvious problem, but I'm also not yet confident that it's not a problem.

Related to the disclosure, it also seems like someone with 'access contextual links' permission could craft some Ajax requests with alternate ids. I don't yet know if that's safe (they'd just get back a JSON response with some links), or if that could be exploited to do something damaging or gain access to sensitive information.

Finally, I think it would be great if we could vary cache by permission. We can do it by role, but that's more granular than necessary. Thereby, only adding the data-contextual-id attributes to users with 'access contextual links' permission. I'm ok with us punting that to a separate issue, but it could potentially alleviate the performance and security concerns.

Wim Leers’s picture

What's the new direction? sdboyer said the current approach is good, but that a currently experimental branch exists where a new and slightly different caching system will be introduced.

So: can somebody clarify that, because the two above comments feel like some things were communicated somewhere else? Thanks :)

Gábor Hojtsy’s picture

@Wim: the new direction is your proposed patch vs. the issue summary. The issue summary currently says this:

To be determined. The patch in #8 is reported to fix this issue with the forum module in Drupal 7, but does not fix the larger problem.

One proposed fix (#48) for Drupal 7 is to have modules implementing custom caches use CACHE_PER_ROLE or CACHE_PER_USER when caching.

That is clearly not the currently proposed fix.

Wim Leers’s picture

Oh, hah, LOL! Stupid me! :D I'll do that later today. :)

As for benchmarks: I get that we now do more work for users without access to contextual links, but OTOH that's precisely the goal, isn't it? No more conditionals, just always have it there, and always have it cached. Over many requests, that will result in a net benefit, if (and only if) the render cache is leveraged (almost) everywhere.
So, in order to be able to do benchmarks, I think we also need to know what the render cache plans are? AFAICT, only block.module currently sets a #cache property, so e.g. Views, full node views etc. don't yet use the render cache.

On per-permission caching: AFAICT this would amount to worse performance, precisely because it's no longer the same for everybody. More cache storage required, more calculations required. The only potential benefit is disclosure prevention (that's assuming there's worthwhile information in there).

On disclosure: AFAICT we don't ever disclose sensitive information, only identifiers, and when the user has sufficient permissions, he can only retrieve more links (though they're again governed by menu system-level permissions).

catch’s picture

Enabling render caching for entities is here #1605290: Enable entity render caching with cache tag support.

@effulgentsia currently forum module caches the links for anonymous users in Drupal 7 and displays them to everyone without any URL trickery, so from an 'inaccessible link disclosure' point of view, even if there's a risk here it's fixing an existing issue that's worse.

Wim Leers’s picture

So, as per #139: disclosure is a non-issue.

@catch: Can you comment on the benchmarks/per-permission caching/general performance aspects? What do you think is necessary to move this forward?

sdboyer’s picture

sdboyer said the current approach is good, but that a currently experimental branch exists where a new and slightly different caching system will be introduced.

So: can somebody clarify that, because the two above comments feel like some things were communicated somewhere else? Thanks :)

first, for clarity, the princess branch is in the SCOTCH sandbox.

i haven't just quite gotten to the caching mechanism yet, so i unfortunately can't point you to code (though i took a break from writing the contextual integration that supports it to write this response). it's rooted in the idea that by knowing what we inject into a block, we know its caching dimensions. blocks will be able to say "i'm uncacheable," or that the granularity level is less than that which the injected data might seem to imply (e.g., they require a user object, but vary merely by role, not by uid), but no more of the grand constants like CACHE_PER_ROLE, CACHE_PER_USER.

since the caching dimensions of a given block will be expressed as an abstracted, additive concept (one can incrementally add more dimensions to the cache), that will make it feasible for alters/external code to inform the cache layer that they are introducing an new dimensions, as well. so, things like only showing contextual links if a role is present is something we can express...though such an approach obviously has diminishing returns.

sdboyer’s picture

Issue summary: View changes

Updated issue summary.

Wim Leers’s picture

Status: Needs work » Needs review

#141: I understand what you're saying. However, it does sound like the net result is: a cache tag/key with another dimension, and thus more cache storage required, and lower cache hit ratio. Correct me if I'm wrong :)


Issue summary updated, but I doubt that I worded the server-side solution proposals well. Please improve that part. Thanks :)


It really sounds like sdboyer (and effulgentsia also maybe?) is arguing to fix this problem on the server-side (more flexible caching and/or post-processing), whereas the patches I've been working on solve this problem on the client-side (maximum cache hit ratio and thus amortized fastest to serve the page).

What catch said in #120 though sounded like we might want to have both:
If we end up establishing js-replacement as the standard approach to this (which I've been trying to encourage for well over a year now), adding a post-process layer to the render cache, explicitly limited to internal caching, would then allow for a PHP implementation of the replacement for sites that don't use *SI and don't want the js dependency.

I think this what we have to decide to move forward: fix render cache breakage on the server-side, client-side, or both?

My thinking: they have different performance characteristics of course, so in that sense "both" is the only sensible option. But, it sounds like for the server-side option to become feasible, more work needs to be done in SCOTCH, so that seems to indicate the server-side solution should be a follow-up? I also still haven't seen convincing arguments yet on why the client-side solution is unacceptable for some sites. "Not wanting the JS dependency" is not really a valid reason IMO, because Contextual by definition requires JS already. The only big reason I can think of: less HTTP requests.

effulgentsia’s picture

Status: Needs review » Needs work
+++ b/core/modules/contextual/contextual.module
@@ -24,14 +36,13 @@ function contextual_toolbar() {
+        array('contextual', 'drupal.contextual-links'),

I have some additional feedback I'll post soon, but in the meantime, I noticed that this makes contextual links unavailable to users without access to the toolbar, or for sites with toolbar disabled.

effulgentsia’s picture

This also needs a reroll due to #1914966-15: Nested contextual links triggers don't work well having landed.

effulgentsia’s picture

Issue summary: View changes

Revamped.

effulgentsia’s picture

Issue summary updated.

Thanks. Looks good.

but I doubt that I worded the server-side solution proposals well. Please improve that part.

I opened #1961884: Add support for 'permissions' and/or 'contexts' to render cache system and linked it from the summary.

As for benchmarks: I get that we now do more work for users without access to contextual links, but OTOH that's precisely the goal, isn't it?

No. A goal to spend more time doing something would be a silly goal :) That time is a cost. The question is whether the cost is offset by better cache hit ratios or other benefits. To answer that, we need to know what the cost is. I ran some benchmarks, and here's what I found:

  • I created a new site with the Standard profile, added 5 Article nodes with titles and empty bodies. That results in a front page with 9 contextual regions: 5 node teasers and 4 blocks. On HEAD, on my machine, running ab -c1 -n100 for the anonymous user came in at a median time of 493ms.
  • That seemed like way too long: I've run benchmarks on my machine before (though not in the last 6 months or so), and most pages I tested back then would clock in at 30-150 ms. So, I ran the same test on D7 HEAD, and that clocked in at 78ms. So unless my computer is anomalous, there's some really serious problems in D8 right now. Maybe it's Twig, maybe Views, maybe something else, but until we fix that, we need to be careful in interpreting benchmarks. An X% degradation now is more like a 5X% degradation in reality once we fix whatever the really bad regressions are.
  • When I applied the patch though, I found 0 regression, which is good, because 5*0 is still 0. In hindsight, it's actually not that surprising: the extra code being run by contextual_preprocess() is minimal: running it 9 times in a page request is easily well under a fraction of 1ms. All the heavy work is offloaded to the AJAX request.
  • However, while there's no CPU regression, it did result in outputting a page that was 14.8KB instead of 14KB (uncompressed). In other words, 5% more download time. I suspect there's a lot of sites where ~90% of traffic comes from anonymous visitors and ~90% of authenticated traffic comes from users without 'access contextual links' permission, so this is adding 5% download time to potentially >99% of site visitors for no benefit to them. That's why I recommend doing #1961884: Add support for 'permissions' and/or 'contexts' to render cache system, but I recommend finishing this issue without that, and doing that one as a follow up.
  • I was initially worried that users without 'access contextual links' permission would also be triggering an AJAX request only to be returned a (uncached) 403 response, but I then saw that that's not the case, because contextual.js is only added to the page if you have that permission: yay! Wim's updated summary also clarifies that nicely. Per #143, we need to decouple this from toolbar permissions, but that should be an easy fix.

from an 'inaccessible link disclosure' point of view, even if there's a risk here it's fixing an existing issue that's worse.

Ok, that works for me. For reference, here are the divs that were added in my test.

<div data-contextual-id="views_ui:admin/structure/views/view:frontpage:location=page&name=frontpage&display_id=page_1"></div>
<div data-contextual-id="node:node:5:"></div>
<div data-contextual-id="node:node:4:"></div>
<div data-contextual-id="node:node:3:"></div>
<div data-contextual-id="node:node:2:"></div>
<div data-contextual-id="node:node:1:"></div>
<div data-contextual-id="block:admin/structure/block/manage:bartik.login:"></div>
<div data-contextual-id="menu:admin/structure/menu/manage:footer:|block:admin/structure/block/manage:bartik.footer:"></div>
<div data-contextual-id="block:admin/structure/block/manage:bartik.powered:"></div>

These don't seem to be disclosing anything sensitive. Though if we do #1961884: Add support for 'permissions' and/or 'contexts' to render cache system, then we also fix this disclosure in case there are contrib/custom modules that have more sensitive links.

it also seems like someone with 'access contextual links' permission could craft some Ajax requests with alternate ids. I don't yet know if that's safe

Looking more into contextual_pre_render_links() and menu_contextual_links(), I agree with the last statement in #138 that this does appear harmless. If it turns out there is a vulnerability there, hopefully someone will find it and file a critical issue for it prior to 8.0 release.

In summary, I agree that this issue is close to ready, so let's finish it up!

Berdir’s picture

@effulgentsia: Those numbers seem quite high to me. On my laptop, I see 150ms vs 37ms. That's still crap but I'm surprised by the difference to your numbers, both absolute and the relative difference. I also displayed 5 nodes (including image and body).

Anyway, right now is quite a bad time to do such a comparison, there are a lot of known issues that we're working on :) Like doing 135 queries (tons and tons of cache queries, looks like a views data cache bug, lots of cache config, plugins, ..), having the Entity BC decorator on nodes and so on.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
5.53 KB
39.01 KB

#143: Absolutely. But… to what else should it be attached then? To the page itself? In D8, it's disallowed to just call drupal_add_library(), we must


#144: Indeed; reroll attached.


#145:

The question is whether the cost is offset by better cache hit ratios or other benefits.

Well… that's what I meant by that of course :) Intentionally spend more time, so that the amortized result is better.

However, while there's no CPU regression, it did result in outputting a page that was 14.8KB instead of 14KB (uncompressed)

Could you do that test also on a page that has no view? e.g. node/1? >90% of that .8 KB is going to come from the crazy stuff I had to add in #127, to support that "ContextualLinks" Views field handler plugin. IMVHO the entire way that plugin functions is just wrong. It's generating contextual links not based on menu system entries (which is how everything else works), which means there's no way to automatically generate them, which is why it setting $element['#links'], which is why we have to serialize the entire $element['#links'] value, which is going to be >90% of that .8 KB. See the interdiff: http://drupal.org/files/interdiff_4059.txt.
For "non-hacky" or "used as intended" contextual links, the attributes that are added are a teeny tiny bit of overhead, most of which will compress away when using gzip.

Per #143, we need to decouple this from toolbar permissions, but that should be an easy fix.

It's indeed simple, but it might be counterintuitive. We can't attach it to each contextual link, because then we'd be loading the contextual JS even for users that don't have the "access contextual links" permission. With the toolbar out, we need something else that is "global" or at least present on every page. In this reroll, I went with hook_page_alter(), if somebody has a better idea: shoot! :)


I also took this opportunity to define the "contextual links placeholder" as a proper Drupal element. Less hacky end result. Now the flow is: any $element['#contextual_links'] element receives a $element['title_suffix']['#type'] = 'contextual_links_placeholder'; if contextual.js is loaded (for users with right perms) and finds these placeholders, they're sent to the server and then rendered as #type === 'contextual_links'. Much cleaner.

This should bring it close to "finished up" :)

Status: Needs review » Needs work

The last submitted patch, contextual_links_render_cache-914382-146.patch, failed testing.

Wim Leers’s picture

I also had those two fails locally, but I was hoping that was due to something being broken locally, rather than in Drupal.

The only way those two failures can happen is if

$this->editor_user = $this->drupalCreateUser(array('access content', 'access contextual links', 'edit any article content'));
$this->authenticated_user = $this->drupalCreateUser(array('access content', 'access contextual links'));

still causes these users to have the 'administer views' permission.

It seems this is a regression in WebTestBase, but that seems so very unlikely…

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
37.61 KB

Patch did not apply anymore. The underlying code changed thanks to #1913086: Generalize the overlay tabbing management into a utility library. Hopefully I properly rerolled it :) Another set of eyes never hurts. Now looking at the fails.

Status: Needs review » Needs work

The last submitted patch, contextual_links_render_cache-914382-150.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
39.82 KB
720 bytes

For some reason where the tests assert NULL it is now coming back rendered as an empty string, not NULL. Also, there is a new views block test that failed, applied the same code as in the views page test from above the patch. The two views contextual link fails will still be there, I could not track that one down yet...

Gábor Hojtsy’s picture

FileSize
4.12 KB

That was the wrong interdiff.

Status: Needs review » Needs work

The last submitted patch, contextual_links_render_cache-914382-152.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
2.3 KB
42.21 KB

Found the cause! Views UI uses routes instead of hook_menu() for access checking. But menu_contextual_links(), which is the driving force for generating contextual links, doesn't use route access checks yet… I added a temporary work-around with @todos linking to the issue where this is going to be solved properly.

Wim Leers’s picture

Status: Needs review » Needs work
effulgentsia’s picture

+++ b/core/modules/contextual/contextual.module
@@ -41,6 +51,22 @@ function contextual_toolbar() {
+function contextual_page_alter(&$page) {

While rerolling, please change this to contextual_page_build(). That's the better hook for adding new things. Then modules can use hook_page_alter() to alter what another module added.

franz’s picture

Status: Needs work » Needs review
FileSize
42.21 KB

Addressing #157

Status: Needs review » Needs work

The last submitted patch, contextual_links_render_cache-914382-155.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Postponed
FileSize
43.5 KB

Rerolled in response to #1971108: Convert contextual.js to use Backbone (and support dynamic contextual links) being committed. But, in doing so, I noticed that this would overlap with changes at #1981760: Edit should use the new drupalContextualLinkAdded event to cleanly instead of hackily insert its "Quick edit" contextual link too.

So, I rerolled this on top of HEAD, then on top of #1981760: Edit should use the new drupalContextualLinkAdded event to cleanly instead of hackily insert its "Quick edit" contextual link. It's RTBC, so it should be committed soonish. Then this will hopefully be pretty much the final patch.

Interdiff between #155 and this is impossible to provide (patch didn't apply anymore). All that has changed is the hunks at core/modules/edit, core/modules/contextual/contextual.js and core/modules/contextual/contextual.toolbar.js. Thanks to the work that's already been done in other issues, this patch has become simpler.

Wim Leers’s picture

Wim Leers’s picture

Let's see if #160 still applies.

Wim Leers’s picture

Status: Needs review » Needs work
Issue tags: +Performance, +Needs backport to D7, +sprint, +Blocks-Layouts, +in-place editing, +Spark, +D8 cacheability

The last submitted patch, contextual_links_render_cache-914382-160.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
43.52 KB

The context of two hunks had changed.

effulgentsia’s picture

Rerolled for HEAD changes.

Status: Needs review » Needs work

The last submitted patch, contextual_links_render_cache-914382-166.patch, failed testing.

effulgentsia’s picture

+++ b/core/modules/block/lib/Drupal/block/Tests/Views/DisplayBlockTest.php
@@ -153,9 +153,28 @@ public function testViewsBlockForm() {
+    $id = 'block:admin/structure/block/manage:' . $block->id() . ':|views_ui:admin/structure/views/view:test_view_block:location=block&name=test_view_block&display_id=block_1';
...
+    $this->assertIdentical($json[$id], '<ul class="contextual-links"><li class="views-ui-edit odd first last"><a href="' . base_path() . 'admin/structure/views/view/test_view_block/edit/block_1?destination=test-page">Edit view</a></li></ul>');

$id includes 2 items, but we're asserting that only 1 got rendered. I suspect it's because the user doesn't have permission to the Block one. If we're intending to test that, then please add a comment saying so. Otherwise, either remove the Block one from $id or add appropriate permissions and assert that it got rendered.

+++ b/core/modules/contextual/contextual.module
@@ -6,6 +6,18 @@
+ * @todo Add an event subscriber to the Ajax system to automatically set the
+ *   base page theme for all Ajax requests, and then remove this one off.

Please add a link to #1954892: Replace 'theme callback' and hook_custom_theme() with a clean theme negotiation system.

+++ b/core/modules/contextual/contextual.module
@@ -142,14 +173,11 @@ function contextual_element_info() {
+ * @see contextual_page_alter()

contextual_page_build().

+++ b/core/modules/contextual/contextual.module
@@ -165,18 +193,41 @@ function contextual_preprocess(&$variables, $hook) {
+    // the render cache. The drupal.contextual-links library's JavaScript must
+    // only be loaded if the user has the 'access contextual links' permission.

Why must it? How about: "Although the empty placeholder is rendered for all users, contextual_page_build() only adds the drupal.contextual-links library for users with 'access contextual links' permission, thus preventing unnecessary HTTP requests for users without that permission."

+++ b/core/modules/contextual/contextual.module
@@ -230,3 +281,74 @@ function contextual_pre_render_links($element) {
+/**
+ * Implements hook_contextual_links_view_alter().
+ *
+ * @see \Drupal\contextual\Plugin\views\field\ContextualLinks::render()
+ */
+function contextual_contextual_links_view_alter(&$element, $items) {
+  if (isset($element['#contextual_links']['contextual'])) {
+    $encoded_links = $element['#contextual_links']['contextual'][2]['contextual-views-field-links'];
+    $element['#links'] = drupal_json_decode(rawurldecode($encoded_links));
+  }
+}

This seems Views-specific. Why is it being added to contextual.module?

+++ b/core/modules/contextual/contextual.module
@@ -230,3 +281,74 @@ function contextual_pre_render_links($element) {
+ * Serializes #contextual_links property metadata to a "contextual id".

Let's avoid the word "metadata" here. How about "Serializes a #contextual_links property value array to a string."

+++ b/core/modules/contextual/contextual.module
@@ -230,3 +281,74 @@ function contextual_pre_render_links($element) {
+ * So, expressed in a pattern:
+ *  <module name>:<parent path>:<path args>:<metadata>
+ *
+ * The (dynamic) path args are joined with slashes. The metadata is encoded as a
+ * query string

I also wonder whether "metadata" is a poor term here as well. Perhaps "options" instead?

+++ b/core/modules/contextual/contextual.module
@@ -230,3 +281,74 @@ function contextual_pre_render_links($element) {
+ *   A contextual id.

What's a "contextual id"? How about: "A string value that can be output to an HTML data attribute."

+++ b/core/modules/contextual/contextual.module
@@ -230,3 +281,74 @@ function contextual_pre_render_links($element) {
+ * Serializes a contextual id back to #contextual_links property metadata.

Unserializes the result of _contextual_links_to_id().

+++ b/core/modules/contextual/lib/Drupal/contextual/ContextualController.php
@@ -0,0 +1,50 @@
+use Drupal\Core\Ajax\AjaxResponse;

The class doesn't use this.

+++ b/core/modules/contextual/lib/Drupal/contextual/ContextualController.php
@@ -0,0 +1,50 @@
+    $ids = $request->request->get('ids');

Let's add a comment here explaining why it's ok that we're bypassing FAPI or any other validation. How about "This is processing a POST request that is not a Drupal form. There is therefore no Form API validation of this value. The rendering logic invoked by the rest of this function must therefore be robust to arbitrary user input."

+++ b/core/modules/views_ui/views_ui.module
@@ -55,16 +55,28 @@ function views_ui_menu() {
+    // @todo Remove this. Route access checking does not work for contextual
+    // links generation yet. @see http://drupal.org/node/1938960#comment-7201292

Looks like #1938960: _menu_translate() doesn't care about new routes has been fixed. Does that mean we can remove these hunks?

effulgentsia’s picture

This seems Views-specific. Why is it being added to contextual.module?

Ignore that. I see now that although it's a Views plugin, it's one provided by contextual.module, so that makes sense to be where it is.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
10.21 KB
45.13 KB

The failure in #166 is due to a new test that ensures contextual links for menu blocks are correct: MenuTest::testBlockContextualLinks(). In this reroll, that test is updated in accordance with the rest of the patch.


All points fixed, a few get extra explanation below:

$id includes 2 items, but we're asserting that only 1 got rendered. I suspect it's because the user doesn't have permission to the Block one. If we're intending to test that, then please add a comment saying so. Otherwise, either remove the Block one from $id or add appropriate permissions and assert that it got rendered.

Great point! I just added the 'administer blocks' permission, now they're both there.

Why must it? How about: "Although the empty placeholder is rendered for all users, contextual_page_build() only adds the drupal.contextual-links library for users with 'access contextual links' permission, thus preventing unnecessary HTTP requests for users without that permission."

Excellent suggestion. Included in patch.

What's a "contextual id"? How about: "A string value that can be output to an HTML data attribute."

I personally found "contextual id" a pretty clear name for the serialized result. In any case, I applied all your suggestions around this and replaced "metadata" with your suggestions. Only this particular case I disagree with. I went with the IMHO much clearer "A serialized representation of a #contextual_links property value array for use in a data- attribute."

Let's add a comment here explaining why it's ok that we're bypassing FAPI or any other validation. How about "This is processing a POST request that is not a Drupal form. There is therefore no Form API validation of this value. The rendering logic invoked by the rest of this function must therefore be robust to arbitrary user input."

While the suggested comment makes sense, I think it's entirely pointless to have such a comment. It's abundantly clear already that there is no Form API involved.
I did add a comment to the effect of "must be robust to arbitrary user input" to the function's doxygen though, which is the important part.

Looks like #1938960: _menu_translate() doesn't care about new routes. Does that mean we can remove these hunks?

OMG YAY YES! :) :)

Status: Needs review » Needs work

The last submitted patch, contextual_links_render_cache-914382-170.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
3.18 KB
44.26 KB

Patch failed to apply because I apparently wasn't running latest HEAD.

Two hunks failed:
- the edit.module hunk, which was no longer necessary
- the edit.js hunk, which needed to be modified slightly to apply

In the interdiff:
- a hunk that became unnecessary: the one that makes the drupal.contextual-toolbar library dependent on drupal.contextual-links. That was a fix to not cause a JS error on pages where there are no contextual links, but the real solution is at #1988328: Contextual link JavaScript error when viewing a page such as /admin/* that doesn't contain any contextual links and it's RTBC.
- swapped hardcoded basepaths in the updated tests for base_path() :)

catch’s picture

Looks good to me. Takes the same approach as the inline editing so we get a small amount of markup for all users, but otherwise there's only overhead of checking the links for users with the permission, and fixes the caching issue nicely.

effulgentsia’s picture

I think this is ready from a PHP standpoint. If someone can review the JS portion, then after that, I think this can be RTBC'd.

nod_’s picture

Reviewed the JS, Only issue I might have with it is that the AJAX framework is avoided and a direct calls to $.ajax is made with the callback specified directly and not as an AJAX command. One could argue that you don't need to load the CSS for contextual links until you know you have some on the page, like it's done for edit and ckeditor. But the AJAX framework is not very fun to work with so it's fair.

one minor code thing about views-contextual.js: This is enough:

Drupal.behaviors.viewsContextualLinks = {
  attach: function (context) {
    var id = $('body').attr('data-views-page-contextual-id');

    $('[data-contextual-id="' + id + '"]')
      .closest(':has(.view)')
      .addClass('contextual-region');
  }
};

All in all, I'm ok with it. Feel free to RTBC if the AJAX thing is not an issue.

Wim Leers’s picture

Suggested minor code improvement in views-contextual.js applied.

But the AJAX framework is not very fun to work with so it's fair.

Precisely. There is nothing to gain. Using jQuery.ajax is simpler.

One could argue that you don't need to load the CSS for contextual links until you know you have some on the page, like it's done for edit and ckeditor.

That would indeed be the only solid reason that would justify switching to the AJAX command framework. OTOH, that would cause separate requests for contextual.base.css and contextual.theme.css, which is only 664 bytes when aggregated + minified + gzipped. The overhead of AJAX command framework + extra HTTP requests just make that plain silly. It's very likely better to have contextual links CSS aggregated with the rest of the CSS (i.e. the way it is now).
(When Drupal 8 is getting stable and front-end perf testing shows *then* that this would be better anyway, then I'd be happy to change it of course. Right now it seems extremely unlikely that would be better. Premature optimization.)

nod_’s picture

Status: Needs review » Reviewed & tested by the community

Agreed, thanks :)

alexpott’s picture

Assigned: Wim Leers » catch

Assigning to catch

catch’s picture

Version: 8.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Really nice to see this one green.

Committed/pushed to 8.x!

Bug is still in 7.x although I don't really see this approach being backported unless David thinks we could get away with it, moving there anyway.

Wim Leers’s picture

Issue tags: -sprint

OMG YAY! Best start of a DrupalCon ever! :D

Thanks especially to catch & effulgentsia for all the feedback to get this done!

I agree with catch that it's very unlikely that this is backportable to Drupal 7.

catch’s picture

Assigned: catch » Unassigned
effulgentsia’s picture

Assigned: Unassigned » David_Rothstein
Issue tags: +Needs committer feedback

Seems like we need guidance from David as to what, if anything, he'll accept, per #179.

David_Rothstein’s picture

I thought we had some ideas above of how to fix this relatively simply in Drupal 7? That's why I was skeptical earlier about expanding this issue (rather than following https://drupal.org/node/767608 and creating a new one)... but then again, it's not like the issue was a hotbed of activity for a while before that either.

In the absolute worst case the approach used in the very first patch in this issue (#4) ought to be possible, though hopefully we can fix it more broadly.

@Cottser also wrote a test for all this in #80...

David_Rothstein’s picture

Assigned: David_Rothstein » Unassigned
Status: Patch (to be ported) » Needs review
FileSize
4.45 KB
2.21 KB

Something like this, for example. (I used @Cottser's tests from #80 directly, without modification.)

This is a simple patch which should fix the problem for all blocks as rendered by the Block module. It might not always work with something like Panels, though (which can render blocks in other parts of the page), nor with other render arrays that aren't blocks. Overall, though, it still fixes way more than the original bug report, and to do better I think we'd need something like a generic alter hook that ran for all render arrays.

David_Rothstein’s picture

So, those tests seem to pass/fail as expected. Any reviews?

Short of adding new hooks, we might want to add a hook_block_view_alter() implementation to the patch to do something similar to the hook_page_alter() implementation that's already in there (that would at least catch some cases where the block is being rendered outside of a normal theme region location). It would not catch all contextual links, though (the Block module's own contextual links unfortunately don't seem to get added to the array until after hook_block_view_alter() is called), which is why I didn't use that hook in the patch in the first place. I think the patch might be fine as is though; again, this seems to fix the bug for all blocks and in most practical cases.

David_Rothstein’s picture

Also, taking a look at the committed 8.x patch:

+function contextual_custom_theme() {
+  if (substr(current_path(), 0, 11) === 'contextual/') {
+    return ajax_base_page_theme();
+  }
+}

This still seems problematic to me (and for reasons other than just ugly code) so I filed a patch at #2011998: String-parsing in hook_custom_theme() should be replaced with theme callbacks (for now) to remove this and the other two similar examples currently in Drupal 8.

Above, @effulgentsia suggested that adding a dummy hook_menu() entry to fix this might not work, but I tried it out and it seems to - see my comment and example code in the above-linked issue for more details - so hopefully it's all good to just replace that.

rooby’s picture

I haven't actually done a code review, but the patch in #184 works as expected.

I previously had an unexpected contextual link on the active forum topics block and after applying this patch it has gone away.

It did make it so no one could see the contextual links but after a cache clear users who should be able to see them could see them again.

It doesn't appear to have had any negative effects.

rooby’s picture

Issue summary: View changes

Added a link to #1961884 in Remaining Tasks

Wim Leers’s picture

I don't think anybody wants to work on this in Drupal 7? The last work happened over 6 months ago. I wonder if we shouldn't just close this issue?

xjm’s picture

Issue summary: View changes
Issue tags: +beta blocker

Beta blocker as a blocker for #2151459: Enable node render caching.

webchick’s picture

Issue tags: -beta blocker

This is a 7.x issue, so not a D8 beta blocker.

Wim Leers’s picture

Yep, this issue fixed it in D8, now it's been changed to D7 to also fix it there.

mgifford’s picture

Kisugi Ai’s picture

Hi,
So i have looked at patch 184: contextual-links-render-cache-914382-184.patch.
I have an actual fresh D7.29 Installation and it works not correctly,

if you use this patch the link will disappear after clearing cache.
but if you make new Forum entries they will not display until you clear the cache.
there is the patch in #8 better and works for the forum block.
i use the patch in #8 since D7 have this issue on my production pages.
Now you again. ;)

Kisugi Ai’s picture

okay next problem.
i have test now both #8 an #184 both works for me.
but the forums blocks are hard cached even though i have caching disabled.
is this designed thats these blocks will refresh first with chron?

MXT’s picture

This workaround may help someone:

If you are here because:

  1. you are programmatically creating your custom block,
  2. the link "configure" is appearing on your block for anonymous users

try this to resolve:

add the key user_access('administer blocks') ? 1 : 0 as key in the keys array for your #cache array.

EXAMPLE:

/**
 * Implements hook_block_view().
 */
function yourModule_block_view($delta = '') {
  $block = array();
  switch ($delta) {
    case 'yourModule_block_delta':
      $block['content'] = array(
        '#theme' => 'yourModule_theme',
        '#pre_render' => array('_yourModule_block_pre_render'),
        '#cache' => array(
          'bin' => 'cache_block',
          'expire' => CACHE_TEMPORARY,
          'keys' => array(
            'yourModule_block_delta',
            user_access('administer blocks') ? 1 : 0,
          ),
        ),
      );
      break;
  }
  
  return $block;
}

The above workaround has been extrapolated from this patch: https://www.drupal.org/files/issues/2144035_2.patch from #2144035: a "Configure block" link shows to anonymous users...

Hope this can help someone

MXT

Wim Leers’s picture

Version: 7.x-dev » 8.0.x-dev
Status: Needs review » Closed (fixed)
Issue tags: -Needs backport to D7

Closing because I wrote this ~1.5 years ago in #191:

I don't think anybody wants to work on this in Drupal 7? The last work happened over 6 months ago. I wonder if we shouldn't just close this issue?

droplet’s picture

Version: 8.0.x-dev » 7.x-dev
Status: Closed (fixed) » Needs review

Could we commit patch in #4, at least it helps forum module. I've waited it for 4 years!!

Wim Leers’s picture

Issue tags: +Needs tests

@droplet Could you then please re-upload #4 :) And provide test coverage.

Kisugi Ai’s picture

I'm not sure what happend in D8.
But in D7 the issue was solved in an earlier version, I don't know which version , i think, it was in 7.26 or some else.

David_Rothstein’s picture

The patch in #184 already has tests. It just needs to be reviewed and then hopefully committed. (This issue was not yet fixed in Drupal 7.)

Regarding #193/#194, that's an existing issue unrelated to this patch (see #1241278: "Active forum topics" and "New forum topics" blocks do not show new forum posts until after a cache clear). That issue is currently marked "works as designed" but I am going to reopen it now as it's definitely a legitimate bug. But it occurs either with or without the patch here applied.

Can we get some reviews of #184 and then hopefully get this in?

rooby’s picture

Ooh wow, I would love to get #184 in.

I will review ASAP.

Kisugi Ai’s picture

@David
Strange, because I have used #8 a long time and in every update I tried d7 without this patch. So I can say that's in any of a D7.2x, I don't remember on which (it was above 7.26), version it was solved so I use at the time d7.36 without any patch. I think thats is was made on d7 thats this issue is solved.

David_Rothstein’s picture

I do not see where it would have been fixed. But let me run the tests-only patch again to make sure it is indeed still failing...

Status: Needs review » Needs work

The last submitted patch, 184: contextual-links-render-cache-914382-184-TESTS-ONLY.patch, failed testing.

David_Rothstein’s picture

Status: Needs work » Needs review

That suggests to me that the bug still exists - back to "needs review" since the full patch from #184 is passing tests.

Kisugi Ai’s picture

Okay you have right! gomen
This wil happend only if have some thing changed in the forum.

so i have tried your patch and it works so far.

waht i have made to reproduse this

fresch 7.36 install
add forum module
ad blocks for forum
add an topic
make cron (why ever you need cron)
then it was happent
but after a cache clear it's difficult to reproduce (most i have cleared the cache tables in the DB)
the same procedur i have made, except install and add blocks, after your patch
and it works.

thx.

Under D8 I don't know if the issue exist to.
Greetings

SocialNicheGuru’s picture

When 'render cache' is being referenced is it this module, https://www.drupal.org/project/render_cache?

  • catch committed eea18a4 on 8.3.x
    Issue #914382 by Wim Leers, Cottser, Gábor Hojtsy, franz, droplet, sun,...

  • catch committed eea18a4 on 8.3.x
    Issue #914382 by Wim Leers, Cottser, Gábor Hojtsy, franz, droplet, sun,...
tim.plunkett’s picture

Issue tags: -Blocks-Layouts

Not actively part of the Blocks-Layouts work.

xjm’s picture

Issue tags: -Needs committer feedback

  • catch committed eea18a4 on 8.4.x
    Issue #914382 by Wim Leers, Cottser, Gábor Hojtsy, franz, droplet, sun,...

  • catch committed eea18a4 on 8.4.x
    Issue #914382 by Wim Leers, Cottser, Gábor Hojtsy, franz, droplet, sun,...