Updated: Comment #65

Problem/Motivation

Drupal 8 will ship with far more JavaScript. Drupal 8 will need to perform well on mobile.

This means we must allow JavaScript code to apply smart client-side caching strategies in order to provide a fast experience. Two of the most obvious things to cache are: 1) user-specific data, 2) permission-dependent data.

Currently, Drupal 8 performs 2 additional HTTP requests per page load that hit Drupal:

  • 1 HTTP request/page load for rendering contextual links
  • 1 HTTP request/page load for retrieving in-place editing metadata

(Assuming the user has permission to both use contextual links and in-place editing, of course.)

Both of these HTTP requests are necessary because the necessary metadata cannot be embedded in the page itself, because that would break render caching.

However, they result in:

  1. worse front-end performance
  2. more networking activity (hence reduced battery life)
  3. worse site scalability (because they each require Drupal to bootstrap)

#2136507: Use client-side cache tags & caching to eliminate 1 HTTP requests/page for rendering Contextual Links gets rid of the 1 HTTP request/page load for rendering contextual links. This issue gets rid of the one for retrieving in-place editing metadata.

Proposed resolution

The solution is to cache these responses in the client-side cache, for the duration of the session (sessionStorage, "session" meaning "until the tab gets closed"). That would mean there's a one-time-per-session cost (i.e. those 2 HTTP requests). That means: better front-end performance, less networking activity, longer battery life, better site scalability.

#1991684: Node history markers (comment & node "new" indicator, "x new comments" links) forces render caching to be per user introduced drupalSettings.user.uid — that allows client-side caching on a per-user basis.

However, to be able to get rid of these two HTTP requests per page load (and use client-side caching instead), that is not enough: the data in the responses to those 2 HTTP requests depend on permissions; so if permissions change, the client-side cache should be invalidated as well.
Allowing JS to have access to the precise list of permissions might be problematic, especially when combining that data with Drupal SAs (security announcements) which often mention specific permissions. However, what really matters is to know whether the permissions have changed at all for a given user, which would force us to invalidate whatever has been cached client-side. So, a hash of the hash salt plus the serialized list of permissions is a secure, yet sufficiently informative identifier — drupalSettings.user.permissionsHash.

Remaining tasks

None.

User interface changes

None.

API changes

  • API addition: drupalSettings.user.permissionsHash, provided on any page where at least some JS depends on drupalSettings.
  • API change: AccountInterface has a new getPermissionsHash() method, which is implemented by \Drupal\user\Entity\User and \Drupal\Core\Session\UserSession. (Which is better than a new user_permissions_get_hash() procedural function, which is what I had first.)
  • API change: A permissions cache tag containing role IDs is introduced.
CommentFileSizeAuthor
#119 interdiff.txt1.13 KBWim Leers
#119 client_side_caching_edit-2005644-118.patch35.07 KBWim Leers
#118 2005644-118-client_side_caching.patch33.58 KBAnonymous (not verified)
#114 client_side_caching_edit-2005644-114.patch34.91 KBWim Leers
#111 interdiff.txt2.1 KBWim Leers
#111 client_side_caching_edit-2005644-111.patch34.76 KBWim Leers
#108 interdiff.txt3.48 KBWim Leers
#108 client_side_caching_edit-2005644-108.patch34.54 KBWim Leers
#104 interdiff-2005644-104.txt10.74 KBdamiankloip
#104 2005644-104.patch31.41 KBdamiankloip
#101 interdiff-2005644-101.txt4.22 KBdamiankloip
#101 2005644-101.patch20.67 KBdamiankloip
#100 interdiff.txt3 KBWim Leers
#100 client_side_caching_edit-2005644-100.patch20.69 KBWim Leers
#98 interdiff-2005644-98.txt1.45 KBdamiankloip
#98 2005644-98.patch18.87 KBdamiankloip
#96 interdiff.txt4.89 KBWim Leers
#96 client_side_caching_edit-2005644-96.patch19.25 KBWim Leers
#93 interdiff.txt16.61 KBWim Leers
#93 client_side_caching_edit-2005644-93.patch18.92 KBWim Leers
#90 interdiff.txt1.78 KBWim Leers
#90 client_side_caching_edit-2005644-90.patch14.33 KBWim Leers
#87 interdiff.txt1.4 KBWim Leers
#87 client_side_caching_edit-2005644-87.patch14.3 KBWim Leers
#83 interdiff.txt10.88 KBWim Leers
#83 client_side_caching_contextual_edit-2005644-83.patch14.28 KBWim Leers
#79 interdiff.txt2.75 KBWim Leers
#79 client_side_caching_contextual_edit-2005644-79.patch25.18 KBWim Leers
#78 interdiff.txt4 KBWim Leers
#78 client_side_caching_contextual_edit-2005644-78.patch25.21 KBWim Leers
#71 interdiff-moshe.txt3.56 KBWim Leers
#71 interdiff-catch.txt1.25 KBWim Leers
#71 client_side_caching_contextual_edit-2005644-71.patch25.21 KBWim Leers
#66 interdiff.txt1.11 KBWim Leers
#66 client_side_caching_contextual_edit-2005644-66.patch25.65 KBWim Leers
#65 client_side_caching_contextual_edit-2005644-65.patch26.7 KBWim Leers
#65 interdiff.txt1.09 KBWim Leers
#63 interdiff.txt1.17 KBWim Leers
#63 client_side_caching_contextual_edit-2005644-63.patch26.54 KBWim Leers
#61 interdiff.txt11.7 KBWim Leers
#61 client_side_caching_contextual_edit-2005644-61.patch25.81 KBWim Leers
#58 interdiff.txt5.98 KBWim Leers
#58 client_side_caching_contextual_edit-2005644-58.patch21.7 KBWim Leers
#56 client_side_caching_contextual_edit-2005644-56.patch16.04 KBWim Leers
#51 client_side_caching_contextual_edit-2005644-51.patch15.94 KBWim Leers
#49 client_side_caching_contextual_edit-2005644-49.patch16 KBWim Leers
#49 interdiff.txt9.45 KBWim Leers
#42 js_caching_metadata-2005644-42.patch7.59 KBamateescu
#39 js_caching_metadata-2005644-39.patch6.98 KBWim Leers
#39 interdiff.txt1017 bytesWim Leers
#35 js_caching_metadata-2005644-35.patch6.9 KBWim Leers
#35 interdiff.txt1.26 KBWim Leers
#28 js_caching_metadata-2005644-28.patch6.58 KBWim Leers
#28 interdiff.txt855 bytesWim Leers
#26 js_caching_metadata-2005644-26.patch6.66 KBWim Leers
#26 interdiff.txt5.47 KBWim Leers
#19 interdiff.txt1.36 KBWim Leers
#18 js_caching_metadata-2005644-18.patch2.95 KBWim Leers
#6 js_caching_metadata-2005644-6.patch2.96 KBWim Leers
#1 js_caching_metadata-2005644-1-cookie.patch3.08 KBWim Leers
#1 js_caching_metadata-2005644-1-drupalSettings.patch2.07 KBWim Leers
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Wim Leers’s picture

