Abstract RenderCache into a separate service that is capable of cache redirects in a non-render array-specific way

Hiding patches as we use a merge request now.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Are we still waiting for the change in https://www.drupal.org/project/drupal/issues/3082032 🐛 ToolbarController::preRenderGetRenderedSubtrees() deletes parent's cacheability. Fixed , or what are the next steps here? Looks like all of @catch's remarks have been resolved. I now feel more confident about this patch compared to #133 almost 5 years ago.

🐛 ToolbarController::preRenderGetRenderedSubtrees() deletes parent's cacheability. Fixed is fixed.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Not sure why the MR keeps saying it needs to be rebased even after I merged in 10.1.x like I did last two times.

There's few big commits arrived today so it needs another rebase/merge, but there's a bug https://gitlab.com/gitlab-org/gitlab/-/issues/407115

left review

Updated the change record.

I have used an ab-like tool to check if there is a difference in page response times. I have done this on my local machine with umami and a couple of extra blocks enabled on the homepage. This difference that we see here is super small, I think this is more likely to be something else that ran on my machine rather than the patch actually making it slower.

Based on this, and the discussions with @kristiaanvandeneynde this looks great.

WITHOUT PATCH
-------- Results --------

Successful calls    		100
Total time          		10.8378 s
Average             		0.1084 s
Fastest             		0.0892 s
Slowest             		0.3559 s
Amplitude           		0.2667 s
Standard deviation  		0.037784
Requests Per Second 		9.23
Requests Per Minute 		553.62

WITH PATCH
-------- Results --------

Successful calls    		100
Total time          		10.2614 s
Average             		0.1026 s
Fastest             		0.0266 s
Slowest             		0.2502 s
Amplitude           		0.2236 s
Standard deviation  		0.036465
Requests Per Second 		9.75
Requests Per Minute 		584.71

Setting this to rtbc

Actually setting to rtbc.

Woot, thanks for the performance review @borisson_! Ever since Blackfire changed, I haven't found the time to work on another performance test. So thanks a million.

Looks like the patch actually makes it slightly faster too? Could also be down to small differences on your machine as you noted.

Made a couple of small commits to resolve unresolved threads. If I haven't broken anything I'm hoping to commit this to 11.x/10.2.x later today or tomorrow.

Tagging needs follow-up for using this in workspaces per #2551419-13: Abstract RenderCache into a separate service that is capable of cache redirects in a non-render array-specific way →

Committed 339643b and pushed to 11.x. Thanks!

After doing that, I realised that we should have an extra change record for the new API and a release note with some information about it, to hopefully encourage contrib take-up where it's applicable.

Moving to needs work for that, but nice to finally land this one!

Opened a small followup 🐛 VariationCache deprecations should refer to 10.2.0 Closed: duplicate

Wow, this is huuuuuge 🎉

After doing that, I realised that we should have an extra change record for the new API and a release note with some information about it, to hopefully encourage contrib take-up where it's applicable.

I'll try to draft a change record ASAP.

Draft change record here: https://www.drupal.org/node/3365546 → Also, I flagged it as 10.2.x but did not see a commit for that (yet?), hope I didn't make a mistake with the version on the CR.

Now I just need to figure out a way to seamlessly allow people to transition from contrib VariationCache to core VariationCache. I was thinking the following:

• As soon as core 10.2.0 lands, I make sure there's a version of contrib VariationCache where the core dependency is 10.2.x and up and a version where the max core version is 10.1.x
• The one going until 10.1.x is simply the current version, the one starting from 10.2.x has every class as an extension of the new corresponding core class
• As long as we support Drupal versions smaller than 10.2.x, we remain this way and keep our modules (i.e. Group) relying on contrib VariationCache
• Once the minimum supported version for core is 10.2.x, we create a new release of Group where we directly call the core classes and drop the contrib dependency.

Would that work? AFAICR, having the same service name in contrib as a core service simply replaces the core one with the contrib one, so we should be fine?

If this needs further discussion, I'm definitely open to take it up on Slack or another D.O issue.

See https://www.drupal.org/about/core/blog/new-drupal-core-branching-scheme-... → for why there's no 10.2.x commit, don't worry it'll be in there :)

The plan for the contrib module in #209 sounds like it will work - should probably be an issue against variationcache for that?

Change record looks like a good start. Moving this back to fixed.

I re-reread the workspaces discussion linked from #13, and the issue that we wanted to fix got fixed via memory cache, so removing the needs follow-up tag.

Added a release notes snippet.

Aha, that was a good read. Now I get why it's committed to 11.x

Thanks for the feedback on #209

I wouldn't rely on your services automatically replace the ones from core, that depends on order in which the files are loaded. I'd do a ServiceProvider and alter them.

One problem is that you can't retroactively change core compatibility of old releases. If you do a new minor release that is not compatible with 10.2 and a new 2.x major version that requires 10.2, composer will by default just downgrade to the previous 1.x release as most people will have version constraint like ^1 in place. They will need to manually update that, assuming they directly require it. If not, then composer will update them to 2.x.

You could see if there's a way to not do a new major version, but support both earlier and newer versions in one release. You won't be able to extend from the core classes then, you'd still need to duplicate everything and either replace or define the services. You might be able to do some class alias trickery to extend from core classes if they exist, the way phpunit/twig and similar BC compatibility sometimes works. If you don't, you'd risk breaking code that relies on the new 10.2 interface.

Epic! 🤩

Congratulations and thank you, @kristiaanvandeneynde!

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

Is there reason why no change record published?

Hmm good point, they're still in draft status

Re #215 That's common issue on drupal.org.
Filed a ticket. ✨ Publish change records when issues are fixed Postponed

I've gone ahead and published both CRs