Problem/Motivation
When we render from a REST Resource we will receive an EarlyRendering Exception. This problem was detected registering new users from the REST Resource(#2291055: REST resources for anonymous users: register) because at some point we are rendering when building the email.
Proposed resolution
We have 3 different issues here as suggested by @Berdir:
1) Anything that is done within mail() should *not* be added to the global cache context. E.g, sending out an email that renders stuff that adds JS/CSS libraries should *not* result in them being added to the main response.
Created a new issue to fix this problem. #2663270: MailManager::mail() should run inside its own render context: it sends e-mails, not (cacheable) responses
2) Don't check the render context stuff for Non-GET requests. They won't be cached.
Created a new issue to fix this problem. #2663638: Avoid checking the render context to detect early rendering for Non-GET requests.
3) Introduce the new non-cacheable rest response class for POST, PATCH and DELETE requests. And here is a quick diagram where only ResourceResponse is implementing CacheableResponseInterface:
Remaining tasks
Comment | File | Size | Author |
---|---|---|---|
#69 | interdiff-2626298-58-69.txt | 5.43 KB | marthinal |
#69 | 2626298-69.patch | 15.17 KB | marthinal |
#58 | 2626298-58.patch | 15.2 KB | marthinal |
#58 | interdiff-2626298-54-58.txt | 1.75 KB | marthinal |
#54 | 2626298-54.patch | 14.63 KB | marthinal |
Comments
Comment #2
andypostPlease point a resource to reproduce this, suppose you mean user-name render
Comment #3
Wim LeersSee #2450993-24: Rendered Cache Metadata created during the main controller request gets lost, point 4:
Comment #4
Wim LeersBesides, this can be generalized to
.Which 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 — regardless of how bad it is — then the solution is given by #3.
Comment #5
marthinal CreditAttribution: marthinal commented@Wim Leers Ok I see. Then I'll try to send the email from a different way, makes sense. I want to continue working on this issue in the upcoming Global Sprints.
For the moment unassigning the issue so anyone can feel free to work on this.
Thanks!
Comment #6
Berdir@marthinal: Make sure to read (and join the discussion) in #2638686: Exception in EarlyRenderingControllerWrapperSubscriber is a DX nightmare, remove it, which is discussing this in a broader way.
contact module doesn't do anything like that. The only thing it does do is to use renderPlain() in contact_mail().
However, what we should do then is to always use a render context directly in MailPluginManager::mail()? I think it is a perfectly safe assumption that anything that happens in there should be a separate render context and never bubble up.
Comment #7
marthinal CreditAttribution: marthinal commented@Berdir Oops I didn't see the link in the block ("referenced by") :) . Thanks!!!
Comment #8
Wim LeersExactly!
Comment #9
marthinal CreditAttribution: marthinal commentedComment #10
marthinal CreditAttribution: marthinal commented1. Creating the user from the UI, we're sending the email in the same request. So IMO from REST we should do the same. So the issue #2291055: REST resources for anonymous users: register is correct as it is.
2. I think we don't need to add a render context in this case because we don't need to cache the response for POST, PATCH , DELETE requests and the current patch fixes this problem.
3. @Berdir, @Wim Leers Anyway I'm not 100% sure if I should use a render context from MailManager::mail() because I'm not sure if we have a use case where we would need it.
Comment #11
BerdirWe're fixing this specific example by introducing non-cacheable responses (which btw, is an API change IMHO as we stop caching requests that were cacheable before, so maybe we need UncacheableRestResponse instead, even if that's inconsistent with other naming).
I also find it wrong that we have to work around this for POST/PUT/DELETE requests. As stated in #2638686: Exception in EarlyRenderingControllerWrapperSubscriber is a DX nightmare, remove it, those requests are not cacheable by (HTTP) design, we shouldn't check or throw an exception for those. It really makes no sense. Might want a separate issue for that.
But, the general problem remains that anything that is done within mail() should *not* be added to the global cache context. E.g, sending out an email that renders stuff that adds JS/CSS libraries should *not* result in them being added to the main response.
Comment #12
BerdirJust ran into another case like this during payment submission, trying to move this forward.
What I meant above is that we can do 3 things:
* Wrap sending mails in a render context. Note that, just as mentioned in #2638686: Exception in EarlyRenderingControllerWrapperSubscriber is a DX nightmare, remove it, we are forced to use @internal classes for this. If this really is a thing anyone is expected to do, it can't possibly be @internal?
* Don't check the render context stuff for Non-GET requests. They won't be cached.
* Introduce the new non-cacheable rest response classes as the current patch here is doing.
We can and we IMHO should do all those things. Any of them would be enough to fix this specific bug because overlaps between all of them, but they all also fix issues in separate areas.
And we should probably do separate issues, since this is already about the response objects, we should open issues for the other two and work on them separately. And clarify this issue's title and summary, including references to the other issues.
Comment #13
Wim LeersCreated an issue+patch for sending mails in a render context: #2663270: MailManager::mail() should run inside its own render context: it sends e-mails, not (cacheable) responses.
Comment #14
marthinal CreditAttribution: marthinal commentedUpdating IS.
Comment #15
marthinal CreditAttribution: marthinal commentedCreated an issue for second point.
Comment #16
marthinal CreditAttribution: marthinal commentedRerolled
Comment #19
BerdirI guess this conflicted with #2571929: REST entity POST request is not cacheable: cacheability metadata is unnecessary which surprises me a bit, not sure how that other issue can work without this?
Comment #20
marthinal CreditAttribution: marthinal commentedShould apply now...
Comment #21
Wim Leers#19: because
i.e. the
CacheableResourceResponse
that this patch introduces actually already exists. Which is why this issue needs to justify introducing that class.Comment #23
Wim LeersOh, apparently
ResourceResponse
is removed. My bad, sorry!(But could you please record moves by using the
-M
flag? That makes patches easier to read.)"uncacheable" usually means "max-age=0". So I find this very confusing.
Comment #24
marthinal CreditAttribution: marthinal commentedWorking a little bit more on it today.
Comment #25
marthinal CreditAttribution: marthinal commentedComment #27
marthinal CreditAttribution: marthinal commentedLet's try again
Comment #29
marthinal CreditAttribution: marthinal commentedThe data provided by RequestHandlerTest::providerTestSerialization() in some cases is empty,false,null...
So, it was impossible to set the content in these cases.
Comment #30
dawehnerFair point.
We in general lack test coverage for the introduced change. I though like the change of having a uncacheable and a cacheabl resource response.
Comment #31
dawehnerComment #32
marthinal CreditAttribution: marthinal commentedI'm not sure about the best way to test it... Maybe to detect that only ResourceResponse is an instance of CacheableResponseInterface. From unit testing we provide the response... So, not really sure if makes sense this test.
We can add an integration test where we verify that POST, PATCH and DELETE are never cached. This is the other possibility I'm thinking about. But in this case we are not checking for the new change(NonCacheableResourceResponse).
Comment #33
dawehnerIMHO we should look at the HTTP response and their cacheable metadata.
Comment #34
marthinal CreditAttribution: marthinal commented@dawehner
We're testing GET responses from Drupal\rest\Tests\PageCacheTest. Detecting if x-drupal-cache is MISS or HIT.
In the case of POST and PATCH response headers we don't have cacheable metadata AFAIK :
So, I think that maybe my last unit test is enough to detect that ResourceResponse is cacheable and that NonCacheableResuorceResponse is not cacheable...
Comment #36
klausiWhy do you remove that comment? I think we should keep it for clarity.
can we use ::class syntax here instead of the class name in the string? And I think this assertion does not make sense, because we are testing what we did just a few lines before? Can we check for HTTP cache headers in the functional tests instead and make sure they are set correctly to non-cacheable there?
Space after the dot.
Same here, we are aserting something which we just did in the test case before, better assert the cache headers in the other web tests.
Comment #37
borisson_I'm on my way to drupalcamp spain and I had some time in the plane. This should resolve @klausi's remarks from #36.
Comment #38
dawehnernitpick: Let's drop it
In general I'm wondering whether these checks should rather belong into the existing test classes like
\Drupal\rest\Tests\UpdateTest
as these code are meant to test code in similar areas alreadyCan't we use the
\Drupal\system\Tests\Cache\AssertPageCacheContextsAndTagsTrait::getCacheHeaderValues
for that?Comment #39
marthinal CreditAttribution: marthinal commented1) Done
2) Adding a POST request to create the entity and verify that POST requests are not cacheable.
3) Done
Comment #41
marthinal CreditAttribution: marthinal commentedReroll
Comment #42
dawehnerJust a couple of more nitpicks ...
Let's document usecases when this class should be used ...
nitpick: should have a new line
I'm wondering whether we could have the same codepath for both cases and simply don't add the cacheablity information in the case of the non cacheable response. This should simplify this here quite a bit
Nitpick as well: let's make both lowercase
empty new line
Let's remove the empty line and let put "Trait" to be lowercase
Comment #43
marthinal CreditAttribution: marthinal commented1) Done
2) Done
3) Done
4) Done
5) Done
6) Done
Thanks @dawhener
Comment #44
dawehnerThis works for me now. Thank you @marthinal
Comment #46
marthinal CreditAttribution: marthinal commentedRerolled. Here is the change: #2718545: Refactor \Drupal\rest\RequestHandler::handle() into smaller methods
Comment #47
dawehnerBack to RTBC
Comment #48
Wim LeersIt feels like this violates the point of #2718545: Refactor \Drupal\rest\RequestHandler::handle() into smaller methods, which was to reduce the complexity of
handle()
. I think we may want to move the inner if/else into therenderResponse()
method instead. That'd not break the intent of #2718545.These comments are pointless.
Comment #49
marthinal CreditAttribution: marthinal commented1. For non-cacheable responses we are not rendering, basically we are serializing... Does it make sense this name for non-cacheable responses??
Comment #51
Wim LeersThe serialized content can become outdated, so yes, its cacheability matters.
Comment #52
marthinal CreditAttribution: marthinal commentedDone. Let's take a look at the test results.
Comment #54
marthinal CreditAttribution: marthinal commentedOk! let's try again...
Comment #55
Wim LeersAwesome! This looks so very close!
I'd have RTBC'd this if it weren't for the second remark in the first point below.
This should get an
@see
toResourceResponse
.Also I think this should actually be called
UncacheableResourceResponse
, to be in line with\Drupal\views\Plugin\views\field\UncacheableFieldHandlerTrait
.Signature updated, but docblock not. Can be fixed on commit.
This should get an
@see
toNonCacheableResourceResponse
.Comment #56
Wim LeersComment #57
Wim LeersOops.
Comment #58
marthinal CreditAttribution: marthinal commented1) Done.
See #23. Well. we can change it again... I have no objections :)
2) Done.
3) Done.
Comment #59
Wim Leers#58.1: Oh hahah, you're totally right! Thanks for pointing that out :)
Then this is ready, thanks so much!
Nits that can be fixed on commit: these need leading backslashes.
Comment #60
catchShould this really be Uncacheable or should we use something like 'UnsafeMethodResourceResponse' to match isMethodSafe()?
Comment #61
Wim Leers#60: see #23 why this is not named
UncacheableResourceResponse
.UnsafeMethodResourceResponse
is logical: your reasoning makes sense. But it's:So, I'm not entirely convinced. But I think it is indeed better. Marking NW for that simple rename then.
Comment #62
catchfwiw I do think it's overly long and scary.
Other options, none help with the lenght:
NonSafeResourceResponse
NonIdempotentResourceResponse
NonSafeMethodResourceResponse
Comment #63
dawehnerAs part of the IRC discussion we talked about:
GETResourceResponse
andNonGETResourceResponse
. This would make it more obvious for PHP class API users, at least from my point of view.Comment #64
Wim LeersSo, rename
NonCacheableResourceResponse
toModifiedResourceResponse
, and then this is RTBC again.Comment #65
Wim LeersAnd update this comment to:
Comment #66
dawehner@Wim Leers
Did you saw my suggestion?
Comment #67
Wim LeersSo let's move forward with
ModifiedResourceResponse
, that also has catch's +1.Comment #68
dawehnerIf both @catch and @Wim Leers think that modified would be better, let's go with it.
Comment #69
marthinal CreditAttribution: marthinal commentedDone! Let's try again!!!! :)
Comment #70
Wim LeersWoot! :)
Comment #71
dawehner+1
Comment #73
catchFixed these on commit:
Committed 8c57032 and pushed to 8.2.x. Thanks!
I don't think there's anything we can do here for 8.1.x, but #2663638: Avoid checking the render context to detect early rendering for Non-GET requests. is a smaller change that will help with the exception.
Comment #74
Wim LeersOMG YES FINALLY! So glad that this is finally in.
Comment #75
catchAdding some review credits.