I tried it first with a cookie approach, then realized the drupalSettings approach could be much, much simpler.

nod_’s picture

drupalSettings++

Not sure where the hash generation and caching thing should happen so i'll leave the RTBC to someone else, that looks good to me.

But i'm wondering about caching. We bent backwards to make contextual links and edit module not break page cache but putting the user id in the page (since drupalSettings ends up inline in the HTML code) makes caching not very possible no? or were contextual and edit all about another caching layer?

Wim Leers’s picture

The changes in contextual.module and edit.module were all about not breaking the render cache, not the page cache. Those changes make it *cheaper* to render user-specific pages.

Plus, many drupalSettings are potentially user-specific. They're one of the things that would pretty much always need to be user-specific.
You're right though that in that sense it is seemingly somewhat easier to go with the cookies approach.

nod_’s picture

That makes sense, thanks for the infos about render cache.

We killed a bunch of cookies already, i'd rather avoid adding on when drupalSettings works. All good on my end.

Status: Needs review » Needs work

The last submitted patch, js_caching_metadata-2005644-1-drupalSettings.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
2.96 KB

It seems ThemeInfoStylesTest was rather poorly written. It fails whenever drupalSettings exist, even though that's not what it's testing.

Status: Needs review » Needs work
Issue tags: -WPO, -sprint, -in-place editing, -Spark

The last submitted patch, js_caching_metadata-2005644-6.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
Issue tags: +WPO, +sprint, +in-place editing, +Spark

#6: js_caching_metadata-2005644-6.patch queued for re-testing.

nod_’s picture

Status: Needs review » Reviewed & tested by the community

That works :)

amateescu’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: +Security

I think we should at least give the security team a chance to look at this..

Gábor Hojtsy’s picture

+++ b/core/modules/system/lib/Drupal/system/Tests/Theme/ThemeInfoStylesTest.phpundefined
@@ -46,6 +46,9 @@ function testStylesheets() {
+    // We should ignore any data in drupalSettings.
+    $content = $this->drupalGetContent();
+    $this->drupalSetContent(substr($content, 0, strpos($content, '<!--//--><![CDATA[//><!--')));

Is this a reliable pattern for drupalSettings? Are there no other parts that might match this pattern? CDATA is a pretty general container.

moshe weitzman’s picture

Status: Needs review » Needs work
Issue tags: -Security

The concept looks good here. I also reviewed this from a security perspective and we are fine (I'm on the Security Team).

This is only being done on the user profile page (user_page_build)? Don't we need this info all pages?

I notice that you do invalidation in the form submit handler. It would be ideal to do this in the role+perm API. Problem is, I see 3 functions which do similar things (see below). Ideally we refactor so they all call one function and put invalidation there.

http://api.drupal.org/api/drupal/core%21modules%21user%21user.module/fun...
http://api.drupal.org/api/drupal/core%21modules%21user%21user.module/fun...
http://api.drupal.org/api/drupal/core%21modules%21user%21user.module/fun...

nod_’s picture

hook_page_build() :)

Wim Leers’s picture

Status: Needs work » Needs review

user_page_build() is a hook_page_build() implementation, hence it appears on all pages.

I agree that ideally it would not live in a form submit handler, but in the API itself. However, rewriting half of User module is not really within scope. As you can see, I'm only adding a new invalidation tag; the invalidation code already existed there. Hence I don't think that should block this issue.

Gábor Hojtsy’s picture

Issue tags: +Security

In terms of the roles hash I don't see a security issue there but will post this to the security team too if they care to review.

Wim Leers’s picture

#15: it's a permissions hash, not roles hash. Also, isn't #12 enough to confirm there's no security problem? :)

Wim Leers’s picture

Status: Needs review » Needs work

moshe clarified #12 in chat. I'll do that.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
2.95 KB

#12 implemented. Tested manually; still works perfectly.

Wim Leers’s picture

FileSize
1.36 KB

File upload crashed earlier, here's the interdiff for #18.

moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community

Thanks for more robust invalidation. This ready for commit.

Dave Reid’s picture

I would think it would be more useful to have the timestamp of when the user was last saved/edited. That way I could clear caches based on other things changing when permissions do not change: field values, etc.

moshe weitzman’s picture

@dave - I think thats a separate feature request. There is no harm in adding multiple bits into the Settings array.

David_Rothstein’s picture

Status: Reviewed & tested by the community » Needs work

Shouldn't there be tests? (The issue summary says so.)

+    $permissions_hash = hash('sha256', drupal_get_hash_salt() . serialize(user_role_permissions($user->roles)));

This isn't a very secure hash, since it's only secured by the salt which is available in the file system/codebase. Normal practice when generating a secure hash is to add drupal_get_private_key() in there also (since that's stored in a separate location, usually the database).

Dave Reid’s picture

So we either add a timestamp of around 10 characters, or a 64 character hash that has to be calculated on every request. It feels like it would just be easier to switch that out now and save 54 characters on every single request with JS?

Wim Leers’s picture

Issue tags: +Needs tests

David: I tried to find best practices for this, but couldn't; things are done differently in various places in core.

Dave: the hash is not calculated for every request. It's only calculated once for each combination of user roles. It's only recalculated if any of the permissions for any of the roles change.
I would like the hash to be shorter though. Suggestions for that?

Wim Leers’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
5.47 KB
6.66 KB

Test coverage added.

This identified a bug: $user->roles is an unsorted array. This resulted in multiple cache entries per role, hence causing problems. Yay tests :)

Wim Leers’s picture

Status: Needs review » Needs work

D'oh, I forgot to do #23.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
855 bytes
6.58 KB

Addresses #23.

nod_’s picture

Status: Needs review » Reviewed & tested by the community

works for me :)

YesCT’s picture

Issue tags: +RTBC July 1

This issue was RTBC and passing tests on July 1, the beginning of API freeze.

Dries’s picture

Status: Reviewed & tested by the community » Needs review

I'm worried that with this patch, we'll do a cache_get() on every page request as well as fairly expensive computations. That may be more expensive than the performance gains ... It would be good to benchmark this.

