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

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.

<strong>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 .

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.

solideogloria → changed the visibility of the branch 2638686-exception-in-earlyrenderingcontrollerwrappersubscriber to hidden.

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.

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

smustgrave → changed the visibility of the branch 11.x to hidden.

smustgrave → changed the visibility of the branch 10.3.x to hidden.

smustgrave → changed the visibility of the branch 10.1.x to hidden.

smustgrave → changed the visibility of the branch 10.2.x to hidden.

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!

catch → closed merge request !7649