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

CommentFileSizeAuthor
#69 interdiff-2626298-58-69.txt5.43 KBmarthinal
#69 2626298-69.patch15.17 KBmarthinal
#58 2626298-58.patch15.2 KBmarthinal
#58 interdiff-2626298-54-58.txt1.75 KBmarthinal
#54 2626298-54.patch14.63 KBmarthinal
#54 interdiff-2626298-52-54.txt758 bytesmarthinal
#52 2626298-52.patch14.38 KBmarthinal
#52 interdiff-2626298-46-52.txt2.63 KBmarthinal
#46 2626298-46.patch13.32 KBmarthinal
#43 2626298-43.patch15.13 KBmarthinal
#43 interdiff-2626298-41-43.txt3.09 KBmarthinal
#41 2626298-41.patch15.15 KBmarthinal
#39 2626298-39.patch15.12 KBmarthinal
#39 interdiff-2626298-37-39.txt8.58 KBmarthinal
#37 rest_module_must_cache-2626298-37.patch14.04 KBborisson_
#37 interdiff.txt5.22 KBborisson_
#32 2626298-32.patch11.03 KBmarthinal
#32 interdiff-2626298-29-32.txt1.57 KBmarthinal
#29 interdiff-2626298-27-29.txt771 bytesmarthinal
#29 2626298-29.patch9.46 KBmarthinal
#27 interdiff-2626298-24-27.txt561 bytesmarthinal
#27 2626298-27.patch9.5 KBmarthinal
#24 2626298-24.png62.78 KBmarthinal
#24 2626298-24.patch9.38 KBmarthinal
#20 2626298-19.patch12.97 KBmarthinal
#16 2626298-16.patch12.88 KBmarthinal
#10 2626298-10.patch12.78 KBmarthinal
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

marthinal created an issue. See original summary.

andypost’s picture

Please point a resource to reproduce this, suppose you mean user-name render

Wim Leers’s picture

See #2450993-24: Rendered Cache Metadata created during the main controller request gets lost, point 4:

