Correctly handle cache data instead of throwing an Exception in EarlyRenderingControllerWrapperSubscriber()

So yeah instead of the exception we could maybe provide some logging or warning, which at least doesn't have any runtime potential breakage.

* In Monitoring in a rest service, I'm calling hook_requirements() and because system_requirements() creates a URL for the cron link, I have to add 10+ lines of code to execute it in my own render execution wrapper. I've also helped 2+ people in IRC that did run into similar problems

For some reasons I even had issues generating some URL in tests, which is, pretty crazy, to say the least.

In general we need a general improvement of the DX of the non trivial bits of the rendering layer.

• First: I totally agree this is a pain.
• Second: the entire reason we did this (#2450993: Rendered Cache Metadata created during the main controller request gets lost) is because we decided to still allow early rendering in controllers, we considered that too big of a change to introduce. I personally think it'd have been much better DX-wise if we'd simply have disallowed early rendering in controllers. This was the consensus.
• Third: the problem is of course made worse by the automatic bubbling of bubbleable metadata in the URL generator. The reason we had to do that is because the pre-existing implementation of the URL generator was relying on globals (in Drupal 7) and doing all sorts of context- and configuration-dependent URL alterations (Drupal 7 & 8), which could cause serious security problems (#2351015: URL generation does not bubble cache contexts + #2335661: Outbound path & route processors must specify cacheability metadata).

The problem described in #2626298: REST module must cache only responses to GET requests, which can be generalized to sending an e-mail from any random place in the code should not trigger exceptions just points to plain sloppiness in code. It doesn't make sense to do blocking I/O very very deep in function call stacks. E-mails should be generated and sent in a queue/job runner/batch/cron job/something that does it outside of the request/response flow. And if you really want to do that, then you just have to wrap the sending of an e-mail in a render context.

* In Monitoring in a rest service, I'm calling hook_requirements() and because system_requirements() creates a URL for the cron link, I have to add 10+ lines of code to execute it in my own render execution wrapper. I've also helped 2+ people in IRC that did run into similar problems

Well, yes, if you're calling hooks, and particularly hooks that are full of legacy code, then you indeed need to execute it in its own render context. Those hook implementations rely on global state. If they had their dependencies injected, state/context isolated, this would not be necessary. So, yes, when you need to call poorly written code from your controller, it's better to execute that in its own render context.
The exception is only thrown in the first place if the response meets this condition: $response instanceof AttachmentsInterface || $response instanceof CacheableResponseInterface || $response instanceof CacheableDependencyInterface. Which means it's capable of having cacheability metadata. Which means it may be cached. Which means that if it includes the return value of hook_requirements(), that the response may need to vary by the cache context that the hook_requirements() invocation bubbled. Otherwise you'll get incorrect results.

Yes, all of this is annoying. Yes, I wish we wouldn't have this wrapper. But it's there for correctness.

* In a payment response processing, I'm saving a node, that node is indexed by search API, and then the solr backend generates a URL to figure out the current domain, so it can index for that. I couldn't care less what it does there, but it completely breaks my page and I have no way of fixing that except changing search_api_solr or executing my node save in such a render execution wrapper.

It seems fishy to me that reindexing for search is happening in the same request. It appears that should also be done out-of-band, i.e. through some queue/cron job/…. And if not, then yes, it should be wrapped in a render context.

All of these examples share one theme: they are about doing things other than creating a response, that happen to be triggered due to historical architectural choices.

I totally agree it's annoying. I totally agree it's unfortunate. I totally wish it could've been avoided. But this was necessary for correctness.

Therefore, I very strongly disagree with:

Remove the exception, just add the cacheability metadata, or ignore it with a warning log message or so if that's not possible.

… but I'd be happy to improve the exception message or help the developer in some other way with tracking down the root cause.

Berdir replied in IRC:

<berdir> WimLeers: I don't get it. You have the render context there. And you *have* the cacheability metadata available. Why not just use it? We are relying on the fact that we *can* catch that metadata there. If we couldn't, we also couldn't detect that something is missing

See:

• #2450993-108: Rendered Cache Metadata created during the main controller request gets lost
• #2450993-114: Rendered Cache Metadata created during the main controller request gets lost
• #2450993-123: Rendered Cache Metadata created during the main controller request gets lost
• #2450993-127: Rendered Cache Metadata created during the main controller request gets lost
• #2450993-133: Rendered Cache Metadata created during the main controller request gets lost

I totally agree that this is a MAJOR pain point. Not necessarily for core/contrib development but for actual sites and custom code. Its so easy to have leaking of metadata that you don't even see where this is coming from, especially because enabling a module can cause code to start leaking metadata. For example every node_access module causes additional leaking via #2729137: NodeStorage::loadByProperties should avoid adding node access query tags for example.

Something like

In order to debug the problem, first figure out which cache contexts/tags are added to $early_rendering_bubbleable_metadata.
The next step is to figure out which code adds those. Therefore look at the backtrace in \Drupal\Core\Render\RenderContext::update. For better experience, just look at the backtraces which include your leaking cache contexts / tags.

this would help everyone to at least get started.

I think to start with should trigger_error() here rather than throw an exception. This happens due to interactions between code which might work independently when being developed, but not when combined on a site, with #2729137: NodeStorage::loadByProperties should avoid adding node access query tags being a good example.

I also ran into the same problem recently, and yes, rather painful to debug. During a POST request too.

...just points to plain sloppiness in code. It doesn't make sense to do blocking I/O very very deep in function call stacks. E-mails should be generated and sent in a queue/job runner/batch/cron job/something that does it outside of the request/response flow

It's totally feasible that someone would just send an email after some action during a request. Not all sites will be implementing queues and such. It is certainly not necessarily indicative of 'sloppiness' in code. Doing stuff like this has not been a problem for years. I don't think anyone would assume this would cause a problem with their response.

I agree with berdir here. Assuming that we will only ever be doing things to do with generating the response is unrealistic.

Could this be mitigated by doing things like e.g. executing mail() calls inside their own render context? Conceptually it's questionable but it seems like that ship has sailed and the DX tradeoff is worth it.

I had a feeling there might already be an issue like that, thanks berdir.

#2744715: Generated URLs should only have cache contexts on GET/HEAD.

URL generation had a rocky ride during 8.x to say the least, one aspect of which was its relationship to the render system in general.

There are not that many places we added cacheability metadata implicitly, so one or two more patches like this ought to cut things down a lot.

I also ran into this problem. Same situation. I was confused that there was one debugging hint a 'url.site' was added to the cache context. Then, I read #12, and commented out my URI related generation feature, and the problem was gone.

Can you reference an example snippet where you do the following?

the workaround is pretty arcane and requires you to trick the system: You claim to handle cacheability metadata yourself but then ignore it.

This workaround was effective:

   $options['absolute'] = TRUE;
    $url = $this->getUrlGenerator()->generateFromRoute('entity.node.canonical', ['node' => $nid], $options, TRUE);
    $response = new CacheableRedirectResponse($url->getGeneratedUrl());

Happened to me in a controller where I used the output of Entity::toUrl in building a redirect. I agree with @berdir: the biggest problem here is the unhelpful message! Secondarily, it doesn't make any sense to fatal on a recoverable problem. This should be a warning level log message IMO. +1 for trigger_error .

For anyone running into this again: you are receiving this fatal because you use a method which counts as rendering, without adding its cache metadata to the render context. The common case (that I keep bumping into, at least), is if you're generating a Url string somewhere. Rendering a Url counts as rendering, and has cache metadata that you should preserve. Instead of:

    $url = $node->toUrl()->toString();

Use:

    $url = $node->toUrl()->toString(TRUE);
    // The generated URL string, as before.
    $url_string = $url->getGeneratedUrl();
    // Add the $url object as a dependency of whatever you're returning. Probably a response.
   $response = new CacheableResponse($url_string, Response::HTTP_OK);
   $response->addCacheableDependency($url);
   return $response;

Or if you're doing something that's uncacheable anyways, just passing TRUE to toString() claims that you're handling the metadata, and will stop the error. So technically $node->toUrl()->toString(TRUE)->getGeneratedUrl() is enough.

I had an issue from calling normalize on an entity. Current fix I am using is to wrap it ala other patches (posting for #lazyweb):

    $data = $this->renderer->executeInRenderContext(new RenderContext(), function () use ($user) {
      return $this->serializer->normalize($user);
    });

Ran into this same issue because we were generating image style URLs as part of a function that was being called to assemble a much larger JSON response. It was extremely time consuming to track down due to the unhelpful error message. Solutions on Stack Overflow that mention the error will not work in general. The only workaround that was viable for our situation without having to do a massive amount of refactoring was already suggested in #20 - using another render context. Unsure of the performance implications of doing this any time we need to generate a URL

A better error message could at least help people determine what needs to be done in a separate render context without having to trace through dozens and dozens of core classes. Is this issue still getting any attention?

Due to the growing trend of decoupled web apps, I guess this issue is increasing in its relevance, as more developers might try to build a "simple" controller delivering some rendered content as Json feeds. I guess there's more documentation needed, both regards that exception mentioned here and how to properly build Json feed controllers (yep, devs might still want to write their own).

Hi @Berdir, I assume you still prefer to remove the exception alltogether or would #12 make sense to fix these issues?

Referencing another issue that seems to be related: RedirectDestination->getAsArray()/get() leaks cacheablity metadata.

This error is probably the biggest DX pain for me at the moment, quite obtuse error msg and takes hours to debug and fix.
Currently have this problem loading a comment in a REST response, but only if the comment has a parent comment...
Doesn't help adding cache tags for both the comment and the parent comment, I guess something else related to the render, probably url...
Would be nice if this could be fixed!

Added workaround section adding #19 and #20 plus the excellent debug/workaround blog by Matt Oliveira https://www.lullabot.com/articles/early-rendering-a-lesson-in-debugging-...

How do you workaround this issue when POSTing to the JSON:API from outside applications? Anytime I POST to a route like /jsonapi/{node}/*, I get this error. The POST is successful, but the error is logged. I'm POSTing and PATCHing 1000s of nodes nightly, so it's unacceptable to wake up to a 3.5 gigabyte log every morning.

g.jordan[]uky.edu - at the moment, you can only fix the issue by fixing whatever code is responsible for indirectly generating the error.

davidwhthomas - do you still get this error? I can match your description about parent comment, to code I see in rdf_comment_storage_load() and which I've probably addressed in #2335487: Remove the multiple comment optimization from rdf.module. I can't get core to throw the exception by itself, so I am guessing you have custom code doing this. If you can create a test (or alternatively upload a stripped-down version of your controller which I could make into a test) to contribute to that issue... it has a better chance of going RTBC.

I have a custom REST endpoint that's creates a couple content nodes and then returns a very basic response but I am now getting this error. I had this part of the project working a month ago and I would have done a Drupal 8 core update since then so I'm thinking there was a core change that now does not like my REST Response.

$response_status = ['status' => 200];
$response = new ResourceResponse($response_status);

LogicException: The controller result claims to be providing relevant cache metadata, but leaked metadata was detected. Please ensure you are not rendering content too early. Returned object class: Drupal\rest\ResourceResponse. in Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->wrapControllerExecutionInRenderContext() (line 154 of /home/onlinetech/public_html/web/core/lib/Drupal/Core/EventSubscriber/EarlyRenderingControllerWrapperSubscriber.php).

I've checked and am not rendering anything in the code and even removed this code in case that was triggering a render internally with no change

    $url = Url::fromRoute('gbstudent-profile.edit', ['node' => $student_node->id()]);
    $token = \Drupal::csrfToken()->get($url->getInternalPath());

What should I be doing to return a basic array response?

Well I found a solution but definitely went down the rabbit hole on this one. Another useful blog article was https://blog.dcycle.com/blog/2018-01-24/caching-drupal-8-rest-resource/ which was helpful but it also linked to other resources including https://spinningcode.org/2017/05/cached-json-responses-in-drupal-8 which is much closer to what I needed.

In the end this was all I needed

use ...
use Drupal\Core\Cache\CacheableJsonResponse;

class StudentRegisterResource extends ResourceBase {

     .. other methods as needed

    public function post($post_data) {
     $context = new RenderContext();
     $response_with_context = \Drupal::service('renderer')->executeInRenderContext($context, function () use ($post_data) {
           ..... code that creates multiple nodes based on $post_date from remote application

           // Prepare response 
           $response_status = [
               'status' => 200,
               'student_id' => $student_node->id(),
                'confirmation_access_token' => $token,
            ];

            return $response_status;
      }

     $response = CacheableJsonResponse::create($response_with_context);
     return $response;
   }
}

So the summary was not clear enough. There is CR 2513810 All rendering must happen in a render context, early rendering's metadata no longer lost

As we having a similar problem then discussed in the last comments I created now a follow up ticket (#3080634: Catch EarlyRendering Exception for REST-Resources) for REST calls explicitly (extending ResourceBase), as this seems a special pain point there.

With the attached patch there its not need anymore to always wrap everything around executeInRenderContext().

This might be a very stupid question, but why instead of throwing an exception we don't retrieve the cacheable metadata from the context and add it to the response before returning it? Wouldn't that let the page and the dynamic page cache to properly cache the response?

According to the change record 2513810 All rendering must happen in a render context, early rendering's metadata no longer lostthis should be possible automatically, but we want that

Since you're returning responses, you want to fully control what is sent, so you should also be a responsible citizen and not do any early rendering.

But it looks like there are cases where it is not possible to prevent early rendering and one cannot really add a wrapper around so many places where this could happen like the one described in #3080634: Catch EarlyRendering Exception for REST-Resources:

Doing a query which is altered, which adds a cache context (see node_query_node_access_alter)

From #36 :

This might be a very stupid question, but why instead of throwing an exception we don't retrieve the cacheable metadata from the context and add it to the response before returning it? Wouldn't that let the page and the dynamic page cache to properly cache the response?

I also thought of that lately, wondering why it's not the case. I hope it's not a so stupid suggestion so I decided to take that route (for CacheableResponseInterface in my case as it is in JSON:API use case).
I'm hoping it'll solves issues related to the combined use of advanced node permission (i.e. node access records & grants) alongside JSON:API which is currently kinda broken. (see #3077302: JSON:API Logic exception caused by leaked metadata, #3040603: QueryAccessHandlerBase should document a workaround for leaking metadata, #3028976: Enable an entity query's return value to carry cacheability & possibly others)

Here's a patch in case you want to try it out.

Instead of the patch in #37, which was a quick & dirty attempt to find a workaround to be able to "go live" on some of our projects, I came up with an independent contrib which does the same thing without hacking the core.
It's a more elegant way of solving the issue for most of our JSON:API based decoupled project (YMMV) while preserving the conservative behavior of the core.

https://www.drupal.org/project/jsonapi_earlyrendering_workaround

Hi @garphy,

Thank you ever so much for the module workaround. It works for me.
Drupal Version
8.7.8

@garphy, awesome... patch solution works for me in drupal8.9.3... but patch was not applied.
I was facing issue while creating node of a particular content type via JSON:API, for that content type there was some playaround with drupal cache tags.

For other content type JSON:API was working fine while creating node.
Thanks a lot..!!!!

Error was:

"title": "Internal Server Error",
            "status": "500",
            "detail": "The controller result claims to be providing relevant cache metadata, but leaked metadata was detected. Please ensure you are not rendering content too early. Returned object class: Drupal\\jsonapi\\ResourceResponse.",

More detailed:
"exception": "LogicException: The controller result claims to be providing relevant cache metadata, but leaked metadata was detected. Please ensure you are not rendering content too early. Returned object class: Drupal\\jsonapi\\ResourceResponse. in {{project}}/core/lib/Drupal/Core/EventSubscriber/EarlyRenderingControllerWrapperSubscriber.php:154\nStack trace:\n#0 {{project}}/core/lib/Drupal/Core/EventSubscriber/EarlyRenderingControllerWrapperSubscriber.php(97): Drupal\\Core\\EventSubscriber\\EarlyRenderingControllerWrapperSubscriber->wrapControllerExecutionInRenderContext(Array, Array)\n#1 {{project}}/vendor/symfony/http-kernel/HttpKernel.php(151): Drupal\\Core\\EventSubscriber\\EarlyRenderingControllerWrapperSubscriber->Drupal\\Core\\EventSubscriber\\{closure}()\n#2 {{project}}/vendor/symfony/http-kernel/HttpKernel.php(68): Symfony\\Component\\HttpKernel\\HttpKernel->handleRaw(Object(Symfony\\Component\\HttpFoundation\\Request), 1)\n#3 {{project}}/core/lib/Drupal/Core/StackMiddleware/Session.php(57): Symfony\\Component\\HttpKernel\\HttpKernel->handle(Object(Symfony\\Component\\HttpFoundation\\Request), 1, true)\n#4 {{project}}/core/lib/Drupal/Core/StackMiddleware/KernelPreHandle.php(47): Drupal\\Core\\StackMiddleware\\Session->handle(Object(Symfony\\Component\\HttpFoundation\\Request), 1, true)\n#5 {{project}}/core/modules/page_cache/src/StackMiddleware/PageCache.php(106): Drupal\\Core\\StackMiddleware\\KernelPreHandle->handle(Object(Symfony\\Component\\HttpFoundation\\Request), 1, true)\n#6 {{project}}/core/modules/page_cache/src/StackMiddleware/PageCache.php(85): Drupal\\page_cache\\StackMiddleware\\PageCache->pass(Object(Symfony\\Component\\HttpFoundation\\Request), 1, true)\n#7 {{project}}/core/lib/Drupal/Core/StackMiddleware/ReverseProxyMiddleware.php(47): Drupal\\page_cache\\StackMiddleware\\PageCache->handle(Object(Symfony\\Component\\HttpFoundation\\Request), 1, true)\n#8 {{project}}/core/lib/Drupal/Core/StackMiddleware/NegotiationMiddleware.php(52): Drupal\\Core\\StackMiddleware\\ReverseProxyMiddleware->handle(Object(Symfony\\Component\\HttpFoundation\\Request), 1, true)\n#9 {{project}}/vendor/stack/builder/src/Stack/StackedHttpKernel.php(23): Drupal\\Core\\StackMiddleware\\NegotiationMiddleware->handle(Object(Symfony\\Component\\HttpFoundation\\Request), 1, true)\n#10 {{project}}/core/lib/Drupal/Core/DrupalKernel.php(708): Stack\\StackedHttpKernel->handle(Object(Symfony\\Component\\HttpFoundation\\Request), 1, true)\n#11 {{project}}/index.php(19): Drupal\\Core\\DrupalKernel->handle(Object(Symfony\\Component\\HttpFoundation\\Request))\n#12 {main}\n\nNext Symfony\\Component\\HttpKernel\\Exception\\HttpException: The controller result claims to be providing relevant cache metadata, but leaked metadata was detected. Please ensure you are not rendering content too early. Returned object class: Drupal\\jsonapi\\ResourceResponse. in {{project}}/core/modules/jsonapi/src/EventSubscriber/DefaultExceptionSubscriber.php:48\nStack trace:\n#0 [internal function]: Drupal\\jsonapi\\EventSubscriber\\DefaultExceptionSubscriber->onException(Object(Symfony\\Component\\HttpKernel\\Event\\GetResponseForExceptionEvent), 'kernel.exceptio...', Object(Drupal\\Component\\EventDispatcher\\ContainerAwareEventDispatcher))\n#1 {{project}}/core/lib/Drupal/Component/EventDispatcher/ContainerAwareEventDispatcher.php(111): call_user_func(Array, Object(Symfony\\Component\\HttpKernel\\Event\\GetResponseForExceptionEvent), 'kernel.exceptio...', Object(Drupal\\Component\\EventDispatcher\\ContainerAwareEventDispatcher))\n#2 {{project}}/vendor/symfony/http-kernel/HttpKernel.php(227): Drupal\\Component\\EventDispatcher\\ContainerAwareEventDispatcher->dispatch('kernel.exceptio...', Object(Symfony\\Component\\HttpKernel\\Event\\GetResponseForExceptionEvent))\n#3 {{project}}/vendor/symfony/http-kernel/HttpKernel.php(79): Symfony\\Component\\HttpKernel\\HttpKernel->handleException(Object(LogicException), Object(Symfony\\Component\\HttpFoundation\\Request), 1)\n#4 {{project}}/core/lib/Drupal/Core/StackMiddleware/Session.php(57): Symfony\\Component\\HttpKernel\\HttpKernel->handle(Object(Symfony\\Component\\HttpFoundation\\Request), 1, true)\n#5 {{project}}/core/lib/Drupal/Core/StackMiddleware/KernelPreHandle.php(47): Drupal\\Core\\StackMiddleware\\Session->handle(Object(Symfony\\Component\\HttpFoundation\\Request), 1, true)\n#6 {{project}}/core/modules/page_cache/src/StackMiddleware/PageCache.php(106): Drupal\\Core\\StackMiddleware\\KernelPreHandle->handle(Object(Symfony\\Component\\HttpFoundation\\Request), 1, true)\n#7 {{project}}/core/modules/page_cache/src/StackMiddleware/PageCache.php(85): Drupal\\page_cache\\StackMiddleware\\PageCache->pass(Object(Symfony\\Component\\HttpFoundation\\Request), 1, true)\n#8 {{project}}/core/lib/Drupal/Core/StackMiddleware/ReverseProxyMiddleware.php(47): Drupal\\page_cache\\StackMiddleware\\PageCache->handle(Object(Symfony\\Component\\HttpFoundation\\Request), 1, true)\n#9 {{project}}/core/lib/Drupal/Core/StackMiddleware/NegotiationMiddleware.php(52): Drupal\\Core\\StackMiddleware\\ReverseProxyMiddleware->handle(Object(Symfony\\Component\\HttpFoundation\\Request), 1, true)\n#10 {{project}}/vendor/stack/builder/src/Stack/StackedHttpKernel.php(23): Drupal\\Core\\StackMiddleware\\NegotiationMiddleware->handle(Object(Symfony\\Component\\HttpFoundation\\Request), 1, true)\n#11 {{project}}/core/lib/Drupal/Core/DrupalKernel.php(708): Stack\\StackedHttpKernel->handle(Object(Symfony\\Component\\HttpFoundation\\Request), 1, true)\n#12 {{project}}/index.php(19): Drupal\\Core\\DrupalKernel->handle(Object(Symfony\\Component\\HttpFoundation\\Request))\n#13 {main}"

Adding re-rolled patch from comment #37 for drupal 9.1.x

#37 works for me, thanks for that

As a followup on generating URLs for responses:

Use:

    $url = $node->toUrl()->toString(TRUE);
    $url_string = $url->getGeneratedUrl();
    $response = new CacheableResponse($url_string, Response::HTTP_OK);
   (...)

I figured this would always be safe. As it turns out, even though the code itself is good, a toString() call can cause code in other modules to be invoked... which can do unsafe things.
Example: have content_moderation enabled, upgrade to Rules 8.x-3.0-alpha6 -> the above starts throwing the "early rendering" exception (because Rules does an Url::fromRoute(...)->toString() during your call to toString(TRUE), and leaks metadata). #3161036-6: CurrentPathContext inadvertently bubbles up 'route' cache context

This makes things more tedious: to fully protect against 'outside' code causing the exception, even all Url::from*() and Url::toString() calls need to be wrapped inside a render context.

For those interested: here's a method which executes a callable (i.e. any potentially-endangered code which you'd execute during a request) plus the URL generation, into a render context and returns a TrustedRedirectResponse: https://git.drupalcode.org/project/samlauth/blob/8.x-3.x/src/Controller/...
Call example:

  public function myRoute() {
    // The callable must return a Url object that will be used for the redirect.
    $function = function () {
      do_stuff();
      do_more_stuff();
      return Url::fromUri('http://i.want.to/redirect/here');
    };
    // Redirect to '<front>' displaying an error, if $function throws an exception.
    // (If left empty, no exception handling is done.)
    return $this->getTrustedRedirectResponse($function, 'executing myRoute', '<front>');
  }

Also, half related: just stumbled onto #2528598: Opt out of cacheablity protection on route level. (I wish I'd know of its existence years ago - I didn't know if it made sense to propose this myself.)

Wow! The documentation in samlauth/blob/8.x-3.x/src/Controller/ExecuteInRenderContextTrait.php is very insightful and well-detailed. It is actual learning material :) Kudos to @roderik

Thanks :) All the info in the trait's code comments is from other sources (issue comments from Wim Leers/Berdir/Crell/..., the Lullabot article). But it's been quite a challenge to form an opinion on how different pain points relate, and which to highlight in a concise summary. (4 multi-day sessions between 2017 and now, so far. I respect that experienced core developers have had no time for this and needed to rush things to get Drupal 8 out the door - but... by now I also get that it hasn't been easy for anyone else to make progress on this.)

In case anyone's wondering why this is in a trait hidden away in a random contrib module, and not made into a patch on this issue: in my estimation, this issue is explicitly stalled since January 2016 / comment #4. The last few patches ignore the comments made around there.

(If I could put my own spin on making a short summary: the best single-comment summary for the rationale behind the current implementation is in #2450993-133: Rendered Cache Metadata created during the main controller request gets lost. Unfortunately, a large part of the points summarized (1, 3, 5) there is untrue. I assume that was harder to see in January 2016 but it's clear now - now that the remaining pain points are hidden behavior of Url::toString() calls, Drupal Core implicitly forcing developers to use cacheable responses in at least a subset of redirect responses, and lack of help/documentation pointing these things out.)

If anyone wants to form a mini initiative / task force around getting URL stuff resolved: feel free to contact me. I think this will need the following steps:

• Talk to people behind the current implementation, outside of the issue queues, to make sure we have an overview of the whole scope and buy-in on plans. Because several related issues haven't seen real action since 2016 and we don't want to start producing work that goes against / will be stalled by other goals we don't know about. Or that won't get reviewed/committed.
• Probably: fall into a swamp of interconnected issues we weren't fully aware of.
• Gather thoughts; set goals/scope.

My own idea for a minimum scope to work on, is to get clarifying help text into Url::toString() and/or other places in documentation, making people aware of the risks of outputting URLs. (Because we're not doing that, thereby letting people introduce new fatal bugs unwittingly.) Last time I set that goal was February 2019, which devolved into working on #2735575-66: Make Url::toString(TRUE) explicit by having a Url::generate() method, as well as having GeneratedUrl::toString for more than a month and then being distracted by other things. Now, after spending time on building that new trait with fresh thoughts... I'll de-prioritize this again for a while... unless other people want to join forces.

#43 works

The patch #43 does the job.

#43 works

Is anyone at DrupalCon Europe/Prague (2022) interested in discussing #50?

The last patch seemed to have failures. Which makes me think it will need it's own tests as it's breaking the existing ones.

Drupal :9.5.9

Module : Rest Block Layout

Issue when Api call /block-layout?_format=json&path=/test It is giving error Previously on drupal 8.7.5 it is working fine but after version updation it is giving error

Error Message : LogicException: The controller result claims to be providing

relevant cache metadata, but leaked metadata was detected. Please
ensure you are not rendering content too early. Returned object class:
Drupal\rest\ResourceResponse. in
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->wrapControllerExecutionInRenderContext()
(line 158 of
\core\lib\Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber.php).
I have applied below #43 mentioned patch but from below patch error is gone , but in my website if user view multiple pages in separate tabs it is showing wrong node content data. It shows data which was latest cached.
. Let me know if anyone has solution how to fix it .

#43 still works 🎉 The nightmare is over for me 😅

Idem, #43 solved my problem.
I'm experiencing this problem in a REST resource. The context user.node_grants:view throws this error.

Patch #43 still applies to 10.3.x. However, it should probably be converted to a merge request.

Also, the result after patching is this:

      elseif ($response instanceof CacheableResponseInterface) {
        $response->addCacheableDependency($early_rendering_bubbleable_metadata);
      }
      // If a non-Ajax Response or domain object is returned and it cares about
      // attachments or cacheability, then throw an exception: early rendering
      // is not permitted in that case. It is the developer's responsibility
      // to not use early rendering.
      elseif ($response instanceof AttachmentsInterface || $response instanceof CacheableResponseInterface || $response instanceof CacheableDependencyInterface) {
        throw new \LogicException(sprintf('The controller result claims to be providing relevant cache metadata, but leaked metadata was detected. Ensure you are not rendering content too early. Returned object class: %s.', get_class($response)));
      }

The || $response instanceof CacheableResponseInterface check should be removed from the second else-if shown.

The new merge request targets 11.x and has the changes from patch #43, as well as the change I mentioned in #66.

I fixed the broken test. It just needed to be updated, since the code changes result in a success response instead of an HTTP 500.

This means that the changes have test coverage. All tests are passing successfully now.

Please review the changes and check whether the test coverage for early rendering needs to be expanded to handle more cases.

See EarlyRenderingControllerTest::testEarlyRendering. The changed test case is the one from lines 75-78

Hiding patches and empty branches

Ran test-only feature to confirm coverage

1) Drupal\Tests\system\Functional\Common\EarlyRenderingControllerTest::testEarlyRendering
Behat\Mink\Exception\ExpectationException: Current response status code is 500, but 200 expected.
/builds/issue/drupal-2638686/vendor/behat/mink/src/WebAssert.php:888
/builds/issue/drupal-2638686/vendor/behat/mink/src/WebAssert.php:145
/builds/issue/drupal-2638686/core/modules/system/tests/src/Functional/Common/EarlyRenderingControllerTest.php:76
/builds/issue/drupal-2638686/vendor/phpunit/phpunit/src/Framework/TestResult.php:729
ERRORS!
Tests: 1, Assertions: 27, Errors: 1.

believe the issue summary proposed solution lines up with moving the CacheableResponseInterface up to it's own check.

Believe this is good and has gotten a few comments about it working for others.

I landed here chasing various things. For how long this issue has been open, how many comments, patches and effort went into this, I was amazed / shocked / delighted / heartbroken that the diffstat on the changes tab in the MR is: +7 / - 3. 😂 Thrilled to see it's actually RTBC at this point, and hopefully going to land soon. +1 to the RTBC. The code changes are great. The test change is that we now get to check for better outcomes to a common DX problem. 🚀 it!

Thanks,
-Derek

p.s. Starting to save credit. Could use a closer look before commit.

Hate to ask, but do we need a change record for this?

The title of this does not match what I read in the patch, it is not removing an exception. I went to improve that using information in the proposed resolution. However, that describes some options. So, I am sadly setting back to NW for a descriptive title and a tweak to the proposed resolution so that it what is being changed.

I opened an MR thread to document how the diff is removing an exception. I'm a little sad to change the title at all, but how's this?

I think the new title is acceptable.

Title update seems good.

I reread this a fair bit.

I think given we have all the information here, and a way to apply it to the response, it's fine to do that and not throw the exception. As discussed in the issue, if we were able to inform developers at development time via an assertion or useful error message that stuff isn't being passed around correctly, that might be better, but there's no solution for this in several years now. The exception hitting on runtime with no obvious way to debug doesn't help anyone.

I wonder how many people will hit the remaining exception here, but I guess we'll find out whether that's an issue or not down the line.

Tried to do my best with issue credit but it's a very long issue so apologies if anyone was overlooked.

Committed/pushed to 11.x and cherry-picked to 10.3.x, thanks!