Mark RenderCache as internal

This sounds like a novice task? It reads as if all you need is to ensure those cache contexts.

Copy Paste mistake ):

When I used the render cache directly I had to deal also on the get() level so yeah tests would be nice.

Reroll + one line change to make unit tests pass.

Clarify issue.

+++ b/core/lib/Drupal/Core/Render/RenderCache.php
@@ -207,9 +217,11 @@ public function set(array &$elements, array $pre_bubbling_elements) {
-      $cache_contexts = $data['#cache']['contexts'];
+      $cache_contexts = Cache::mergeContexts($data['#cache']['contexts'], $required_cache_contexts);

This is wrong; it should merge the required cache contexts with those in $data. Otherwise the render cache item, when retrieved from cache and used in something else, will not bubble up the required cache context.

Of course… one could reason that required cache contexts should not ever be exposed, and should indeed be added this way: at the very last moment, without them showing up while debugging anywhere else in the render pipeline.

But, the reasoning that we've applied in the past is: "less magic, easier debugging". That's why Renderer::doRender() does do what I described above.

Second: this will introduce some overhead/performance penalty, because it will mean required cache contexts are added both in the Render and in the RenderCache services.
That is also the reason why RenderCache does not yet deal with required cache contexts in HEAD, which is exactly what this issue is trying to change.

The only reason this issue wants to change that is for the very few, very rare occasions where code is using the RenderCache service directly. In fact, #2450897: Cache Field views row output was the only occurrence of this. (BTW while trying to find more usages, I also found #2507459: CachePluginBase and subclasses no longer need Renderer/RenderCache services injected.)

But, actually… that also doesn't use the RenderCache service directly! See #2450897-118: Cache Field views row output and related patches: […] we found another approach here where we can get back to the nicer #pre_render pattern. Which means nothing actually directly talks to the RenderCache service.

Which brings me to my proposal (which is the conclusion I've come to while writing this comment): let's make the RenderCache service private. That just enforces what we already know: just use the Renderer service. That's what turned out to make most sense. This issue is a "bug" or a "problem" only if you want to use this service directly. But that doesn't make a lot of sense. It's tightly coupled to the Renderer. It helps make Renderer more understandable. Let's keep it that way. And thus let's make this service private instead.

(Fun detail: this would be nicely symmetrical with FormBuilder and FormCache: FormCache is private to FormBuilder too!)

let's make the RenderCache service private.

Its unusable anyway, but well, there are a lot of tests in views relying on it, if I remember correctly.

There is still the buggy behavior however that is justifying the move.

There's no actual bug as far RenderCache is concerned. It just does what is told, works with what it is given.

Fact is that we will always need this in Renderer. Which means we'll be doing the same work twice. Why not KISS? We can still choose to make RenderCache a public service later.

Finally, green patch :)

Just a rebase, conflicts automatically resolved by git. Still applies

I think it's too late to do this now?

Well seems so. We should at least mark it as internal?

+1.

A rare case of PIFR being green and DrupalCI being red. Which is right?

UpdatestUpdateImportSourceRemote
fail: [Other] Line 144 of core/modules/locale/src/Tests/LocaleUpdateTest.php:
Updates for Contrib module one

Seems like DrupalCI is drunk for once. Re-testing.

I think marking something @internal during RC is okay, but check with a committer when adding the tag (and mention who you talked to and the reasoning). Cheers!

Committed and pushed to 8.0.x.