It may be better to have a "global hash" that is identical for all users. The global hash would be updated when any configuration change is made, or maybe when a permission is changed. This would mean we use global cache invalidation instead of partial cache invalidation. The benefit would be that the hash calculations could be replaced by much faster hashes like a "last updated time" or even a random number. More importantly, we could (more easily) avoid expensive I/O operations like cache_get()s on every request.

The same global configuration hash could also be reused by other parts of Drupal. It could mark a fundamental paradigm change in how we do cache invalidation in Drupal.

effulgentsia’s picture

Issue tags: -RTBC July 1

There's nothing in the latest patch that breaks BC, so this is still eligible post-July-1. Removing the tag.

Wim Leers’s picture

#31: What you describe is a good additional measure, Dries, but it solves a different problem. The hash used here is able to uniquely identify user and role, so that you can cache user- and role-specific data on the client side. With just a "global config hash", that is not possible. It would be possible to clear caches whenever any setting changes, which is good. But the hash would remain the same for all users, hence preventing client-side caching of user-specific data.

To address your concerns regarding performance impact: we could alternatively create a cookie whenever a new session is started (this was one of the two approaches in #1). Secondly, there are *no* expensive computations whatsoever.

So:
- I'll run benchmarks tomorrow.
- What you propose is indeed one of the next logical steps, but for it to work reliably *every* setting must be in the config system. It should be a separate issue because it is so much broader. (Also: I do see the potential for big performance problems there, because it implies calculating a hash of all data in the config system…!)

moshe weitzman’s picture

@Wim - Dries' idea is to have a UUID that gets regenerated on every config change or drupal_flush_all_caches(). It can be generated by uuid API so is not a hash of anything. I think that removes performance concerns. I'm not sure if such a cache key would be useful the js caching you have in mind. One of our challenges in reviewing this issue is that we didn't know where the proposed hash gets used.

Wim Leers’s picture

Benchmarks

Benchmarks (using $start = microtime(TRUE); <calculations here> $end = microtime(TRUE); var_dump($end - $start);) show that:

  • cache hits: <= 1 ms (order of magnitude: 1 ms)
  • cache misses: <= 9 ms (order of magnitude: 10 ms)

Conclusion: 1 ms amortized per page load.

Rerolled patch: improved performance

Potential performance improvements:

  • Entire pages are cached for anonymous users, so we only need to consider performance improvements for authenticated users.
  • Authenticated users use sessions, we could cache the user permissions hash in the session. This implies that permission changes while the user is logged in won't be reflected until (s)he is logged out, i.e. until the session is closed.

Updated benchmark results

I implemented this, and then the cost is <= 0.1 ms (order of magnitude: 0.1 ms). Often it is undetectable by microtime(TRUE);.

Conclusion: 0.1 ms amortized per page load.

Use cases

From #1605290-73: Enable entity render caching with cache tag support:

[This issue] is essential for client-side caching of these AJAX-retrieved, user- or role-specific pieces of content. Otherwise, these have to be retrieved every single time, as is the case right now in Drupal core (i.e. for rendering contextual links and in-place editing metadata).

Why is no use case implemented in this patch?

In a nutshell: because Drupal is too flexible. Drupal allows for dynamic access checks, like "disallow perm X for users of role Y every day after 17:00 UTC", or "allow perm A for all users of any role on January first of every year". Basically, anything is possible: access checks might be influenced by context like time, location, IP address, workflow state, language and so on.
If all access checks were guaranteed to be solely permission-based, then there would be predictability, but alas, we have no way of knowing that all access checks are solely permission-based, and not dependent on some context.

The two use cases I cited above are client-side caching of 1) rendered contextual links, 2) in-place editing metadata. For the latter, I've already implemented a Drupal 8 contrib module that does precisely this: https://drupal.org/project/edit_metadata_cache. There I provide a very sensible, not at all far-fetched example:

If a journalist creates a story, then edits the title in-place, the metadata for in-place editing will have been cached in the journalist's browser. But if the journalist then changes the workflow state to "needs review" so that the editor-in-chief might approve it, then the journalist is no longer allowed to edit it. However, the cached in-place editing metadata in the journalist's browser will not be cleared, since the journalist still has the same set of permissions; it's not the roles/permissions system that determines whether the journalist may edit the story, it's the workflow state.

The majority of sites would be able to install https://drupal.org/project/edit_metadata_cache and save 1 HTTP request for every page load. That also means one less Drupal bootstrap.
We can apply the exact same technique to contextual.module, where we can again save 1 HTTP request for every page load, and save another Drupal bootstrap.


More generally…

It is possible/likely that we may want to add additional metadata to drupalSettings later on to facilitate client-side caching. E.g.:

  • the interface language (a client-side cached rendered snippet of HTML often contains a specific interface language)
  • the last translation change
  • the theme
  • the "global configuration hash" Dries referred to in #31

This really boils down to "everything that is expensive to calculate should have cache tags". We can then cache the results of those calculations forever, until >=1 cache tags change.

But we've of course already been working on this for a long time in Drupal 8: #636454: Cache tag support & #1605290: Enable entity render caching with cache tag support. Finishing this will probably be essential to make Drupal 8's performance good.

So this is definitely true:

It could mark a fundamental paradigm change in how we do cache invalidation in Drupal.

… except that it's not really something new, it's just something we haven't managed yet to permeate all across Drupal 8 core.

The problem is that it's not nearly as powerful and useful as it can be until it is an inherent part of every API that should have it. Ideally, cache tags would be used everywhere in Drupal's PHP APIs, and the necessary metadata would be passed on to the client-side automatically, so that we could do as much client-side caching as possible.

This is crucial for effective ESI. It is how Facebook manages to stay online at all. It may be crucial for Drupal 8. It will be a top priority for Drupal 9 if it's not finished in Drupal 8.

A man can dream :)

Status: Needs review » Needs work

The last submitted patch, js_caching_metadata-2005644-35.patch, failed testing.

moshe weitzman’s picture

Nice reply! Failed testing :(

Wim Leers’s picture

Yes, and I'd worked on fixing the tests for many hours of my own time, but couldn't get it to work. Weird, subtle things going on. I'll post what I had.

Wim Leers’s picture

Assigned: Wim Leers » Unassigned
Status: Needs work » Needs review
FileSize
1017 bytes
6.98 KB

This fixes the five failures in HttpBasicTest. I did not find how to fix the three failures in SessionTest yet, and I'd appreciate it if somebody else could push this forward in the mean time! (I'll be AFK for the next 7 days.)

Built on 47cdba9853bbe9a2445ce86f71c94439962b7e04.

Status: Needs review » Needs work

