Problem/Motivation

The dynamic page cache subscriber introduced a request attribute to store the cache policy result.

Proposed resolution

Store the request policy result inside the subscriber object.

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#2 remove_request_policy-2575847-2.patch2.89 KBznerol
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

znerol created an issue. See original summary.

znerol’s picture

znerol CreditAttribution: znerol commented
Status: Active » Needs review
FileSize
remove_request_policy-2575847-2.patch2.89 KB
Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Component: cache system » dynamic_page_cache.module
Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Priority: Normal » Minor
Status: Needs review » Reviewed & tested by the community

Makes sense to me.

+++ b/core/modules/dynamic_page_cache/src/EventSubscriber/DynamicPageCacheSubscriber.php
@@ -104,6 +96,13 @@ class DynamicPageCacheSubscriber implements EventSubscriberInterface {
+  protected $requestPolicyResult;

Needs trailing s. Can be fixed on commit.

  • catch committed b97d20d on 8.1.x 
    Issue #2575847 by znerol: Remove request policy attribute from dynamic...
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Version: 8.0.x-dev » 8.1.x-dev
Status: Reviewed & tested by the community » Fixed

Looks great. Less request attributes the better.

Committed/pushed to 8.1.x only because this is internal refactoring, removes the constant, adds the protected property etc. I can't see a reason this would need to go into 8.0.x but if there is one please re-open for backport.

Fixed the missing s on the protected property.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented

Looks great. Less request attributes the better.

Exactly :)

Status: Fixed » Closed (fixed)

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

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Related issues: +#2759247: Request attribute removed from DynamicPageCacheSubscriber, but comment was not updated

This failed to update one comment. Created a minor follow-up: #2759247: Request attribute removed from DynamicPageCacheSubscriber, but comment was not updated.