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:
- worse front-end performance
- more networking activity (hence reduced battery life)
- 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 ondrupalSettings
. - API change:
AccountInterface
has a newgetPermissionsHash()
method, which is implemented by\Drupal\user\Entity\User
and\Drupal\Core\Session\UserSession
. (Which is better than a newuser_permissions_get_hash()
procedural function, which is what I had first.) - API change: A
permissions
cache tag containing role IDs is introduced.
Comment | File | Size | Author |
---|---|---|---|
#119 | interdiff.txt | 1.13 KB | Wim Leers |
#119 | client_side_caching_edit-2005644-118.patch | 35.07 KB | Wim Leers |
#100 | client_side_caching_edit-2005644-100.patch | 20.69 KB | Wim Leers |
#90 | client_side_caching_edit-2005644-90.patch | 14.33 KB | Wim Leers |
Comments
Comment #1
Wim LeersI tried it first with a cookie approach, then realized the
drupalSettings
approach could be much, much simpler.Comment #2
nod_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?
Comment #3
Wim LeersThe 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.
Comment #4
nod_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.
Comment #6
Wim LeersIt seems
ThemeInfoStylesTest
was rather poorly written. It fails wheneverdrupalSettings
exist, even though that's not what it's testing.Comment #8
Wim Leers#6: js_caching_metadata-2005644-6.patch queued for re-testing.
Comment #9
nod_That works :)
Comment #10
amateescu CreditAttribution: amateescu commentedI think we should at least give the security team a chance to look at this..
Comment #11
Gábor HojtsyIs this a reliable pattern for drupalSettings? Are there no other parts that might match this pattern? CDATA is a pretty general container.
Comment #12
moshe weitzman CreditAttribution: moshe weitzman commentedThe 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...
Comment #13
nod_hook_page_build() :)
Comment #14
Wim Leersuser_page_build()
is ahook_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.
Comment #15
Gábor HojtsyIn 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.
Comment #16
Wim Leers#15: it's a permissions hash, not roles hash. Also, isn't #12 enough to confirm there's no security problem? :)
Comment #17
Wim Leersmoshe clarified #12 in chat. I'll do that.
Comment #18
Wim Leers#12 implemented. Tested manually; still works perfectly.
Comment #19
Wim LeersFile upload crashed earlier, here's the interdiff for #18.
Comment #20
moshe weitzman CreditAttribution: moshe weitzman commentedThanks for more robust invalidation. This ready for commit.
Comment #21
Dave ReidI 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.
Comment #22
moshe weitzman CreditAttribution: moshe weitzman commented@dave - I think thats a separate feature request. There is no harm in adding multiple bits into the Settings array.
Comment #23
David_Rothstein CreditAttribution: David_Rothstein commentedShouldn't there be tests? (The issue summary says so.)
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).
Comment #24
Dave ReidSo 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?
Comment #25
Wim LeersDavid: 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?
Comment #26
Wim LeersTest 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 :)Comment #27
Wim LeersD'oh, I forgot to do #23.
Comment #28
Wim LeersAddresses #23.
Comment #29
nod_works for me :)
Comment #30
YesCT CreditAttribution: YesCT commentedThis issue was RTBC and passing tests on July 1, the beginning of API freeze.
Comment #31
Dries CreditAttribution: Dries commentedI'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.
Comment #32
effulgentsia CreditAttribution: effulgentsia commentedThere's nothing in the latest patch that breaks BC, so this is still eligible post-July-1. Removing the tag.
Comment #33
Wim Leers#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…!)
Comment #34
moshe weitzman CreditAttribution: moshe weitzman commented@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.
Comment #35
Wim LeersBenchmarks
Benchmarks (using
$start = microtime(TRUE); <calculations here> $end = microtime(TRUE); var_dump($end - $start);
) show that:Conclusion: 1 ms amortized per page load.
Rerolled patch: improved performance
Potential performance improvements:
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:
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:
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.: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:
… 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 :)
Comment #37
moshe weitzman CreditAttribution: moshe weitzman commentedNice reply! Failed testing :(
Comment #38
Wim LeersYes, 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.
Comment #39
Wim LeersThis fixes the five failures in
HttpBasicTest
. I did not find how to fix the three failures inSessionTest
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.
Comment #41
Wim LeersIt should just needs reroll! :)
Comment #42
amateescu CreditAttribution: amateescu commentedRerolled.
Comment #44
jessebeach CreditAttribution: jessebeach commentedWould a hash the rolls up the Role's permission profile be sufficient? We should need a per-user cache.
Comment #45
jessebeach CreditAttribution: jessebeach commentedWim 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.
Comment #46
jessebeach CreditAttribution: jessebeach commentedActually, 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.
Comment #47
Wim Leers#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.
Comment #48
Wim Leers.
Comment #49
Wim Leers#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:
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:sessionStorage
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.
Comment #50
Wim LeersWill be on my plate in 1 week or so hopefully.
Comment #51
Wim LeersdrupalSettings.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.
Comment #53
Wim LeersCorrection, 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.
Comment #54
Wim LeersSome 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.
Comment #55
Wim LeersAdding "Performance" tag for tracking purposes.
Comment #56
Wim LeersStraight reroll of #51; tracking HEAD. Next step: make this pass tests.
Comment #58
Wim LeersTests 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.
Comment #60
Wim Leers58: client_side_caching_contextual_edit-2005644-58.patch queued for re-testing.
Comment #61
Wim LeersI think it's in line with the spirit of D8 to not add a new procedural function to
user.module
, but to extendAccountInterface
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 thedestination
parameter using&
instead of?
.Comment #63
Wim LeersComment #65
Wim LeersThis should do the trick at last.
Comment #66
Wim LeersComplete 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).
Comment #67
Wim LeersAlso see #607244-79: Decrease performance impact of contextual links — I think committing this patch will allow us to immediately close that issue as well! :)
Comment #68
Wim LeersComment #69
catchCouple 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.
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.
Comment #70
moshe weitzman CreditAttribution: moshe weitzman commentedVery 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.
Comment #71
Wim Leers#69:
hook_menu()
'stitle callback
, was it used in practice?$_SESSION
-cached permissions hashI'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:
Comment #72
moshe weitzman CreditAttribution: moshe weitzman commentedI 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.
Comment #73
webchickAssigning to catch.
Comment #75
penyaskito71: client_side_caching_contextual_edit-2005644-71.patch queued for re-testing.
Comment #76
Anonymous (not verified) CreditAttribution: Anonymous commentedthis patch looks great. one question about:
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++
Comment #77
amateescu CreditAttribution: amateescu commentedGets ...
Based on the last point below, these @see references are pointless.
Why is the permission hash cached under the contextual "namespace"?
This should be moved to the postSave() and postDelete() methods on the Role entity.
Not used anywhere?
These can be removed because the invalidation should already happen in Role::postSave().
Comment #78
Wim Leers#76: #77 already deals with this :)
#77:
I hope that's sufficiently clear.
Comment #79
Wim LeersHaving touched the cache tag handling again in #78: shouldn't the cache tag be called
role
rather thanpermissions
? Becausepermissions:authenticated
does not make much sense,role:authenticated
does.Comment #80
catch@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).
Comment #81
Wim Leers#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?Comment #82
catchWe 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.
Comment #83
Wim Leers#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.
Comment #84
moshe weitzman CreditAttribution: moshe weitzman commentedOK, restore RTBC for this simplified patch.
Comment #85
tim.plunkettIt's a shame to add function calls to Drupal\Core code.
These could be changed as well.
Comment #86
catchLet's do those.
Comment #87
Wim Leers#85: great catch! Thanks!
Comment #88
Wim LeersBack to RTBC per #84.
Comment #89
damiankloip CreditAttribution: damiankloip commentedThis should use \Drupal::cache() directly I think.
In other places I think we're been going with: entity_type => array(id => id)?
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?
Comment #90
Wim Leers#89:
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?Comment #91
damiankloip CreditAttribution: damiankloip commentedNope, 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.
Comment #92
Wim Leersdamiankloip has convinced me that
getPermissionHash()
doesn't belong onAccountInterface
. msonnabaum agrees with that statement too.So: refactoring a bit.
Comment #93
Wim LeersSo, refactored. No more
AccountInterface
modification (and therefor accompanying changes in theUser
entity and inUserSession
), but only a newuser.permissions_hash
service, implemented by thePermissionsHash
class with agenerate()
method. Comes with unit test coverage.P.S.: As usual when writing unit tests, this took me hours to get working :(
Comment #94
damiankloip CreditAttribution: damiankloip commentedI think this is looking much better already.
property names should be camelCased.
There is no doc to inherit from here :)
We could use the Drupal\user\Entity\Role[] syntax for the doc here? I really like that.
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?
Ah! Now the above inheritdoc does make sense. The PermissionsHash class just needs to implement the new interface :)
Some @group tags too would be cool, maybe @Drupal, @User or something?
Add the mocked class too here:
@var \Drupal\user\UserInterface|\PHPUnit_Framework_MockObject_MockObject
I think we add {@inheritdoc} here too now.
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 ...
We should use CacheBackendInterface here instead. We should prob add some assertions that are testing the cache gets/sets too.
\Drupal::service('user.permission_hash')->generate()
Comment #96
Wim Leersuser_role_permissions()
and that hasn't been OOPified, so sadly, no choice. Added a @todoUsing the
MemoryBackend
is also whatDefaultPluginManagerTest
does.Comment #98
damiankloip CreditAttribution: damiankloip commentedThat'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.
Comment #100
Wim Leers#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.
Comment #101
damiankloip CreditAttribution: damiankloip commentedYes, 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.
Comment #104
damiankloip CreditAttribution: damiankloip commentedOk, 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.
Comment #105
Wim LeersWow, 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 :)
Comment #107
damiankloip CreditAttribution: damiankloip commentedSo, 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...
Comment #108
Wim LeersLogically,
ContentTranslationLocalTasksTest
should be aDrupalUnitTestBase
orWebTestBase
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 brokenContentTranslationLocalTasksTest
, which needed that to happen.So, in this reroll, I keep damiankloip's changes, but bring back the loading of the
.module
file for thecontent_translation
module, to get its test to pass again. Not pretty, but it's still prettier/cleaner/better than before. And nowContentTranslationLocalTasksTest
has a@ runTestsInSeparateProcesses
annotation, so the procedural functions incontent_translation.module
won't "bleed through" to other PHPUnit tests anymore — thanks to damiankloip for pointing that out :)Comment #110
dawehnerTo 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.
Comment #111
Wim Leers#110: ok, that works too.
Comment #112
damiankloip CreditAttribution: damiankloip commentedI 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!
Comment #113
catchIs 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().
Comment #114
Wim LeersStraight reroll; patch didn't apply anymore.
Good question!
The caching was added to address Dries' performance concerns in #31.
From #35:
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.
Comment #115
damiankloip CreditAttribution: damiankloip commented+1 on that.
Comment #116
Anonymous (not verified) CreditAttribution: Anonymous commentedreally 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:
Comment #117
catchLet'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.
Comment #118
Anonymous (not verified) CreditAttribution: Anonymous commentedok, here's a reroll with one line added to doGenerate(), no other changes:
if it's green, i think this is RTBC.
Comment #119
Wim LeersWow, 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.
Comment #120
Wim LeersOh! 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.Comment #121
Anonymous (not verified) CreditAttribution: Anonymous commentedyay for Wim fixing my patch :-)
so, just commenting to say i think this is good to go.
Comment #122
catchCommitted/pushed to 8.x, thanks!
Comment #123
Wim LeersYay, thanks! :)
Comment #124
Wim Leers#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!
Comment #126
Wim LeersBackported to the Drupal 7 version of Edit: #2178567: Backport: Use client-side cache tags & caching to eliminate 1 HTTP requests/page for in-place editing metadata, introduce drupalSettings.user.permissionsHash.