The last submitted patch, js_caching_metadata-2005644-39.patch, failed testing.

Wim Leers’s picture

It should just needs reroll! :)

amateescu’s picture

Status: Needs work » Needs review
FileSize
7.59 KB

Rerolled.

Status: Needs review » Needs work

The last submitted patch, js_caching_metadata-2005644-42.patch, failed testing.

jessebeach’s picture

Would a hash the rolls up the Role's permission profile be sufficient? We should need a per-user cache.

jessebeach’s picture

Wim Leers just mentioned to me in chat that " the menu entity cache is only invalidated if anything in the menu itself changes". Changes to the permissions of a role would not invalidate a menu, so we really do need a hash of the permission profile of a user in order to determine if we need to re-request any data that we've cached in localStorage on the client. Please ignore comment #44.

jessebeach’s picture

Actually, changes to a user's permission set (essentially a change to the permission set of a role, since no permission exists outside of a role) will not invalidate a menu's cache either, so caching at the role level or the user level should be functionally equivalent. There will almost always be a significant order of magnitude lower number of roles than users on a site. When permissions of a role change, we can invalidate the cache of this hash. Hashing either a user's permissions or the permission of the roles associated with the user seem analogous. Maybe it doesn't matter in the end. I'm just tossing out some thoughts.

Wim Leers’s picture

#1991684: Node history markers (comment & node "new" indicator, "x new comments" links) forces render caching to be per user got committed, which already includes the user ID. So now we can scope this down to just the permissions hash.

Wim Leers’s picture

Title: Provide the user ID and permissions hash to allow JavaScript to maintain client-side caches » Provide a permissions hash to allow JavaScript to maintain client-side caches

.

Wim Leers’s picture

Title: Provide a permissions hash to allow JavaScript to maintain client-side caches » Use client-side caching to eliminate 2 HTTP requests/page (Edit + Contextual), introduce drupalSettings.user.permissionsHash
Assigned: Unassigned » Wim Leers
Category: feature » task
FileSize
9.45 KB
16 KB

#1927174: Complete the work to make the Toolbar administration menu sub tree rendering more efficient made me realize that my comment in #35 is exaggerated:

In a nutshell: because Drupal is too flexible. Drupal allows for dynamic access checks, like "disallow perm X for users of role Y every day after 17:00 UTC", or "allow perm A for all users of any role on January first of every year". Basically, anything is possible: access checks might be influenced by context like time, location, IP address, workflow state, language and so on.
If all access checks were guaranteed to be solely permission-based, then there would be predictability, but alas, we have no way of knowing that all access checks are solely permission-based, and not dependent on some context.

