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.
Comments
Comment #1
furamag CreditAttribution: furamag commentedI'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:
Per my understanding we should use 'access arguments' for menu item. We need something like this:
But I can see 'Configure block' link even if I'm using 'access arguments'. What I did wrong?
Comment #2
tsvenson CreditAttribution: tsvenson commentedThere 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.
Comment #3
droplet CreditAttribution: droplet commentedat forum.module cached the block forever.....
Comment #4
droplet CreditAttribution: droplet commentedsuper simple solution for it
Comment #5
montesq CreditAttribution: montesq commentedI 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"
Comment #6
ajdeeley CreditAttribution: ajdeeley commentedIt 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
Comment #7
droplet CreditAttribution: droplet commented@ajdeeley,
hoping this patch will commit into Drupal 7.1, then save own time to repatch again & again.
Comment #8
droplet CreditAttribution: droplet commentedPATCH UPDATED. CACHE_TEMPORARY, DRUPAL_CACHE_PER_ROLE are constants :)
don't know why I do that before.
Comment #9
montesq CreditAttribution: montesq commentedI 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...
Comment #10
droplet CreditAttribution: droplet commented@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
Comment #11
kalebheitzman CreditAttribution: kalebheitzman commentedTested 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.
Comment #12
montesq CreditAttribution: montesq commented@droplet: thanks a lot for your accurancy
As kalebheitzman confirms this patch is the good one, we can set the case as RTBC
Comment #13
webchickNice catch.
Is it possible to add a test for this?
Comment #14
montesq CreditAttribution: montesq commentedHow 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...
Comment #15
David_Rothstein CreditAttribution: David_Rothstein commentedThe 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.
Comment #16
droplet CreditAttribution: droplet commentedhmm.. 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)
Comment #17
paul_constantine CreditAttribution: paul_constantine commentedTried 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.
Comment #18
droplet CreditAttribution: droplet commented@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)
Comment #19
paul_constantine CreditAttribution: paul_constantine commentedDid 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.
Comment #20
droplet CreditAttribution: droplet commentedanyone can reproduce the bug ??
@paul_constantine
can you test it with a fresh install ?? thanks.
Comment #21
paul_constantine CreditAttribution: paul_constantine commentedSorry 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
Comment #22
montesq CreditAttribution: montesq commented@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?
Comment #23
paul_constantine CreditAttribution: paul_constantine commentedIf 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).
Comment #24
paul_constantine CreditAttribution: paul_constantine commentedProblem 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.
Comment #25
droplet CreditAttribution: droplet commented@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
Comment #26
paul_constantine CreditAttribution: paul_constantine commentedIt 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 :-)
Comment #27
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedSubscribe.
Comment #28
catchSubscribing. 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).
Comment #29
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedStraight forward D7 Port of droplets patch.
Comment #30
Kisugi Ai CreditAttribution: Kisugi Ai commentedwhy you have this issues still in actual dev release?
every time if i update my drupal release i have to applay this patch.
Comment #31
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedA testcase is missing. After that it should be good to go.
Comment #32
willkaxu CreditAttribution: willkaxu commented#8: 914382-forum.patch queued for re-testing.
Comment #33
NPC CreditAttribution: NPC commentedPatch in #8 solved it for me, thank you very much!
Comment #34
chx CreditAttribution: chx commentedTests incoming this week.
Comment #35
wranvaud CreditAttribution: wranvaud commentedAs 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....
Comment #36
catchThere'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.
Comment #37
franzIs 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
Comment #38
sunAfter 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.
Comment #39
franzShould we change contextual weight?
Comment #40
catchBlock 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.
Comment #41
Kisugi Ai CreditAttribution: Kisugi Ai commentedHi,
patch 914382-contextual_block_cache.patch dont resolve
only 914382-forum.patch will work for me
Comment #42
franzAlyx, 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.
Comment #43
droplet CreditAttribution: droplet commentednot 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
Comment #44
droplet CreditAttribution: droplet commentedRe-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. ^_^)
Comment #45
catchIt'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.
Comment #46
droplet CreditAttribution: droplet commentedI'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 "|=")
No ?? disabled contextual module / disable block caches, same thing still happen to forum block CONTENTS.
Comment #47
catchForum 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.
Comment #48
franzI 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.
Comment #49
franzI'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.
Comment #50
xjmTagging issues not yet using summary template.
Comment #51
Kisugi Ai CreditAttribution: Kisugi Ai commented@franz
jeah i will try later
but i have had no other block problems
Comment #52
HongPong CreditAttribution: HongPong commentedsubscribe
Comment #53
catchSince 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.
Comment #54
sun#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.
Comment #55
sunwhoopsie, forgot one hunk.
Comment #56
franz@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.
Comment #57
catch@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.
Comment #58
AlexWebDesigner CreditAttribution: AlexWebDesigner commentedI confirm! I solved the "configure block" link visible to anonymus user simply with a "clear Cache" ..
Comment #59
David_Rothstein CreditAttribution: David_Rothstein commentedI 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 :)
Comment #60
catch@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).
Comment #61
franz@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...
Comment #62
David_Rothstein CreditAttribution: David_Rothstein commentedYes, 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.
Comment #63
franzOK, so please re-open the issue in #61, it has a specific fix for it.
Comment #64
catchThe 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).
Comment #65
David_Rothstein CreditAttribution: David_Rothstein commented@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.
Comment #66
franzDavid, yes, I forgot there was this parallel patching...
Comment #67
catchMore 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.
Comment #68
moshe weitzman CreditAttribution: moshe weitzman commentedI 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.
Comment #69
snoopy77 CreditAttribution: snoopy77 commentedDroplet (post #4) , thank you. Your simple patch fixed the abnormal behavior on my 7.10 installation. Cheers :-)
Comment #70
moshe weitzman CreditAttribution: moshe weitzman commentedI'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.
Comment #71
catchI 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.
Comment #72
Sms2luv CreditAttribution: Sms2luv commentedHow to apply patch
Comment #73
Niklas Fiekas CreditAttribution: Niklas Fiekas commented@Sms2luv: There's this: http://drupal.org/node/620014.
Comment #74
star-szrIssue summary added.
Comment #75
star-szrOn 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.
Comment #76
Kisugi Ai CreditAttribution: Kisugi Ai commentedin 7.14 to
what ever thez have made
Comment #77
star-szrFrom 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.
Comment #78
star-szrHere's a test case for 8.x, I'll work on backporting this test to 7.x later.
Comment #79
star-szrTo 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.
Comment #80
star-szrHere'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.
Comment #80.0
star-szrAdding issue summary.
Comment #82
star-szrThe 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.
Comment #82.0
star-szrNoting that the issue is fixed in 8.x, updating remaining tasks, other tweaks
Comment #83
droplet CreditAttribution: droplet commentedanyone able to merge fixes and tests patch?
Comment #84
star-szrHere's #55 along with the test in #80. Setting to 7.x temporarily for testbot.
Comment #85
star-szrAnd back to 8.x with an updated version of the test from #78 per http://drupal.org/node/1710766.
Comment #85.0
star-szrLinkifying comment number, adding note about test for 7.x, updating remaining tasks.
Comment #86
dags CreditAttribution: dags commentedComment #87
dags CreditAttribution: dags commentedThe 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?
Comment #88
dags CreditAttribution: dags commentedComment #89
star-szr@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.
Comment #90
star-szr#85: 914382-85.patch queued for re-testing.
Comment #92
star-szrThe test will need to be updated, I'll take a look in the next few days.
Comment #93
star-szrUpdated 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.
Comment #93.0
star-szrUpdating patch links
Comment #94
tim.plunkettContains \Drupal\...
protected function setUp
This can be combined into one line
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).
Comment #95
star-szrThanks for the review, @tim.plunkett! Here's a revised patch with those changes.
Comment #95.0
star-szrUpdated issue summary.
Comment #95.1
star-szrUpdated issue summary.
Comment #96
catchWhy is the test passing?
Comment #97
star-szrThis 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).
Comment #98
Wim Leers#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?
Comment #99
Wim LeersSigh, d.o.
Comment #100
catch@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?
Comment #101
Wim Leers#100: the issue summary of this issue claims so, maybe it's wrong?
Comment #102
star-szrBack 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.
Comment #103
star-szrJust (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.
Comment #105
star-szrI really should have named that with -do-not-test.patch.
Comment #106
catchOK 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.
Comment #107
Wim LeersIndeed, 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.
Comment #108
effulgentsia CreditAttribution: effulgentsia commented@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.
Comment #109
Wim LeersI'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.
Comment #110
effulgentsia CreditAttribution: effulgentsia commentedAdding the caching tag to issues that need to be fixed for blocks to be cacheable (for now with render cache, eventually with *SI).
Comment #111
Wim LeersI likely will be able to start working on this tomorrow.
Comment #112
xjmComment #113
Wim LeersThe 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), andclass="contextual-region"
. If the user has access to contextual links, the JS will be loaded and will fill in thediv[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.
Comment #114
jessebeach CreditAttribution: jessebeach commentedGreat, 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.
This if statement should be on its own line.
Otherwise it looks good.
Comment #116
David_Rothstein CreditAttribution: David_Rothstein commentedAre 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).
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.
Comment #117
moshe weitzman CreditAttribution: moshe weitzman commentedThe 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.
Comment #118
dawehnerThese lines certainly need some documentation :)
Manual testing showed the following problems:
I'm happy to help here, but I'm just wondering first whether your approach supports this in general?
Comment #119
Gábor Hojtsy@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 :)
Comment #120
catch@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.
Comment #121
Wim Leers#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:
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.
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):
data-contextual-id
attribute in the "dynamic HTML"#118:
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.
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 newcontextual
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 useviews_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:
$element['#contextual_links']
Comment #123
Wim LeersOops.
Comment #125
dawehner@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?
Comment #126
Wim LeersStupid, 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.
Comment #127
Wim LeersRegarding the Views field handler plugin
ContextualLinks.php
: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).Comment #128
dawehnerThanks for fixing it!
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
Comment #129
David_Rothstein CreditAttribution: David_Rothstein commentedI 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:
However, on the opposite side I see what looks like it could be a performance regression for the site overall:
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?
No need to parse paths in hook_custom_theme(); just use 'theme callback' in hook_menu() instead.
What happens if another module (rather than Views UI) is attaching contextual links to the view?
Comment #130
Wim Leers#128: Great :)
#129:
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:
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 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).
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.Comment #131
David_Rothstein CreditAttribution: David_Rothstein commentedComment #132
Wim LeersIsn'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"?Comment #133
effulgentsia CreditAttribution: effulgentsia commentedI 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.
Comment #134
sdboyer CreditAttribution: sdboyer commented@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:
this is good, and SCOTCH will be fine with this approach.
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.
and
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.
Comment #135
effulgentsia CreditAttribution: effulgentsia commentedThe 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.Comment #136
Wim LeersWhat'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 :)
Comment #137
Gábor Hojtsy@Wim: the new direction is your proposed patch vs. the issue summary. The issue summary currently says this:
That is clearly not the currently proposed fix.
Comment #138
Wim LeersOh, 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).
Comment #139
catchEnabling 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.
Comment #140
Wim LeersSo, 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?
Comment #141
sdboyer CreditAttribution: sdboyer commentedfirst, 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.
Comment #141.0
sdboyer CreditAttribution: sdboyer commentedUpdated issue summary.
Comment #142
Wim Leers#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.
Comment #143
effulgentsia CreditAttribution: effulgentsia commentedI 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.
Comment #144
effulgentsia CreditAttribution: effulgentsia commentedThis also needs a reroll due to #1914966-15: Nested contextual links triggers don't work well having landed.
Comment #144.0
effulgentsia CreditAttribution: effulgentsia commentedRevamped.
Comment #145
effulgentsia CreditAttribution: effulgentsia commentedThanks. Looks good.
I opened #1961884: Add support for 'permissions' and/or 'contexts' to render cache system and linked it from the summary.
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:
ab -c1 -n100
for the anonymous user came in at a median time of 493ms.Ok, that works for me. For reference, here are the divs that were added in my test.
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.
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!
Comment #146
Berdir@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.
Comment #147
Wim Leers#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:
Well… that's what I meant by that of course :) Intentionally spend more time, so that the amortized result is better.
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.
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'
; ifcontextual.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" :)
Comment #149
Wim LeersI 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
still causes these users to have the 'administer views' permission.
It seems this is a regression in
WebTestBase
, but that seems so very unlikely…Comment #150
Gábor HojtsyPatch 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.
Comment #152
Gábor HojtsyFor 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...
Comment #153
Gábor HojtsyThat was the wrong interdiff.
Comment #155
Wim LeersFound the cause! Views UI uses routes instead of
hook_menu()
for access checking. Butmenu_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.Comment #156
Wim Leers#1971108: Convert contextual.js to use Backbone (and support dynamic contextual links) landed first, #155 will have to be rerolled now.
Comment #157
effulgentsia CreditAttribution: effulgentsia commentedWhile 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.
Comment #158
franzAddressing #157
Comment #160
Wim LeersRerolled 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
andcore/modules/contextual/contextual.toolbar.js
. Thanks to the work that's already been done in other issues, this patch has become simpler.Comment #161
Wim Leers#1981760: Edit should use the new drupalContextualLinkAdded event to cleanly instead of hackily insert its "Quick edit" contextual link got committed, this is now unblocked.
Comment #162
Wim LeersLet's see if #160 still applies.
Comment #163
Wim Leers#160: contextual_links_render_cache-914382-160.patch queued for re-testing.
Comment #165
Wim LeersThe context of two hunks had changed.
Comment #166
effulgentsia CreditAttribution: effulgentsia commentedRerolled for HEAD changes.
Comment #168
effulgentsia CreditAttribution: effulgentsia commented$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.
Please add a link to #1954892: Replace 'theme callback' and hook_custom_theme() with a clean theme negotiation system.
contextual_page_build().
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."
This seems Views-specific. Why is it being added to contextual.module?
Let's avoid the word "metadata" here. How about "Serializes a #contextual_links property value array to a string."
I also wonder whether "metadata" is a poor term here as well. Perhaps "options" instead?
What's a "contextual id"? How about: "A string value that can be output to an HTML data attribute."
Unserializes the result of _contextual_links_to_id().
The class doesn't use this.
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."
Looks like #1938960: _menu_translate() doesn't care about new routes has been fixed. Does that mean we can remove these hunks?
Comment #169
effulgentsia CreditAttribution: effulgentsia commentedIgnore 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.
Comment #170
Wim LeersThe 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:
Great point! I just added the
'administer blocks'
permission, now they're both there.Excellent suggestion. Included in patch.
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."
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.
OMG YAY YES! :) :)
Comment #172
Wim LeersPatch 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()
:)Comment #173
catchLooks 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.
Comment #174
effulgentsia CreditAttribution: effulgentsia commentedI 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.
Comment #175
nod_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:
All in all, I'm ok with it. Feel free to RTBC if the AJAX thing is not an issue.
Comment #176
Wim LeersSuggested minor code improvement in
views-contextual.js
applied.Precisely. There is nothing to gain. Using
jQuery.ajax
is simpler.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
andcontextual.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.)
Comment #177
nod_Agreed, thanks :)
Comment #178
alexpottAssigning to catch
Comment #179
catchReally 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.
Comment #180
Wim LeersOMG 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.
Comment #181
catchComment #182
effulgentsia CreditAttribution: effulgentsia commentedSeems like we need guidance from David as to what, if anything, he'll accept, per #179.
Comment #183
David_Rothstein CreditAttribution: David_Rothstein commentedI 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...
Comment #184
David_Rothstein CreditAttribution: David_Rothstein commentedSomething 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.
Comment #185
David_Rothstein CreditAttribution: David_Rothstein commentedSo, 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.
Comment #186
David_Rothstein CreditAttribution: David_Rothstein commentedAlso, taking a look at the committed 8.x patch:
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.
Comment #187
rooby CreditAttribution: rooby commentedI 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.
Comment #187.0
rooby CreditAttribution: rooby commentedAdded a link to #1961884 in Remaining Tasks
Comment #188
Wim LeersI 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?
Comment #189
xjmBeta blocker as a blocker for #2151459: Enable node render caching.
Comment #190
webchickThis is a 7.x issue, so not a D8 beta blocker.
Comment #191
Wim LeersYep, this issue fixed it in D8, now it's been changed to D7 to also fix it there.
Comment #192
mgifford184: contextual-links-render-cache-914382-184.patch queued for re-testing.
Comment #193
Kisugi Ai CreditAttribution: Kisugi Ai commentedHi,
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. ;)
Comment #194
Kisugi Ai CreditAttribution: Kisugi Ai commentedokay 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?
Comment #195
MXTThis workaround may help someone:
If you are here because:
try this to resolve:
add the key
user_access('administer blocks') ? 1 : 0
as key in the keys array for your #cache array.EXAMPLE:
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
Comment #196
Wim LeersClosing because I wrote this ~1.5 years ago in #191:
Comment #197
droplet CreditAttribution: droplet commentedCould we commit patch in #4, at least it helps forum module. I've waited it for 4 years!!
Comment #198
Wim Leers@droplet Could you then please re-upload #4 :) And provide test coverage.
Comment #199
Kisugi Ai CreditAttribution: Kisugi Ai commentedI'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.
Comment #200
David_Rothstein CreditAttribution: David_Rothstein commentedThe 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?
Comment #202
rooby CreditAttribution: rooby commentedOoh wow, I would love to get #184 in.
I will review ASAP.
Comment #203
Kisugi Ai CreditAttribution: Kisugi Ai commented@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.
Comment #204
David_Rothstein CreditAttribution: David_Rothstein commentedI 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...
Comment #207
David_Rothstein CreditAttribution: David_Rothstein commentedThat suggests to me that the bug still exists - back to "needs review" since the full patch from #184 is passing tests.
Comment #208
Kisugi Ai CreditAttribution: Kisugi Ai commentedOkay 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
Comment #209
SocialNicheGuru CreditAttribution: SocialNicheGuru commentedWhen 'render cache' is being referenced is it this module, https://www.drupal.org/project/render_cache?
Comment #212
tim.plunkettNot actively part of the Blocks-Layouts work.
Comment #213
xjm