Added code that also invalidates cache tags when an entity is removed from a group and expanded the test case.
Backporting as we speak.
I could live with #37.
Seems to check all the boxes:
- Document as "Use at your own risk"
- We won't stop you from using it at your own risk
- You will get zero support when ignoring @final
Adding an attribute to the class that skips it for registration if the dependency is unmet seems like a good solution to the optional constructor argument problem. If we know that the dependency needs to exist for our hooks to be registered as a service, we can safely use the dependency's services with autowiring.
Be very careful when using the patches provided here. It bypasses Group's access promises when certain data could not be found. Use at your own risk.
Re @firewaller please don't reopen closed issues, the module is far from unusable without this patch and I encourage you to look into which module is checking group relationship create access without providing the right context.
I think this will be fixed as part of 🐛 Node update hook is triggered upon group content create RTBC . I.e.: We will no longer save entities when they are added to or removed from a group.
Test-only fails, full MR goes green. I call that a win :D
kristiaanvandeneynde → created an issue.
Seems like this MR also needs to tackle similar code in ::postDelete()
We also need to make sure that, when we're on a config form that got modified by our service, we do not call ConfigWrapperStorage->wrapEntity() during the form validation step as that actually saves a ConfigWrapper in the system for a config entity that may not get saved after all. It should be harmless if that happens, but better safe than sorry.
We should instead manually create one so that it has isNew() still set. It's only for validation purposes, so the wrapper should never get saved.
Okay I just realized we can kill the wizard with this approach + 📌 Create a new editor flow that does not involve wizards or custom pages Active
First of all we will need to remove the wizard, but not with the automatic selection of the group from the other issue. This can be done as follows:
- Adjust GroupRelationshipCardinalityValidator so it works with new referenced entities
- Create a service that attaches (part of) GroupRelationship form to another Group or entity form. This will only attach the form if there are any custom fields on it that are accessible.
- If it can't attach a form, it will still attach a submit handler that creates a blank GroupRelationship (GR) entity with either the newly created Group or groupable entity added to it.
- If it can attach a form, we attach a validation handler and submit handler along with the form:
- Validation handler builds the original form's entity and assigns it to the GR's correct reference's "entity" property. It then calls validate() on that and flags violations on the GR subform.
- Submit handler builds and saves the original form's entity, then assigns that to the GR it builds and saves the GR.
- Double check that cardinality is properly reported even if fields for group and entity are hidden. Should be fine because we call $entity->validate() ourselves
- when we retrieve an item from the fast cache, we check if the node uuid matches the node uuid we're on, if it does, then we ignore $last_write_timestamp only for the current node.
Imagine this set-up (with write-through on set):
- Node A writes item X to the persistent cache, we flag item X with the UUID for node A
- Node A immediately writes item X to the fast cache, also flagged with the UUID for node A
- We keep a last write timestamp per node
Now, item X should remain valid indefinitely unless its tags got invalidated, it's got a max age that has expired or it got manually flushed. The first two scenarios are no troublemakers as both the fast and persistent cache can deal with tags and max ages just fine. What has me worried is the manual clear.
If I trigger a manual clear on node B, that will clear the persistent cache used by all nodes, but only the fast cache of node B (unless otherwise set up). Now node B has to recalculate item X and stores it in the persistent cache as originating from node B's UUID. Cool.
But node A's fast cache hasn't been cleared and it still contains item X as originating from node A. In the meantime, node A's last write timestamp has not been updated. So now node A will happily keep serving a stale cache item because it has no clue that the underlying persistent item now originates from node B. With the current system, this cannot happen as we have one timestamp to rule them all.
This can be cured by making sure markAsOutdated() called from invalidate calls also updates the last write timestamp of all nodes. That would, however, beat the purpose of having a timestamp per node unless we differentiate between writes and invalidations when calling markAsOutdated().
- we also have to compare against the max($timestamps) of all the other nodes though, just in case they've written more recently than the item was written.
Maybe that was your way around the above, but then I wonder if we still gain anything. Checking if any other node recently wrote something seems to undo what we're trying to achieve here. It sounds a lot like one centralized timestamp, with extra steps.
I think the big question is whether the better hit rate here, would make it worse diluting the grace period approach in #3526080: Reduce write contention to the fast backend in ChainedFastBackend.
We would need to bring back the write-through on save and delete, or at least delete the APCu item if that's better for locking and space usage, we couldn't just ignore it as is done in that issue.
The more I think about this, the more I'm wondering whether we should just pick one road and go down that one. Trying to combine the grace period from the other issue, dropping write-throughs in the process, seems rather incompatible with this one.
And given my thoughts above I'm not entirely sure how much benefit we could gain here. But that might very well be because I'm not fully picturing what you had in mind yet with the timestamps per node.
Say you have a group relation plugin called group_media_foo to enable adding media entities to a group. This plugin can be installed on a group type and by doing so a group relationship type will be created for that unique group_type-plugin combo. Any media added to groups of that type will be using a group_relationship entity of the automatically created group relationship type (aka bundle).
After a while you install pathauto and you need it to work with grouped media entities. If supporting new functionality would require a change in plugin to , say, group_media_bar, then you would not be able to switch existing relationships for media over to that plugin as it would inherently mean you'd have to swap out their bundle.
That's why group relation plugins serving as an extension point is a bad idea. Changing the plugin definition to point to the extending class also doesn't work because then only one module can ever do that.
on other hand, all that the hooks implementation knows is that cache has been cleared without context about group related operation .
No, they would have to implement hook_group_relationship_insert(). Which tells you nothing about a cache being cleared and everything about an entity being created.
I would also strongly advise against adjusting or adding group relation plugins to achieve the desired result, as they are what power group relationship types. Those are bundles and so you'd have to be very careful with changing a plugin for another as then you'd have to update the group relationship entities' bundle, which is a total pain in the ass.
Re #78:
if we go back to this comment, may i suggest to add new method to Drupal\group\Plugin\Group\Relation\GroupRelationInterface which takes care of invoking exactly the kind of hooks required for each entity instead of calling $entity->save() ?
The issue with that is that we cannot know about people's implementations of those hooks. So one might very much want us to invoke the hook, whereas the other absolutely does not want us to. Then we're back at square one where we can't do things right for everyone.
Clearing caches and then expecting people to respond to the creation of a group relationship makes a lot more sense and we will no longer be doing any harm by guessing what people want.
Okay, after resolving the mix-up in my brain between ChainedFastBackend and BackendChain I actually really like this MR now. I still have some notes about spelling and docs in general, but the code looks good to me.
On a cold start we don't want several processes trying to write the same thing to APCu (or other software) thousands of times, so the 50ms (now 1s) locking mechanism prevents that. We still are able to retrieve the calculated thing from the DB cache, so it's not that bad. Over time, we will populate the fast backend properly and then we get the full benefit of it. But at least we won't be choking it when on cold caches.
The only thing I could still see is that both the 50ms and 1s window seem arbitrary. But something is definitely better than nothing here, so I suppose that's fine.
Other than the comments on the docs, I don't have any objections here. Seems like a great addition.
One thing I wondered about was whether we could have the fast backend store a random key to identify the local cache, and add that key to items in the persistent backend, and then only use last_write_timestamp when it doesn't match. This would help on single web head setups a lot, and maybe a bit with more web heads too. It would mean stuffing it into the persistent item transparently (like nesting the actual data in an array with the hash.
So each item in the persistent cache would be flagged with a key like so: ['some_key' => 'abcdef...', 'data' => $the_actual_cache_data] and upon retrieval we convert that back to $the_actual_cache_data?
But to what purpose? I'm not sure I can follow what you mean with "to identify the local cache" here.
Yeah for a second there I confused ChainedFastBackend with BackendChain where we combine a memory and database cache. That is often what we use for items that are requested multiple times per page. Okay let me revisit what I wrote and see which bits still make sense.
Also if we are to go ahead with the current approach, the class docs need updating. This bit won't hold true anymore:
* (chained) cache backend. Those cache entries that were created before the
* last write are discarded, but we use their cache IDs to then read them from
* the consistent (slower) cache backend instead; at the same time we update
* the fast cache backend so that the next read will hit the faster backend
* again. Hence we can guarantee that the cache entries we return are all
* up-to-date, and maximally exploit the faster cache backend.
It seems to me that we are limited by the $lastWriteTimestamp being shared regardless of CID. Meaning if we write 5 different items with a small delay between them, entries one through four will be considered invalid because of the fifth item being written. Why?
From the docs:
* Because this backend will mark all the cache entries in a bin as out-dated
* for each write to a bin, it is best suited to bins with fewer changes.
Isn't that bit worth reconsidering instead? If I have an expensive item I know will be requested multiple times, why would I accept it not being found in the cache because another item in the same bin so happened to got written just a few ms before my retrieval attempt? Seems like keeping a last write per CID could also go a long way here.
Gave this a review, generally like the idea and it seems promising in reducing a cache stampede's impact. However, I have some questions about whether we are profiling all the pieces involved in the process.
If I understand this correctly, we are completely eliminating fast backend lookups and writes for a whole second whenever anything is written to the persistent backend. Won't that simply move the stampede from the fast backend to the persistent backend?
We know that ChainedBackend is used for cache items that are expensive and/or requested frequently during a request. So turning off for a whole second the very thing why people use ChainedBackend seems a bit... odd?
If we are talking about higher and lower level caches being warmed, wouldn't it make more sense for the lower level caches to not use ChainedBackend if we're going to be storing calculations derived from the lower level data in a higher level cache? Talking about your example from #8.
Either way, not sure about this.
My idea was that if the submodule is small enough, I could put it in this project. I'll have a look at the link and code you provided and try to make a decision. Thanks for the info.
One thing that doesn't get solved here is fields on the relationship.
We need to figure out which fields we want to add:
- Do we add the entire relationship form (minus actions)?
- Do we scan the form for "extra" fields and only show those?
Also, do we call the relationship's form submit handler or use entity validation and manually save it?
kristiaanvandeneynde → created an issue.
That text was added to show that you now can upgrade from v2 to v3, but only if you first update v2 to 2.3.x. It does get confusing over time, I suppose.
The proposed resolution doesn't seem to be fully actionable, what would you suggest we change and where?
List is getting smaller :) Next one to tackle is probably the easy navigation entry and then "Disallow automatic membership outside of form approach". The brand-new creation UI I'm on the fence whether I want to add this now or at a later time. Still deciding...
CR added here: https://www.drupal.org/node/3526255 →
Looks good enough to me, gonna leave at NR for a few days and then commit. Will add CR now.
kristiaanvandeneynde → created an issue.
Okay, thanks for the clarification. It seems you have no objections going forward like this, so consider mine retracted.
That would fix the concern raised in #4, but it would still analyze all functions starting with the module name and "guess" that it could be a hook and add it to the container, no?
I'm asking because the initial issue title and summary were about having to put a manual stop sign in your module file and now it seems to have been repurposed entirely, not addressing the initial concern.
Okay, I've given this some thought and I'm going to move the Pathauto thingy to a hook implementation in a submodule (group_support_pathauto). I really don't like privileged code and if I were to put Pathauto stuff in GroupRelationship, then it won't be long before others start asking if they can be in there too.
That module can be part of a follow-up.
Well the issue still persists.
We are allowing people to call ModuleHandlerInterface::alter() with any number of unspecified variables. This in itself is a code smell, but for the sake of argument let's assume we want to go with the original proposal. We'd have to reduce the amount of extra parameters to 1, typehint it as an array and release a CR detailing this BC break.
That said, I'm not a fan at all of jamming all data we need to pass into a magic array either. A better way would be to find a way to be able to call specific (alter) hooks and know what these hooks' signature is. So perhaps in the future we could do something like this instead:
class MyModuleApi {
/**
* Full hook information goes here, like it would in *.api.php
*
* @param $arg1
* @param $arg2
* @param $arg3
*
* @return void
*/
public static function myAmazingHook($arg1, &$arg2, $arg3): void {
\Drupal::moduleHandler()->invokeAll('my_amazing_hook', func_get_args());
}
}
So rather than call module handler's invoke(), invokeAll(), alter(), etc. we would make those functions only to be called by these MyModuleApi classes. People who'd want to invoke a specific hook would now call MyModuleApi::myAmazingHook(1, 2, 3) instead.
This would make it so the public API has all the right type-hints and reference handles, but the internal API, i.e.: ModuleHandler, can still accept any number of arguments and loop over all hook implementations, passing said arguments along. IIRC, func_get_args() should be able to keep references on those parameters that were passed by reference. We could probably pull this off with minimal BC break too, given how we can reuse most ModuleHandler methods as is, just troublemakers like alter() would need to have an updated signature.
If there is no support for this in a few months, we can still close this. I just really dislike that we can call any (alter) hook with no real signature. It's served its purpose but we should try to do better in this modern age of php. Also, if you think this goes too far off topic of the original IS, we could close this one and open a new one.
Follow-up can be a child issue of this one: 📌 Replace direct comparisons to user 1 in core. Active
LGTM.
But now we need a follow-up as I wanted to comment here that we should also remove MigrateAccessCheck then, but realized it has more implementations. So we need a follow-up where MigrateAccessCheck is removed, because it still runs UID == 1 checks.
+1 for deprecating passing NULL instead
Issue title is perhaps slightly off as I also noticed an issue with a comment in doRenderRoot(), updating
Working on it here: 🐛 Renderer::doRender() contains some outdated information Active
kristiaanvandeneynde → created an issue.
Posted a first stab at this to see which tests fail. Also not sure about the approach and comments re Pathauto. Feels like this needs to be done in a hook or event or something, even if Pathauto won't be the one implementing it.
Going to commit this in its current shape. Can still make changes before release if the meta in core evolves regarding best practices.
I noticed the docs in doRender() were not adjusted:
// Set the bubbleable rendering metadata that has configurable defaults, if:
// - this is the root call, to ensure that the final render array definitely
// has these configurable defaults, even when no subtree is render cached.
// - this is a render cacheable subtree, to ensure that the cached data has
// the configurable defaults (which may affect the ID and invalidation).
- if ($is_root_call || isset($elements['#cache']['keys'])) {
+ if (isset($elements['#cache']['keys'])) {
They still mention that doRender() can be the root call, and from the MR it seems that doRender() should no longer be concerned with any of that. Tackle this in a tiny follow-up?
Having it check for the "access site reports" permission would also be an acceptable solution to me. The essence is that the UID 1 check needs to go and preferably be replaced with a permission check.
Declaring all your hooks as extensions of Hook would definitely solve this as we can then support a "source" property on the base Hook class that we can scan for as described in the IS. The only drawback I see is we'd be creating tons of one-of attributes. They would be contained to namespaces that have Hook in them, so I suppose that could be fine.
So maybe add an optional "source" property to Hook that we can specify in the Hook constructor as a named argument and then extensions of Hook can prefill it for you? The default value could be empty or "drupal" to indicate that it's a core hook.
Anyways, big +1 to the idea. We shouldn't add classes to the container if we know they will never be called.
The whole point of these issues is to make sure core no longer hard-codes anything based on the idea that user 1 is special. There are plans to remove user 1's special behavior in a future major Drupal release. So we really can't leave this test to rely on UID 1.
This access check explicitly allows only UID 1:
So that is what we need to change: Introduce a new permission and make the route check for said permission. Admins (including UID1) will automatically get the new permission, so a simple change record detailing that non-admins who want access need to be assigned the permission should do.
You can invalidate it, like you could already before, but I'm not even sure that's necessary or useful, as AFAIK nothing is added to that entity on load.
Right, was too quick in my previous comment. It's indeed not the entity data that triggers any changes, but the group relationship to said entity data. So we're all good on that front, you're right.
Regarding Pathauto, I see your point. Either I trigger it, or I fire an event and expect Pathauto to follow my event. Given how Pathauto has far more reported users, it might be more prudent for me to trigger Pathauto. I'll have to look into that.
Good insights, thanks.
Yes, the approach to be tried here now is the one that introduces the notion of a pseudo cache context.
🐛 Cache redirect error displayed when using 'view own unpublished content' permission alongside node grants Active landed, so the MR here can now be rewritten to remove the workaround committed there.
This is now actionable: We remove the behavior from v4 and use cache tag invalidation instead. When I first wrote Group, the entity layer still used an internal property cache and I was weary of the ramifications of only invalidating cache tags so I went with the re-save.
Years later, I feel more confident in saying that if anything relies on a given entity, it should add said entity as a dependency and therefore the invalidation of said entity should trigger a recalculation on the consumer's end.
The only thing I'm slightly concerned about is hook_entity_update() and similar entity class methods as people may use that to trigger certain things when an entity is saved and said hook does not run on a simple cache tag invalidation. So if people do something there that does not somehow get cached (with the entity as a dependency), we might be in trouble.
Then again, if it persists across requests then it is cached and it should have the dependency. If it doesn't persist, then invalidating won't hurt because the next lookup of said calculations will run them anew with the entity now being recognized as grouped.
So let's go ahead and remove the old behavior in favor of cache tag invalidation.
Could look into SDC, is there an example of applying it to entities? I don't think I have anything else in my hook_theme. The permissions UI uses an inline template for some small things.
The top level menu thing we definitely need to look into for D11.
Just re-emphasizing #32 as Nic commented on the MR where I'm about to commit the hook conversion to Group 📌 Convert hooks to OOP hooks with attributes Active :
I will not be prefixing my hook classes with my module name. I will simply be naming them EntityHooks, UserHooks, and so on. It feels really wrong to completely ignore namespaces when naming classes.
My reasoning now (I may have not always done this in the past) for adding the word Group to some classes and not to others is this: Does it have to do with the concept of Group? Then I add it. E.g.: The concept of permissions within a group is specific and not to be confused with the concept of permissions in general. So I name those classes GroupPermissionFoo. Implementing hooks has nothing to do with Group itself, it's implementing third-party API, so I choose not to add Group to the class names.
If I CMD+O in PhpStorm for "UserHooks", I see two results, but if I type "g userho" I already see what I want. Even "group Hooks" gives me everything I need if I were to search for Group's hook implementations. I see no reason to make class names extremely long if we already have all the info we need. Keep in mind some modules have really long names.
All green for next minor, so will commit after one final round of reviewing the code and threads.
While I am still a believer that the final keyword has its place, there is no point in being stubborn for the sake of being stubborn if a perfectly suitable alternative like @final presents itself. So consider me on board with the latest proposal.
Two remarks, but I don't see why this MR can't be RTBC once they are discussed/resolved.
Leaving at NR as there are no actionable items, yet. Depends on where the discussion leads us.
Also, autoplacholdering doesn't disable caching like max-age 0 does, it would still be cached on its own.
Which is why I added:
Then all we'd have to do is adjust RenderCache a bit to also check for that cache tag
But aside from that detail, I agree that making this rely on config might not be such a great idea. Your further clarifications I also tend to agree with. The suggested implementation here is one of "do not cache", but it doesn't mean that other implementations have to go down that same path.
Okay in that spirit I'll go over the MR again, but I would cautiously agree with the approach. We need to be very aware of #108, though. Ideally those issues get fixed sooner rather than later so the changes introduced here don't run havoc there, even if it's technically not "our fault" if it does.
Just adding this: I like how clean the current MR looks and I can see the reasoning behind it. But flagging the LanguageBlock as "cache optional" does not cover the load here. Because we want it to not be cached, ever. So it's hardly optional. Furthermore, we also want to placeholder it. And I can see that (placeholder & no cache) being the case for more pieces of content in the future.
Hence my suggestion for something that would both indicate we do not want to cache it and we want to placeholder it.
Quoting option B:
B) Use the new \Drupal\Core\Block\BlockPluginInterface::createPlaceholder() to force this to be a placeholder and cache it separately. In our projects, we're seeing 10k+ variants of this block, very rarely reused. That's MR !9798.
However, that MR seems to also involve CacheOptionalInterface? And I'm not sure we want to be using that here.
My reasoning is this: CacheOptionalInterface was introduced with access policies, where multiple smaller pieces will be cached together as a whole. In that context the interface meant: "If all of the smaller pieces indicate they are insignificant, we need not cache the whole."
However, render arrays with cache keys (including the language switcher block) aren't necessarily like that. They get cached individually and as part of a whole (DPC). So using the interface there seems ambiguous as it's not clear in which caching layer they would be considered optional.
Now, with that in mind, LanguageBlock should already be auto-placeholdered by virtue of its max age being 0 and BlockViewBuilder making it use a lazy builder. So setting createPlaceholder() should effectively change nothing about the current situation. The block will be placeholdered and DPC will not cache it.
So it seems the goal is to rid LanguageBlock if its max age of 0 and then make sure it doesn't poison the page. That part is not fully clear from the issue title and summary.
In that case, option B seems like the most straightforward fix, but we would indeed not want to individually cache all variations of LanguageBlock because it varies too much. So removing it's max-age 0 is a good thing, as it would otherwise kill all page caching when combined with 🐛 [pp-3] Bubbling of elements' max-age to the page's headers and the page cache Postponed .
Then again, if we merely want to placeholder it and we don't want the placeholdered HTML to be cached, we have more options than to resort to max age 0. I'm wondering if this was ever explored. Let's look at renderer.config:
renderer.config:
required_cache_contexts: ['languages:language_interface', 'theme', 'user.permissions']
auto_placeholder_conditions:
max-age: 0
contexts: ['session', 'user']
tags: []
debug: false
As you can see, we could achieve the very same result of having LanguageBlock auto-placeholdered by providing a cache tag that indicates the desired behavior. Much like the special "CACHE_MISS_IF_UNCACHEABLE_HTTP_METHOD:" cache tag we have. This tag would also bubble up to FinishResponseSubscriber, but it would have zero effect on the max-age of the page.
So rather than use CacheOptionalInterface in a way that might confuse people, why don't we introduce a "SHOULD_AUTO_PLACEHOLDER_BUT_NOT_CACHE" cache tag, add it to LanguageBlock and renderer.config.auto_placeholder_conditions.tags?
Then all we'd have to do is adjust RenderCache a bit to also check for that cache tag and we'd have a system that works for ALL render arrays (with a lazy builder) rather than having to figure out ways to apply an interface to them.
Adding my vote as I've personally seen how much he's involved in organizing multiple events for many years now:
Joris Vercammen (borisson_)
longwave → credited kristiaanvandeneynde → .
So essentially you get 1 to 1,5 year to adjust to internal deprecations and 3 to 3,5 years for public API deprecations as of 10.3?
The latter seems a bit on the high end if you ask me, but I agree that an absolute minimum of 1 year is too short for contrib and client projects to adapt to disruptive changes. Ideally, we have a 2-year window as a hard cut-off, but trying to align that with major core release dates is tricky.
Essentially you'd have to deprecate things right out the gate when a new major release is cut, which seems impossible unless you postponed deprecations from the last major. And that would essentially be the same deprecating over 2-4 years, but with extra steps.
So with that in mind I suppose RTBC +1, even though I'm not fully a fan of keeping dead weight around for more than 3 years.
Thanks for confirming!
Tests are failing on me forgetting to convert tokens.inc and views.inc, also need to tackle submodules still.
Hmm, reading more into it, this is exactly what OPT_IN_TEST_NEXT_MINOR is for.
https://git.drupalcode.org/project/gitlab_templates/-/blob/main/includes...
kristiaanvandeneynde → created an issue.
Having to put a manual stop sign in several files in your module seems a bit tedious and counter-intuitive. Nothing seems to happen if you don't, so I can imagine many developers simply not knowing about this memory optimization.
I like that you can flag your module as "I've converted all my hooks" in one place (services file), though. We can scan for said flag when procedural hooks are gone and then give a little nudge saying you can now delete the container param.
So from what little understanding I have from trying to implement hooks in Group and having a cursory read of HookCollectorPass, I tend to agree with Berdir that we can do better here and significantly reduce the fallout if someone were to forget to add the stop signs. Then we should evaluate as a whole whether we cannot do away with the stop signs altogether.
Pushed a commit that uses hook ordering and the new theme initial preprocess callbacks. The entire module file is gone now and I've bumped minimum required version to 11.2. That last bit may make tests fail as we don't have an official 11.2 release yet.
kristiaanvandeneynde → changed the visibility of the branch 3522723-remove-plugin-definitions to hidden.
Never mind, this was fixed in HEAD by 📌 Bump PHPStan to version 2.0.0 Active
kristiaanvandeneynde → created an issue.
Thanks all!
Generally, the approach is to commit to the target branch first and then the other active branches, in the same issue. Please don't open new issues for porting MRs as it makes my huge issue queue even larger.
Will do final check and commit on Friday probably, maybe early next week.
All green on the Group issue when using 'main' branch of gitlab_templates :)
Okay, pushed a commit with those instructions.
I managed to make the tests go green using the workaround, so that seems to confirm my findings. Is there a way for me to test this on a specific Group issue MR?
kristiaanvandeneynde → created an issue.
We definitely want to keep the module name as part of it, finding the right EntityHooks file out of dozens in a project will be a massive pain.
I'm not sure about that. I tend to press the shortcut to find classes in PhpStorm and then type e.g.: "group Ent" and it would show me classes starting with Ent in the group namespace. I feel that adding your own module name to your classes is a bit redundant and leads to classes being harder to find. Namespaces are your friend here and I'm sure most IDEs support them in their search.
Gave some preliminary feedback on Slack, if it's not enough then we can quickly have a look in person in Leuven a week and a half from now. My only concern is that a too loose validation would allow config to contain values that the code does not handle well (or at all). So as long as that concern is taken care of, I'm fine with this.
Added follow-up here: 📌 Decide if we need to load #type defaults for #pre_render callbacks Active
So setting back to RTBC.
kristiaanvandeneynde → created an issue.
I just had another look and while I agree that part of ::getValidatedCachedRedirectChain() rehashes what we already commented elsewhere, I don't think it hurts here to be so verbose. VariationCache is becoming quite complex and the comments may help understand it.
As for the "you can" vs "See xy", I'll leave that up to committers to split hairs over :) I'm also not a native speaker but I try to do my best to write proper English in my comments, yet might fail sometimes.
if it's all the same to you, I'll leave the MR as-is now.
We initially counted all database queries in performance tests, but this caused random test failures with the chained fast backend because it uses timestamps for whether an item is valid, so millisecond rounding issues can result in an item being valid or not. For me personally I think the current split by cache bins is good - we know implicitly which bins use chained fast or not.
Right, almost forgot about how annoying we were to everyone else with our performance tests 😅 Agreed that we can leave this as is.
I'll see if I can comb over the MR one more time to find places to improve the comments. VariationCache has become a lot more complex with the addition of getMultiple() and the internal cache now, so maybe having this many comments isn't a bad thing?
Just wanted to add for posterity that this is all a slightly convoluted alternative to making the render cache a chained backend with a memory cache in front. However, that would have several drawbacks:
- We have to use MemoryBackend rather than MemoryCache or otherwise the render cache would no longer be a true cache. MemoryBackend is slow due to all the (un)serializing
- We might not see any benefit in our performance tests, or even worse performance, because we don't differentiate between DB gets and memory gets. Maybe we should?
- It would needlessly check all cache redirects for invalidation, even though they don't need to be invalidated. For now... some might if we land PseudoCacheContext from 🐛 Cache redirect error displayed when using 'view own unpublished content' permission alongside node grants Active
So all in all, given how crucial the render VariationCache is to a page's performance, I think it's a good thing we came up with a custom solution here.
That reduced the cache GETs by 3, probably fixing what you found in #35
Pushing something wild.
Rather than a simple "is the entire chain still valid", I'm swapping it out with "give me the part of the chain that is still valid". This should fix the false invalid result on chains that led to a cache HIT before, but also immediately optimize scenario's where an entire chain was deemed invalid if cache contexts changed value mid-request, even though we may have been able to reuse the start of the chain.
Oh, right. I will amend that, good catch!
But because we don't persist the cache hit in the $chain, this will never be TRUE for any lookups that were a HIT, it only works for misses.
That was by design, though, and discussed previously. We cannot persist the cache HIT as it may get invalidated and our redirect chain cache would have no clue that it did. Tests were even failing because of that because at first I did store cache hits in the cached redirect chain.
Off the top of my head: Don't grants also not apply if an access hook already gave a definitive answer?
Just noticed, fixed it.
catch → credited kristiaanvandeneynde → .
Done
It is. It's the new API that we introduced here.
The CachedStrategy itself isn't that CR-worthy (IMO) as it is merely an optimization.