While that is theoretically possible, the reality is that we've made such assumptions before in Drupal core: for example in the Toolbar module's toolbar/subtrees/* route as it currently (bbb106da758651e4d948545bf40873b011397206) lives in HEAD. That is, server-side caching of permission-dependent data has also been assuming predictability. Without some assumed predictability, we can't cache anything, ever. So, if server-side caching can assume predictability, then so can client-side caching.


In this reroll, I'm leveraging drupalSettings.user.permissionsHash to get rid of:

  1. 1 HTTP request/page load for rendering contextual links; they're now cached in sessionStorage
  2. 1 HTTP request/page load for retrieving Edit metadata; they're now cached in sessionStorage

In other words: it's a big front-end performance win! It also results in better scalability for Drupal sites because they'll have to answer less requests.

This is a rough, initial patch to get this moving forward again; I did not work on getting the tests passing.

Wim Leers’s picture

Issue tags: -sprint

Will be on my plate in 1 week or so hopefully.

Wim Leers’s picture

Status: Needs work » Needs review
Issue tags: +sprint
FileSize
15.94 KB

drupalSettings.user.permissionsHash is a blocker for #2090783: Run comment op links (delete, edit, reply, approve + contrib) through #post_render_cache to prevent render caching granularity being per-user, which means this is a blocker for node render caching.


Reroll. Let's see how many tests fail.

Status: Needs review » Needs work

The last submitted patch, client_side_caching_contextual_edit-2005644-51.patch, failed testing.

Wim Leers’s picture

Correction, this is not a blocker for #2090783: Run comment op links (delete, edit, reply, approve + contrib) through #post_render_cache to prevent render caching granularity being per-user after all. Working on that issue and its actual blockers first. Will get back to this one.

Wim Leers’s picture

Some might say that #51 should be rerolled to leverage #2118703: Introduce #post_render_cache callback to allow for personalization without breaking the render cache. But that's not true. Edit + Contextual appear on pretty much all pages. Hence the cost of retrieving the data from the server once is inherently amortized significantly.

Conclusion: the concept of this patch is solid, I just need to work on getting it to pass all tests.

Wim Leers’s picture

Issue tags: +Performance

Adding "Performance" tag for tracking purposes.

Wim Leers’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
16.04 KB

Straight reroll of #51; tracking HEAD. Next step: make this pass tests.

Status: Needs review » Needs work

The last submitted patch, 56: client_side_caching_contextual_edit-2005644-56.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
21.7 KB
5.98 KB

Tests pass locally, including those of SessionTest (which I spent many hours trying to get to pass months ago, without success), so yay! :)

Through manual testing, I confirmed that it's working perfectly for contextual links. I found a race condition for in-place editing, which has been fixed in this reroll.

Status: Needs review » Needs work

The last submitted patch, 58: client_side_caching_contextual_edit-2005644-58.patch, failed testing.

Wim Leers’s picture

Wim Leers’s picture

I think it's in line with the spirit of D8 to not add a new procedural function to user.module, but to extend AccountInterface instead. So that's what I do in this reroll.

Also: when the URL a contextual link links to already contains a ?, this updated patch then concatenates the destination parameter using & instead of ?.

Status: Needs review » Needs work

The last submitted patch, 61: client_side_caching_contextual_edit-2005644-61.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
26.54 KB
1.17 KB

Status: Needs review » Needs work

The last submitted patch, 63: client_side_caching_contextual_edit-2005644-63.patch, failed testing.

Wim Leers’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
1.09 KB
26.7 KB

This should do the trick at last.

Wim Leers’s picture

Title: Use client-side caching to eliminate 2 HTTP requests/page (Edit + Contextual), introduce drupalSettings.user.permissionsHash » Use client-side cache tags & caching to eliminate 2 HTTP requests/page (Edit + Contextual), introduce drupalSettings.user.permissionsHash
Issue summary: View changes
Issue tags: +JavaScript
Related issues: +#1872264: Minimize metadata HTTP requests triggered by Edit's JS, +#1991684: Node history markers (comment & node "new" indicator, "x new comments" links) forces render caching to be per user
FileSize
25.65 KB
1.11 KB

Complete makeover of the hopelessly outdated issue summary.

Rerolled to track HEAD and to move some unrelated changes (see interdiff) into its own patch (see #2135845: UserInterface duplicates some methods of AccountInterface).

Wim Leers’s picture

Also see #607244-79: Decrease performance impact of contextual links — I think committing this patch will allow us to immediately close that issue as well! :)

Wim Leers’s picture

catch’s picture

Couple of questions:

With the caching of contextual links, let's say the following happens:

1. A module adds a contextual link to nodes for 'add to $nodequeue', when the node is in that queue, the link changes to 'remove from $nodequeue'.

2. User A gets the HTML for those contextual links cached.

3. User B adds the node to the nodequeue.

4. Does User A now get stale HTML or not?

If the answer is they don't, then we might need some clarification about what the cached HTML is - this is a question of access checks vs. some other change in the contextual links that depends on the node object itself.

+++ b/core/lib/Drupal/Core/Session/UserSession.php
@@ -142,6 +144,34 @@ public function hasPermission($permission) {
+    if (function_exists('drupal_session_started') && drupal_session_started() && isset($_SESSION['user_permissions_hash'])) {

This gets the permission hash before checking the user's roles.

So if an admin updates the roles for a user, I think you could end up with a stale permissions hash since after that's happened, while the cache entry is cleared via tags, the session won't be updated.

If we first check the roles, then the permissions hash based on that, it might be OK.

moshe weitzman’s picture

Very nice patch and issue summary. Thanks. Catch gave a great tech review, so I'll just nitpick. My aplogies.

dreditor won't paste my review so here goes ...

if (function_exists('drupal_session_started') && drupal_session_started() && isset($_SESSION['user_permissions_hash'])) {

Is this defensiveness needed? Why not just have the isset() check? Same later instance of similar code in same function.

$serialized_roles = implode(',', $sorted_roles);

I would call this $role_list so we don't confuse it with serialize().

Lastly, cache_invalidate_tags() has been deprecated in favor of OO cache api.

Wim Leers’s picture

#69:

RE: dynamic contextual link
Very interesting question! Answer in three parts:
  1. In the current implementation, user A would get stale HTML.
  2. There's only one dynamic contextual link in Drupal core, and that's Edit.module's. It's done in JS. It needs to be even more dynamic than your example though, because it depends on what the user is doing on the current page.
  3. I don't think we've ever had a dynamic contextual link like the one you describe, in Drupal core or contrib? Though I think that would've been theoretically possible thanks to hook_menu()'s title callback, was it used in practice?
RE: the $_SESSION-cached permissions hash
The only reason this exists is because of performance concerns voiced earlier, it reduces the cost of retrieving the permissions hash from <= 1ms to <= 0.1 ms (see #35). You're right about the potential for that going stale, but there's more than one way that can happen: 1) if an admin changes a user's roles, as you say, 2) if an admin changes the permissions within a role.
I'd prefer to get rid of the "cache in $_SESSION" thing because that really just feels like a very bad precedent (imagine if every module would do this). Because the problem you point out essentially boils down to replicating cache tag invalidation behavior outside of the caching system… and that's not acceptable, IMO — it's not worth the benefit.
Hence: removed from patch.

#70:

  1. See my second point in the above.
  2. Fixed.
  3. Fixed.
moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community

I agree with getting rid of caches of caches. So, I'm going to RTBC this last patch and Catch can do the final review and commit once latest patch comes back green.

webchick’s picture

Assigned: Wim Leers » catch

Assigning to catch.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 71: client_side_caching_contextual_edit-2005644-71.patch, failed testing.

penyaskito’s picture

Status: Needs work » Needs review
Anonymous’s picture

this patch looks great. one question about:

@@ -1622,6 +1628,7 @@ function user_role_grant_permissions($rid, array $permissions = array()) {
     $role->grantPermission($permission);
   }
   $role->save();
+  Cache::invalidateTags(array('permissions' => $rid));
 }

does that need to go somewhere like user_user_role_presave() so that changes made via the entity API directly also get picked up?

otherwise, looks good to do to me. WimLeers++

amateescu’s picture

  1. +++ b/core/lib/Drupal/Core/Session/AccountInterface.php
    @@ -43,6 +43,20 @@ public function getRoles();
    +   * Get a hash that uniquely identifies the user's permissions.
    

    Gets ...

  2. +++ b/core/lib/Drupal/Core/Session/AccountInterface.php
    @@ -43,6 +43,20 @@ public function getRoles();
    +   * @see user_role_change_permissions()
    +   * @see user_role_grant_permissions()
    +   * @see user_role_revoke_permissions()
    

    Based on the last point below, these @see references are pointless.

  3. +++ b/core/modules/contextual/js/contextual.js
    @@ -17,24 +17,51 @@ var options = $.extend(drupalSettings.contextual,
    +var cachedPermissionsHash = storage.getItem('Drupal.contextual.permissionsHash');
    

    Why is the permission hash cached under the contextual "namespace"?

  4. +++ b/core/modules/user/lib/Drupal/user/RoleStorageController.php
    @@ -17,6 +18,15 @@ class RoleStorageController extends ConfigStorageController implements RoleStora
    +    // Clear the user permissions cache.
    +    Cache::invalidateTags(array('permissions' => $ids));
    

    This should be moved to the postSave() and postDelete() methods on the Role entity.

  5. +++ b/core/modules/user/user.module
    @@ -1,6 +1,8 @@
    +use Drupal\Core\Cache\CacheBackendInterface;
    

    Not used anywhere?

  6. +++ b/core/modules/user/user.module
    
    @@ -1622,6 +1628,7 @@ function user_role_grant_permissions($rid, array $permissions = array()) {
    +  Cache::invalidateTags(array('permissions' => $rid));
    ...
    
    @@ -1642,6 +1649,7 @@ function user_role_revoke_permissions($rid, array $permissions = array()) {
    +  Cache::invalidateTags(array('permissions' => $rid));
    

    These can be removed because the invalidation should already happen in Role::postSave().

Wim Leers’s picture

#76: #77 already deals with this :)

#77:

  1. Good catch — thanks, fixed!
  2. Ok, gone.
  3. Which piece of code would then be responsible for storing "the canonical" version of this cache tag in the client-side cache? Drupal.js? The code responsible for storing the canonical cache entry would then also be responsible for clearing all client-side cache entries in case that cache tag changes, but how could it possibly figure out which cache entries depend on it? And it's impossible to make the owner of the cache entries (in this case, contextual.js) "listen" for changes in the cache tag, because it's entirely possible that no contextual links exist on the page that the user happens to visit when the cache tag changes; when the user then loads another page where contextual.js is loaded, no "changed cache tag" event could possibly be triggered by the code responsible for the "canonical" cache tag, and then contextual.js would be working with stale data.
    I hope that's sufficiently clear.
  4. Interesting :) Done.
  5. You're right. A remnant from the procedural implementation of this. Thanks, gone!
  6. HAH! I don't know why I didn't realize that! THANK YOU! Much, much cleaner like that :)
Wim Leers’s picture

Having touched the cache tag handling again in #78: shouldn't the cache tag be called role rather than permissions? Because permissions:authenticated does not make much sense, role:authenticated does.

catch’s picture

@Wim I'm not aware of dynamic contextual links, but it's very, very common for comment links (and whichever module it is that adds similar links to nodes).

Wim Leers’s picture

#80: Okay. That's fair :)
What about this: rescope this patch to only introduce drupalSettings.user.permissionsHash and client-side caching of in-place editing metadata. Make the client-side caching of contextual links a follow-up. Then we can at least move forward. Agreed?

catch’s picture

We can split it, that's fine.

I think this is fixable though - either here if we keep the patch together or in the new issue. Personally I don't think we should worry about completely arbitrary dynamic links, only those which depend on something about the entity.

If the entity changes, then the render cache item for the entity is updated, so we should be able to use that (the last updated timestamp?) to affect the key used for the client-side contextual links. That way a node getting published or unpublished will force new contextual links to be rendered for it.

Wim Leers’s picture

Title: Use client-side cache tags & caching to eliminate 2 HTTP requests/page (Edit + Contextual), introduce drupalSettings.user.permissionsHash » Use client-side cache tags & caching to eliminate 1 HTTP requests/page for in-place editing metadata, introduce drupalSettings.user.permissionsHash
Priority: Normal » Major
Issue summary: View changes
Issue tags: -D8 cacheability
Related issues: +#2136507: Use client-side cache tags & caching to eliminate 1 HTTP requests/page for rendering Contextual Links
FileSize
14.28 KB
10.88 KB

#82: interesting :) However, that does feel like it could use some more thinking and scrutiny, so splitting that into a separate issue: #2136507: Use client-side cache tags & caching to eliminate 1 HTTP requests/page for rendering Contextual Links.

I also believe a "Major" priority was actually warranted here all along, because this significantly impacts front-end performance.

moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community

OK, restore RTBC for this simplified patch.

tim.plunkett’s picture

  1. +++ b/core/lib/Drupal/Core/Session/UserSession.php
    @@ -142,6 +144,24 @@ public function hasPermission($permission) {
    +    if ($cache = cache()->get("user_permissions_hash:$role_list")) {
    ...
    +      $permissions_hash = hash('sha256', drupal_get_private_key() . drupal_get_hash_salt() . serialize(user_role_permissions($sorted_roles)));
    +      cache()->set("user_permissions_hash:$role_list", $permissions_hash, CacheBackendInterface::CACHE_PERMANENT, array('role' => $sorted_roles));
    

    It's a shame to add function calls to Drupal\Core code.

  2. +++ b/core/modules/user/lib/Drupal/user/Entity/User.php
    @@ -236,6 +237,26 @@ public function hasPermission($permission) {
    +    if ($cache = cache()->get("user_permissions_hash:$role_list")) {
    ...
    +      cache()->set("user_permissions_hash:$role_list", $permissions_hash, CacheBackendInterface::CACHE_PERMANENT, array('role' => $sorted_roles));
    

    These could be changed as well.

catch’s picture

Status: Reviewed & tested by the community » Needs work

Let's do those.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
14.3 KB
1.4 KB

#85: great catch! Thanks!

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC per #84.

damiankloip’s picture

  1. +++ b/core/lib/Drupal/Core/Session/UserSession.php
    @@ -142,6 +144,24 @@ public function hasPermission($permission) {
    +      cache()->set("user_permissions_hash:$role_list", $permissions_hash, CacheBackendInterface::CACHE_PERMANENT, array('role' => $sorted_roles));
    
    +++ b/core/modules/user/lib/Drupal/user/Entity/User.php
    @@ -237,6 +238,26 @@ public function hasPermission($permission) {
    +      cache()->set("user_permissions_hash:$role_list", $permissions_hash, CacheBackendInterface::CACHE_PERMANENT, array('role' => $sorted_roles));
    

    This should use \Drupal::cache() directly I think.

  2. +++ b/core/modules/user/lib/Drupal/user/Entity/Role.php
    @@ -126,10 +127,21 @@ public function preSave(EntityStorageControllerInterface $storage_controller) {
    +    Cache::invalidateTags(array('role' => $this->id()));
    ...
    +    Cache::invalidateTags(array('role' => $ids));
    

    In other places I think we're been going with: entity_type => array(id => id)?

  3. +++ b/core/modules/user/lib/Drupal/user/Entity/User.php
    @@ -237,6 +238,26 @@ public function hasPermission($permission) {
    +      $permissions_hash = hash('sha256', drupal_get_private_key() . drupal_get_hash_salt() . serialize(user_role_permissions($sorted_roles)));
    

    You can use \Drupal::service('private_key')->get() for this.

Also, we are duplicating the code in getPermissionHash(), it would be nice if we didn't have to do that. Also, it just gets me thinking (and just asking) does this belong on the AccountInterface?

Wim Leers’s picture

#89:

  1. that's precisely what #85 already said, and was already fixed in #87 :)
  2. I don't think there's any real difference there, because what gets stored is the values, not the keys?
  3. Didn't know about that one! Changed. Thanks :)
  4. I probably hate the duplication even more than you do. I think it belongs on AccountInterface, because some contrib module might want to retrieve multiple user's permission hashes (not just the one for the current session), or compare them?
damiankloip’s picture

that's precisely what #85 already said, and was already fixed in #87 :)

Nope, don't think so. The dreditor review I did was from #87 :) Looks like you changed the get calls, but not set.

I think there should be another object that gets/sets these hashes, doesn't seem like a part of the interface to me. As this new 'thing' would just rely on AccountInterface anyway.

Wim Leers’s picture

Status: Reviewed & tested by the community » Needs work

damiankloip has convinced me that getPermissionHash() doesn't belong on AccountInterface. msonnabaum agrees with that statement too.

So: refactoring a bit.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
18.92 KB
16.61 KB

So, refactored. No more AccountInterface modification (and therefor accompanying changes in the User entity and in UserSession), but only a new user.permissions_hash service, implemented by the PermissionsHash class with a generate() method. Comes with unit test coverage.

P.S.: As usual when writing unit tests, this took me hours to get working :(

damiankloip’s picture

I think this is looking much better already.

  1. +++ b/core/modules/user/lib/Drupal/user/PermissionsHash.php
    @@ -0,0 +1,80 @@
    +  protected $private_key;
    

    property names should be camelCased.

  2. +++ b/core/modules/user/lib/Drupal/user/PermissionsHash.php
    @@ -0,0 +1,80 @@
    +  /**
    +   * {@inheritdoc}
    +   *
    +   * Cached by role, invalidated whenever permissions change.
    +   */
    +  public function generate(AccountInterface $account) {
    

    There is no doc to inherit from here :)

  3. +++ b/core/modules/user/lib/Drupal/user/PermissionsHash.php
    @@ -0,0 +1,80 @@
    +   * @param array $roles
    

    We could use the Drupal\user\Entity\Role[] syntax for the doc here? I really like that.

  4. +++ b/core/modules/user/lib/Drupal/user/PermissionsHash.php
    @@ -0,0 +1,80 @@
    +    $permissions = user_role_permissions($roles);
    

    Would be nice to just inject the user role controller here, but looks like user_role_permissions still has a bit of logic in that this may need?

    Maybe a @todo there?

  5. +++ b/core/modules/user/lib/Drupal/user/PermissionsHashInterface.php
    @@ -0,0 +1,28 @@
    +interface PermissionsHashInterface {
    

    Ah! Now the above inheritdoc does make sense. The PermissionsHash class just needs to implement the new interface :)

  6. +++ b/core/modules/user/tests/Drupal/user/Tests/PermissionsHashTest.php
    @@ -0,0 +1,130 @@
    + * Tests the user permissions hash generator service.
    ...
    +class PermissionsHashTest extends UnitTestCase {
    

    Some @group tags too would be cool, maybe @Drupal, @User or something?

  7. +++ b/core/modules/user/tests/Drupal/user/Tests/PermissionsHashTest.php
    @@ -0,0 +1,130 @@
    +   * @var \Drupal\user\UserInterface
    

    Add the mocked class too here:

    @var \Drupal\user\UserInterface|\PHPUnit_Framework_MockObject_MockObject

  8. +++ b/core/modules/user/tests/Drupal/user/Tests/PermissionsHashTest.php
    @@ -0,0 +1,130 @@
    +  public static function getInfo() {
    

    I think we add {@inheritdoc} here too now.

  9. +++ b/core/modules/user/tests/Drupal/user/Tests/PermissionsHashTest.php
    @@ -0,0 +1,130 @@
    +      ->getMockBuilder('Drupal\user\Entity\User')
    

    Should live on the same line as $this (atleast that is what we've done in all other PHPUnit tests). Maybe it's preference, not sure ...

  10. +++ b/core/modules/user/tests/Drupal/user/Tests/PermissionsHashTest.php
    @@ -0,0 +1,130 @@
    +    $this->cache = $this->getMockBuilder('Drupal\Core\Cache\MemoryBackend')
    

    We should use CacheBackendInterface here instead. We should prob add some assertions that are testing the cache gets/sets too.

  11. +++ b/core/modules/user/user.module
    @@ -131,9 +131,13 @@ function user_js_alter(&$javascript) {
    +      'permissionsHash' => \Drupal::getContainer()->get('user.permissions_hash')->generate($user),
    

    \Drupal::service('user.permission_hash')->generate()

The last submitted patch, 93: client_side_caching_edit-2005644-93.patch, failed testing.

Wim Leers’s picture

  • 1–3: fixed
  • 4: Yes, we need user_role_permissions() and that hasn't been OOPified, so sadly, no choice. Added a @todo
  • 5: :)
  • 6: good point, done
  • 7–8: I didn't know that, thanks, done
  • 9: silly me, of course, fixed
  • 10: I did that first, but that doesn't work (and the fact that it doesn't work makes sense):
    Fatal error: Class Mock_CacheBackendInterface_469c6992 contains 12 abstract methods and must therefore be declared abstract or implement the remaining methods (Drupal\Core\Cache\CacheBackendInterface::getMultiple, Drupal\Core\Cache\CacheBackendInterface::delete, Drupal\Core\Cache\CacheBackendInterface::deleteMultiple, ...) in /Users/wim.leers/Work/drupal-tres/core/vendor/phpunit/phpunit-mock-objects/PHPUnit/Framework/MockObject/Generator.php(231) : eval()'d code on line 101
    

    Using the MemoryBackend is also what DefaultPluginManagerTest does.

  • 11: Hah! I had totally forgotten about that one :)

The last submitted patch, 96: client_side_caching_edit-2005644-96.patch, failed testing.

damiankloip’s picture

FileSize
18.87 KB
1.45 KB

I did that first, but that doesn't work (and the fact that it doesn't work makes sense):