4) For the edge cases where you want to render a completely new context (such as an email page, or creating an HTML page other than the one you're about to return, etc.), you have to explicitly create a new context. This is already happening in, eg, contact module.

Wim Leers’s picture

Besides, this can be generalized to sending an e-mail from any random place in the code should not trigger exceptions.

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.

marthinal’s picture

Assigned: marthinal » Unassigned

@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!

Berdir’s picture

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

4) For the edge cases where you want to render a completely new context (such as an email page, or creating an HTML page other than the one you're about to return, etc.), you have to explicitly create a new context. This is already happening in, eg, contact module.

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.

marthinal’s picture

@Berdir Oops I didn't see the link in the block ("referenced by") :) . Thanks!!!

Wim Leers’s picture

Issue tags: +D8 cacheability

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.

Exactly!

marthinal’s picture

Assigned: Unassigned » marthinal
marthinal’s picture

Status: Needs work » Needs review
Issue tags: +SprintWeekend2016
FileSize
12.78 KB

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

Berdir’s picture

We'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.

Berdir’s picture

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

Wim Leers’s picture

marthinal’s picture

Title: EarlyRendering Exception when an email is sent from a REST resource. » REST module must cache only GET requests.
Issue summary: View changes

Updating IS.

marthinal’s picture

Created an issue for second point.

marthinal’s picture

Rerolled

Status: Needs review » Needs work

The last submitted patch, 16: 2626298-16.patch, failed testing.

The last submitted patch, 16: 2626298-16.patch, failed testing.

Berdir’s picture

I 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?

marthinal’s picture

Status: Needs work » Needs review
FileSize
12.97 KB

Should apply now...

Wim Leers’s picture

Status: Needs review » Needs work
Issue tags: +Needs issue summary update

#19: because

class ResourceResponse extends Response implements CacheableResponseInterface {

  use CacheableResponseTrait;

i.e. the CacheableResourceResponse that this patch introduces actually already exists. Which is why this issue needs to justify introducing that class.

The last submitted patch, 20: 2626298-19.patch, failed testing.

Wim Leers’s picture

Oh, apparently ResourceResponse is removed. My bad, sorry!

(But could you please record moves by using the -M flag? That makes patches easier to read.)

+++ b/core/modules/rest/src/UncacheableResourceResponse.php
@@ -0,0 +1,30 @@
+class UncacheableResourceResponse extends Response implements ResourceResponseInterface {

"uncacheable" usually means "max-age=0". So I find this very confusing.

marthinal’s picture

Issue summary: View changes
Status: Needs work » Needs review
Issue tags: -Needs issue summary update
FileSize
9.38 KB
62.78 KB

Working a little bit more on it today.

marthinal’s picture

Issue summary: View changes

Status: Needs review » Needs work

The last submitted patch, 24: 2626298-24.patch, failed testing.

marthinal’s picture

Status: Needs work » Needs review
FileSize
9.5 KB
561 bytes

Let's try again

Status: Needs review » Needs work

The last submitted patch, 27: 2626298-27.patch, failed testing.

marthinal’s picture

Status: Needs work » Needs review
FileSize
9.46 KB
771 bytes

The data provided by RequestHandlerTest::providerTestSerialization() in some cases is empty,false,null...

  public function providerTestSerialization() {
    return [
      [NULL],
      [''],
      ['string'],
      ['Complex \ string $%^&@ with unicode ΑΒΓΔΕΖΗΘΙΚΛΜΝΞΟΣὨ'],
      [[]],
      [['test']],
      [['test' => 'foobar']],
      [TRUE],
      [FALSE],
      // @todo Not supported. https://www.drupal.org/node/2427811
      // [new \stdClass()],
      // [(object) ['test' => 'foobar']],
    ];
  }

So, it was impossible to set the content in these cases.

dawehner’s picture

So, it was impossible to set the content in these cases.

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

dawehner’s picture

Status: Needs review » Needs work
marthinal’s picture

I'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).

dawehner’s picture

I'm not sure about the best way to test it...

IMHO we should look at the HTTP response and their cacheable metadata.

marthinal’s picture

@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 :


POST request to: http://d81/entity/entity_test

Code: 201Response headers: Array
(
[:status] => HTTP/1.1 201 Created
[date] => Mon, 29 Feb 2016 11:47:36 GMT
[server] => Apache/2.4.16 (Unix) PHP/5.5.29
[x-content-type-options] => nosniff,nosniff
[x-powered-by] => PHP/5.5.29
[location] => http://d81/entity_test/1
[cache-control] => must-revalidate, no-cache, private
[x-ua-compatible] => IE=edge
[content-language] => en
[x-frame-options] => SAMEORIGIN
[expires] => Sun, 19 Nov 1978 05:00:00 GMT
[x-generator] => Drupal 8 (https://www.drupal.org)
[content-length] => 389
[content-type] => application/json
)

So, I think that maybe my last unit test is enough to detect that ResourceResponse is cacheable and that NonCacheableResuorceResponse is not cacheable...

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

klausi’s picture

Status: Needs review » Needs work
Issue tags: +DrupalBCDays
  1. +++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
    @@ -115,9 +116,7 @@ public function post(EntityInterface $entity = NULL) {
    -      // Responses after creating an entity are not cacheable, so we add no
    -      // cacheability metadata here.
    +      $response = new NonCacheableResourceResponse($entity, 201, ['Location' => $url->getGeneratedUrl()]);
    

    Why do you remove that comment? I think we should keep it for clarity.

  2. +++ b/core/modules/rest/tests/src/Kernel/RequestHandlerTest.php
    @@ -71,15 +72,21 @@ public function testBaseHandler() {
    +    // Only GET request must be cacheable.
    +    $this->assertInstanceOf('Drupal\Core\Cache\CacheableResponseInterface', $handler_response);
    

    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?

  3. +++ b/core/modules/rest/tests/src/Kernel/RequestHandlerTest.php
    @@ -71,15 +72,21 @@ public function testBaseHandler() {
    +    // We will call the PATCH method this time.Response will return a
    

    Space after the dot.

  4. +++ b/core/modules/rest/tests/src/Kernel/RequestHandlerTest.php
    @@ -71,15 +72,21 @@ public function testBaseHandler() {
    +    $this->assertNotInstanceOf('Drupal\Core\Cache\CacheableResponseInterface', $handler_response);
    

    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.

borisson_’s picture

dawehner’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/rest/src/NonCacheableResourceResponse.php
    @@ -0,0 +1,30 @@
    +/**
    + * @file
    + * Contains \Drupal\rest\NonCacheableResourceResponse.
    + */
    
    +++ b/core/modules/rest/src/ResourceResponseInterface.php
    @@ -0,0 +1,23 @@
    +
    +/**
    + * @file
    + * Contains \Drupal\rest\ResourceResponseInterface.
    + */
    
    +++ b/core/modules/rest/src/ResourceResponseTrait.php
    @@ -0,0 +1,30 @@
    +
    +/**
    + * @file
    + * Contains \Drupal\rest\ResourceResponseTrait.
    + */
    +
    

    nitpick: Let's drop it

  2. +++ b/core/modules/rest/src/ResourceResponseTrait.php
    index 1958fdc..c31af80 100644
    --- a/core/modules/rest/src/Tests/PageCacheTest.php
    
    --- a/core/modules/rest/src/Tests/PageCacheTest.php
    +++ b/core/modules/rest/src/Tests/PageCacheTest.php
    
    +++ b/core/modules/rest/src/Tests/PageCacheTest.php
    +++ b/core/modules/rest/src/Tests/PageCacheTest.php
    @@ -21,9 +21,14 @@ class PageCacheTest extends RESTTestBase {
    
    @@ -55,6 +60,47 @@ public function testConfigChangePageCache() {
    +
    +    // Log in for deleting / updating entity.
    

    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 already

  3. +++ b/core/modules/rest/src/Tests/PageCacheTest.php
    @@ -55,6 +60,47 @@ public function testConfigChangePageCache() {
    +    foreach ($headers as $header) {
    +      if (strpos($header, 'x-drupal-cache') !== FALSE) {
    +        $this->fail('Patch request is cached.');
    +      }
    +    }
    ...
    +    foreach ($headers as $header) {
    +      if (strpos($header, 'x-drupal-cache') !== FALSE) {
    +        $this->fail('Patch request is cached.');
    +      }
    +    }
    

    Can't we use the \Drupal\system\Tests\Cache\AssertPageCacheContextsAndTagsTrait::getCacheHeaderValues for that?

marthinal’s picture

Status: Needs work » Needs review
Issue tags: +neworleans2016
FileSize
8.58 KB
15.12 KB

1) Done

2) Adding a POST request to create the entity and verify that POST requests are not cacheable.

3) Done

Status: Needs review » Needs work

The last submitted patch, 39: 2626298-39.patch, failed testing.

marthinal’s picture

Status: Needs work » Needs review
FileSize
15.15 KB

Reroll

dawehner’s picture

Just a couple of more nitpicks ...

  1. +++ b/core/modules/rest/src/NonCacheableResourceResponse.php
    @@ -0,0 +1,25 @@
    +
    +class NonCacheableResourceResponse extends Response implements ResourceResponseInterface {
    

    Let's document usecases when this class should be used ...

  2. +++ b/core/modules/rest/src/NonCacheableResourceResponse.php
    @@ -0,0 +1,25 @@
    +  }
    +}
    

    nitpick: should have a new line

  3. +++ b/core/modules/rest/src/RequestHandler.php
    @@ -97,27 +98,36 @@ public function handle(RouteMatchInterface $route_match, Request $request) {
    +      // Cacheable Response.
    +      if ($response instanceof CacheableResponseInterface) {
    +        // Serialization can invoke rendering (e.g., generating URLs), but the
    +        // serialization API does not provide a mechanism to collect the
    +        // bubbleable metadata associated with that (e.g., language and other
    +        // contexts), so instead, allow those to "leak" and collect them here in
    +        // a render context.
    +        // @todo Add test coverage for language negotiation contexts in
    +        //   https://www.drupal.org/node/2135829.
    +        $context = new RenderContext();
    +        $output = $this->container->get('renderer')->executeInRenderContext($context, function () use ($serializer, $data, $format) {
    +          return $serializer->serialize($data, $format);
    +        });
    +        $response->setContent($output);
    +        if (!$context->isEmpty()) {
    +          $response->addCacheableDependency($context->pop());
    +        }
     
    ...
    +        $response->headers->set('Content-Type', $request->getMimeType($format));
    +        // Add rest settings config's cache tags.
    +        $response->addCacheableDependency($this->container->get('config.factory')->get('rest.settings'));
    +      }
    +      else {
    +        // Non-Cacheable Response.
    +        $output = $serializer->serialize($data, $format);
    +        $response->setContent($output);
    +        $response->headers->set('Content-Type', $request->getMimeType($format));
    +      }
    

    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

  4. +++ b/core/modules/rest/src/ResourceResponseInterface.php
    @@ -0,0 +1,18 @@
    + * Defines a common interface for Resource Responses.
    

    Nitpick as well: let's make both lowercase

  5. +++ b/core/modules/rest/src/ResourceResponseInterface.php
    @@ -0,0 +1,18 @@
    +  public function getResponseData();
    +}
    

    empty new line

  6. +++ b/core/modules/rest/src/ResourceResponseTrait.php
    @@ -0,0 +1,25 @@
    +
    +Trait ResourceResponseTrait {
    

    Let's remove the empty line and let put "Trait" to be lowercase

marthinal’s picture

1) Done

2) Done

3) Done

4) Done

5) Done

6) Done

Thanks @dawhener

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

This works for me now. Thank you @marthinal

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 43: 2626298-43.patch, failed testing.

marthinal’s picture

Status: Needs work » Needs review
FileSize
13.32 KB
dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC

Wim Leers’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: -Needs tests
  1. +++ b/core/modules/rest/src/RequestHandler.php
    @@ -98,9 +99,20 @@ public function handle(RouteMatchInterface $route_match, Request $request) {
    +    if ($response instanceof ResourceResponseInterface) {
    +      if ($response instanceof CacheableResponseInterface) {
    +        // Cacheable Response.
    +        $response = $this->renderResponse($request, $response, $serializer, $format);
    +      }
    +      else {
    +        // Non-Cacheable Response.
    +        $output = $serializer->serialize($response->getResponseData(), $format);
    +        $response->setContent($output);
    +        $response->headers->set('Content-Type', $request->getMimeType($format));
    +      }
    +    }
    

    It 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 the renderResponse() method instead. That'd not break the intent of #2718545.

  2. +++ b/core/modules/rest/src/RequestHandler.php
    @@ -98,9 +99,20 @@ public function handle(RouteMatchInterface $route_match, Request $request) {
    +        // Cacheable Response.
    ...
    +        // Non-Cacheable Response.
    

    These comments are pointless.

marthinal’s picture

Status: Needs work » Needs review

1. For non-cacheable responses we are not rendering, basically we are serializing... Does it make sense this name for non-cacheable responses??

Status: Needs review » Needs work

The last submitted patch, 46: 2626298-46.patch, failed testing.

Wim Leers’s picture

The serialized content can become outdated, so yes, its cacheability matters.

marthinal’s picture

Status: Needs work » Needs review
FileSize
2.63 KB
14.38 KB

Done. Let's take a look at the test results.

Status: Needs review » Needs work

The last submitted patch, 52: 2626298-52.patch, failed testing.

marthinal’s picture

Status: Needs work » Needs review
FileSize
758 bytes
14.63 KB

Ok! let's try again...

Wim Leers’s picture

Status: Needs review » Needs work

Awesome! This looks so very close!

I'd have RTBC'd this if it weren't for the second remark in the first point below.

  1. +++ b/core/modules/rest/src/NonCacheableResourceResponse.php
    @@ -0,0 +1,32 @@
    +class NonCacheableResourceResponse extends Response implements ResourceResponseInterface {
    

    This should get an @see to ResourceResponse.

    Also I think this should actually be called UncacheableResourceResponse, to be in line with \Drupal\views\Plugin\views\field\UncacheableFieldHandlerTrait.

  2. +++ b/core/modules/rest/src/RequestHandler.php
    @@ -137,22 +138,29 @@ public function csrfToken() {
    -  protected function renderResponse(Request $request, ResourceResponse $response, SerializerInterface $serializer, $format) {
    +  protected function renderResponse(Request $request, ResourceResponseInterface $response, SerializerInterface $serializer, $format) {
    

    Signature updated, but docblock not. Can be fixed on commit.

  3. +++ b/core/modules/rest/src/ResourceResponse.php
    @@ -14,16 +14,10 @@
    +class ResourceResponse extends Response implements CacheableResponseInterface, ResourceResponseInterface {
    

    This should get an @see to NonCacheableResourceResponse.

Wim Leers’s picture

Title: REST module must cache only GET requests. » REST module must cache only respones to GET requests
Wim Leers’s picture

Title: REST module must cache only respones to GET requests » REST module must cache only responses to GET requests

Oops.

marthinal’s picture

Status: Needs work » Needs review
FileSize
1.75 KB
15.2 KB

1) Done.

Also I think this should actually be called UncacheableResourceResponse, to be in line with \Drupal\views\Plugin\views\field\UncacheableFieldHandlerTrait

See #23. Well. we can change it again... I have no objections :)

2) Done.

3) Done.

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

#58.1: Oh hahah, you're totally right! Thanks for pointing that out :)

Then this is ready, thanks so much!


+++ b/core/modules/rest/src/NonCacheableResourceResponse.php
@@ -9,6 +9,8 @@
+ * @see Drupal\rest\ResourceResponse

+++ b/core/modules/rest/src/ResourceResponse.php
@@ -13,6 +13,8 @@
+ * @see Drupal\rest\NonCacheableResourceResponse

Nits that can be fixed on commit: these need leading backslashes.

catch’s picture

Status: Reviewed & tested by the community » Needs review

Should this really be Uncacheable or should we use something like 'UnsafeMethodResourceResponse' to match isMethodSafe()?

Wim Leers’s picture

Status: Needs review » Needs work

#60: see #23 why this is not named UncacheableResourceResponse.

UnsafeMethodResourceResponse is logical: your reasoning makes sense. But it's:

  • very long
  • sounds very scary

So, I'm not entirely convinced. But I think it is indeed better. Marking NW for that simple rename then.

catch’s picture

fwiw I do think it's overly long and scary.

Other options, none help with the lenght:

NonSafeResourceResponse

NonIdempotentResourceResponse

NonSafeMethodResourceResponse

dawehner’s picture

As part of the IRC discussion we talked about: GETResourceResponse and NonGETResourceResponse. This would make it more obvious for PHP class API users, at least from my point of view.

Wim Leers’s picture

13:38:10 <WimLeers> dawehner: catch then we might as well go with ModifiedResourceResponse
13:38:22 <catch> WimLeers: oooh.
13:38:34 <WimLeers> that would be vastly simpler
13:39:02 <catch> WimLeers: very +1
13:39:09 <WimLeers> ok cool :)
13:39:15 <WimLeers> dawehner: also +1?
13:40:41 <dawehner> WimLeers: +1

So, rename NonCacheableResourceResponse to ModifiedResourceResponse, and then this is RTBC again.

Wim Leers’s picture

+++ b/core/modules/rest/src/NonCacheableResourceResponse.php
@@ -0,0 +1,34 @@
+ * Use this class in case the response should not be cached. An example would be
+ * for POST/PATCH/DELETE requests.

And update this comment to:

Used when resources are modified by a request: responses to unsafe requests (POST/PATCH/DELETE) can never be cached.
dawehner’s picture

@Wim Leers
Did you saw my suggestion?

Wim Leers’s picture

20:34:20 <WimLeers> dawehner: https://www.drupal.org/node/2626298#comment-11220789 confuses me
20:34:22 <Druplicon> https://www.drupal.org/node/2626298 => REST module must cache only responses to GET requests #2626298: REST module must cache only responses to GET requests => 66 comments, 10 IRC mentions
20:35:01 <dawehner> WimLeers: well catch and I talked a bit about using GET and NonGET, which could be easier to understand
20:35:21 <WimLeers> dawehner: I did read your suggestions in IRC. GetResourceResponse & NonGetResourceResponse. I think they're less clear. Also, we can't change the existing ResourceResponse's name. So then "ResourceResponse" vs "ModifiedResourceResponse" sound very sensible.
20:36:22 <dawehner> WimLeers: we totally can with a BC layer
20:36:44 <dawehner> WimLeers: well, for me ModifiedResourceResponse is hard to find
20:36:52 <dawehner> WimLeers: but yeah this is just me
20:37:47 <WimLeers> dawehner: what do you mean by "hard to find"?
20:38:10 <dawehner> WimLeers: well, imagine that one would write their own plugin
20:38:32 <dawehner> WimLeers: but yeah I agree GET and NonGet isn't easier

So let's move forward with ModifiedResourceResponse, that also has catch's +1.

dawehner’s picture

If both @catch and @Wim Leers think that modified would be better, let's go with it.

marthinal’s picture

Status: Needs work » Needs review
FileSize
15.17 KB
5.43 KB

Done! Let's try again!!!! :)

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

Woot! :)

dawehner’s picture

+1

  • catch committed 8c57032 on 8.2.x
    Issue #2626298 by marthinal, borisson_, Wim Leers, dawehner: REST module...
catch’s picture

Status: Reviewed & tested by the community » Fixed

Fixed these on commit:

FILE: ...8.x/core/modules/rest/src/ResourceResponse.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 38 | ERROR | [x] The closing brace for the class must have an empty
    |       |     line before it
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

Time: 28ms; Memory: 3.75Mb


FILE: ...8.x/core/modules/rest/src/ResourceResponseInterface.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 18 | ERROR | [x] Expected 1 newline at end of file; 2 found
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

Time: 23ms; Memory: 3.75Mb

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.

Wim Leers’s picture

OMG YES FINALLY! So glad that this is finally in.

catch’s picture

Adding some review credits.

Status: Fixed » Closed (fixed)

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