That's because you are settings just the get/set methods for the stub. If you remove the setMethods() like this, or just use an empty array when you want to add expectations for methods this should work fine.

I still think this needs a test method to assert the expectations on the cache getting/setting. Then I think this is good to go.

The last submitted patch, 98: 2005644-98.patch, failed testing.

Wim Leers’s picture

FileSize
20.69 KB
3 KB

#98: ok. Still learning more PHPUnit. Thank you!

In this reroll, I corrected one change introduced in #98, updated a bunch of docs, added cache method calls expectations and worked to try to get the tests to pass.

About the cache method calls expectations: I actually spent probably an hour of my time two days ago trying to get that to work. My conclusion was that the only way to get that to work is to use the insanely verbose ->at() calls, which made the end result feel way too verbose. So I left it out. At your request, I've recreated that.
But I must say this really feels like completely overkill testing for such simple logic to me.

damiankloip’s picture

Yes, I can appreciate that it feels verbose and really picky, this is unit testing though :)

Instead of all the at() calls and trying to put everything in one test method, I would recommend breaking it out like this, and testing the different scenarios like this. How do you feel about that instead? It's much clearer what is going on I think.

The last submitted patch, 100: client_side_caching_edit-2005644-100.patch, failed testing.

The last submitted patch, 101: 2005644-101.patch, failed testing.

damiankloip’s picture

FileSize
31.41 KB
10.74 KB

Ok, so what I think is happening is tests extending LocalTaskIntegrationTest (of which UserLocalTasksTest was one of) are breaking things...

LocalTaskIntegrationTest is creating an actual instance of ModuleHandler to use in it's unit tests.. (should prob not do this anyway). Leading up to this, tests extending LocalTaskIntegrationTest are implementing a moduleList with the path to the module files. ModuleHandler is then calling dirname() on this file, which is loading it (!!). One of these being user.module. So it tries to parse the user_role_permissions function again but it's already been declared in the new unit test added in this patch!

So we could fix it by fixing the local task tests.

Wim Leers’s picture

Wow, awesome detective work!

Indeed, it looks like the local task unit tests are not really unit tests, but DUTB-like tests.

If this is green again, I'm happy :)

The last submitted patch, 104: 2005644-104.patch, failed testing.

damiankloip’s picture

So, not sure what to do about the fact the the 'unit' tests before were relying on invoking hook_local_tasks_alter in content_translation module...

Wim Leers’s picture

FileSize
34.54 KB
3.48 KB

Logically, ContentTranslationLocalTasksTest should be a DrupalUnitTestBase or WebTestBase test, since it loads a .module file and invokes a hook. That's what both damiankloip and I think.
So I started looking into the history of ContentTranslationLocalTasksTest, to figure out how it got to be the way it is. It was introduced as part of the Big Local Task Conversion. There, it seems to be considered normal that hooks are invoked as part of this unit test, because it's not considered a unit test, but an integration test… I want to avoid a bikeshed, so the simplest possible solution is to repurpose tim.plunkett's patch there at
#2102125-107: Big Local Task Conversion to solve this issue.

damiankloip already explained in #104 why actually loading .module files (or files with procedural functions in general) is problematic: because they might have been stubbed by some unit test. That's why he made the necessary changes to make sure that all the local task "unit/integration" tests don't load .module files anymore. But that itself has now broken ContentTranslationLocalTasksTest, which needed that to happen.

So, in this reroll, I keep damiankloip's changes, but bring back the loading of the .module file for the content_translation module, to get its test to pass again. Not pretty, but it's still prettier/cleaner/better than before. And now ContentTranslationLocalTasksTest has a @ runTestsInSeparateProcesses annotation, so the procedural functions in content_translation.module won't "bleed through" to other PHPUnit tests anymore — thanks to damiankloip for pointing that out :)

The last submitted patch, 108: client_side_caching_edit-2005644-108.patch, failed testing.

dawehner’s picture

Logically, ContentTranslationLocalTasksTest should be a DrupalUnitTestBase or WebTestBase test, since it loads a .module file and invokes a hook. That's what both damiankloip and I think.

To be honest I kind of disagree. An integration tests a couple of code and the interaction between them. This is a total valid usecase, using the PHPunit framework.

What we could do to solve the .module loading problem is also to basically copy the code of content_translation_local_tasks_alter into our module returnCallback.

Wim Leers’s picture

FileSize
34.76 KB
2.1 KB

#110: ok, that works too.

damiankloip’s picture

Status: Needs review » Reviewed & tested by the community

I am good with that change. It is not the fault of this issue.

I semi disagree with #110, having an integration test with PHPUnit is fine, as long as it doesn't affect other tests. Which unfortunately that does..

Thanks for your patience, Wim!

catch’s picture

Status: Reviewed & tested by the community » Needs review
+++ b/core/modules/user/lib/Drupal/user/PermissionsHash.php
@@ -0,0 +1,82 @@
+  protected function doGenerate(array $roles) {

Is this definitely worth caching in its own entry?

I'd assume we load the permissions per-user per-request anyway for user_access() calls, which leaves the hash/serialize().

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
34.91 KB

Straight reroll; patch didn't apply anymore.


Good question!

The caching was added to address Dries' performance concerns in #31.

From #35:

Benchmarks (using $start = microtime(TRUE); <calculations here> $end = microtime(TRUE); var_dump($end - $start);) show that:

  • cache hits: <= 1 ms (order of magnitude: 1 ms)
  • cache misses: <= 9 ms (order of magnitude: 10 ms)

I just tested it again, and those numbers are still accurate. One nuance that was missing in those numbers is that a cache miss still involves *filling* the cache. Without filling the cache, it takes 2.5–5 ms (usually 2.9±0.2 ms). So, all in all, the caching still seems to be worth it.

Because of that: back to RTBC, because this is a committer judgement call.

damiankloip’s picture

+1 on that.

Anonymous’s picture

really like this patch. apart from the one question below, i agree that this is RTBC.

one question - does user_role_permissions() guarantee a predictable order on returned permissions? given the storage is pluggable, i guess not, so we could get different hashes for the same set of roles, and we prolly want a sort() in here:

+  protected function doGenerate(array $roles) {
+    // @todo Once Drupal gets rid of user_role_permissions(), we should be able
+    // to inject the user role controller and call a method on that instead.
+    $permissions = user_role_permissions($roles);
+    return hash('sha256', $this->privateKey->get() . drupal_get_hash_salt() . serialize($permissions));
+  }
catch’s picture

Assigned: catch » Unassigned
Status: Reviewed & tested by the community » Needs review

Let's add that here. I can imagine committing this without, then debugging why caches aren't behaving quite as they should in 2 years on a client site, then running git blame.

Anonymous’s picture

ok, here's a reroll with one line added to doGenerate(), no other changes:

  protected function doGenerate(array $roles) {
    // @todo Once Drupal gets rid of user_role_permissions(), we should be able
    // to inject the user role controller and call a method on that instead.
    $permissions = user_role_permissions($roles);
    sort($permissions);
    return hash('sha256', $this->privateKey->get() . drupal_get_hash_salt() . serialize($permissions));
  }

if it's green, i think this is RTBC.

Wim Leers’s picture

Assigned: Unassigned » catch
Status: Needs review » Reviewed & tested by the community
FileSize
35.07 KB
1.13 KB

Wow, excellent catch beejebus, thank you! I totally agree with catch that this is the sort of thing that could cause a very, very subtle bug that's very hard to track down.

Fixed :) Back to RTBC as per #116 and earlier, since the fix is trivial.

Wim Leers’s picture

Oh! Cross-patch-post! :/

@beejeebus: your patch is incorrect, because it assumes $permissions is a 1D array. It's a 2D array. The order of the first dimension (role IDs) is given by the function parameter, so it doesn't need to be sorted. It's their children (the second dimension) that is retrieved from pluggable storage, and hence those need to be sorted.

Anonymous’s picture

yay for Wim fixing my patch :-)

so, just commenting to say i think this is good to go.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

Wim Leers’s picture

Issue tags: -sprint

Yay, thanks! :)

Wim Leers’s picture

#2136507: Use client-side cache tags & caching to eliminate 1 HTTP requests/page for rendering Contextual Links is unblocked now that this is committed. I rerolled that patch. Please review that patch as well, because it gets rid of another HTTP request per page load!

Status: Fixed » Closed (fixed)

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

Wim Leers’s picture