Problem/Motivation

The node module implements Drupal\node\PageCache\DenyNodePreview to prevent node previews from being cached.
The image module implements \Drupal\image\PageCache\DenyPrivateImageStyleDownload to prevent private image styles from being cached

But this can be done much simpler and with less code.

Proposed resolution

Remove DenyNodePreview and DenyPrivateImageStyleDownload
Remove the references in node.services.yml and image.services.yml
Add a no_cache: TRUE in node.routing.yml and image.routing.yml

API changes

Removal of 2 page cache response policies that are marked as private.

CommentFileSizeAuthor
#34 reroll_diff_18-34.txt5.74 KBravi.shankar
#34 2554641-34.patch10.8 KBravi.shankar
#18 remove-trivial-cache-policy-handlers-and-add-nocache-test-2554641-18.patch10.71 KBduaelfr
#13 2554641-remove-trivial-cache-policy-handlers-and-add-nocache-test2.patch14.77 KBbdragon
#11 2554641-remove-trivial-cache-policy-handlers-and-add-nocache-test.patch14.76 KBbdragon
#7 drupal-remove_denynodepreview-2554641-2.patch4.93 KBjeremy
#2 drupal-remove_denynodepreview-2554641.patch2.4 KBjeremy

Issue fork drupal-2554641

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

Jeremy created an issue. See original summary.

jeremy’s picture

jeremy commented
Status: Active » Needs review
StatusFileSize
new drupal-remove_denynodepreview-2554641.patch2.4 KB

This patch removes src/PageCache/DenyNodePreview.php, the reference in node.services.yml, and adds a no_cache: TRUE in node.routing.yml. There are no functional or API changes, this is just a simplification patch.

Status: Needs review » Needs work

The last submitted patch, 2: drupal-remove_denynodepreview-2554641.patch, failed testing.

Status: Needs work » Needs review

Wim Leers queued 2: drupal-remove_denynodepreview-2554641.patch for re-testing.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs review » Needs work

Yay, less code! :)

One being the proposed resolution here, the other being setting max_age = 0 on the controller.

The solution here should work, but setting max-age = 0 on the controller won't work until #2352009: Bubbling of elements' max-age to the page's headers and the page cache lands.

So, +1 to the solution proposed here.

DrupalCI is green, but DenyNodePreviewTest instantiates DenyNodePreview. Since that's removed, I'd expect both DrupalCI & classic testbot to come back red. Re-tested both.

This patch should also remove DenyNodePreviewTest, and add an integration test instead.

The last submitted patch, 2: drupal-remove_denynodepreview-2554641.patch, failed testing.

jeremy’s picture

jeremy commented
StatusFileSize
new drupal-remove_denynodepreview-2554641-2.patch4.93 KB

Removing DenyNodePreviewTest for now. Still needs a replacement test.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs work » Needs review
dawehner’s picture

dawehner
Primary language German
commented
Status: Needs review » Reviewed & tested by the community 
+++ b/core/modules/node/node.routing.yml
@@ -36,6 +36,7 @@ entity.node.preview:
   options:
+    no_cache: TRUE
     parameters:

We already have test coverage for 'no_cache' in general so I don't think we actually need test coverage here. Its almost like testing '_title' explicitly.

bdragon’s picture

bdragon commented
Assigned: Unassigned » bdragon
Status: Reviewed & tested by the community » Needs work

Actually, it appears we DON'T have test coverage.
Also, DenyPrivateImageStyleDownload that DenyNodePreview was based on is ALSO redundant when we have no_cache.

bdragon’s picture

bdragon commented
Title: Simplify node module, avoid caching without implementing src/PageCache/DenyNodePreview.php » Switch trivial page cache response handlers to use no_cache option instead
Status: Needs work » Needs review
StatusFileSize
new 2554641-remove-trivial-cache-policy-handlers-and-add-nocache-test.patch14.76 KB

Updated patch implementing an actual test for the no_cache option, removing comment copypasto referring to node, and removing DenyPrivateImageStyleDownload as well.

bdragon’s picture

bdragon commented
Title: Switch trivial page cache response handlers to use no_cache option instead » Switch trivial page cache policy handlers to use no_cache option instead

s/response/policy/

bdragon’s picture

bdragon commented
StatusFileSize
new 2554641-remove-trivial-cache-policy-handlers-and-add-nocache-test2.patch14.77 KB

Note to self: Remember to rerun test after renaming test.

The last submitted patch, 11: 2554641-remove-trivial-cache-policy-handlers-and-add-nocache-test.patch, failed testing.

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

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should 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.

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

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

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

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

duaelfr’s picture

duaelfr
he/him
Primary language French
Location Montpellier, France
commented
Version: 8.3.x-dev » 8.4.x-dev
StatusFileSize
new remove-trivial-cache-policy-handlers-and-add-nocache-test-2554641-18.patch10.71 KB

Patch rerolled against 8.4.x

duaelfr’s picture

duaelfr
he/him
Primary language French
Location Montpellier, France
commented
Issue tags: +DevDaysSeville

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

mstrelan’s picture

mstrelan commented
Status: Needs review » Needs work

I think we'll need to deprecate the services, and remove in 10.x

kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
commented
Category: Bug report » Task

Reviewed this as part of the Bug Smash Initiative. Looks like an improvement task, rather than a bug.

kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
commented
Issue tags: +Bug Smash Initiative
mstrelan’s picture

mstrelan commented
quietone’s picture

quietone commented
Assigned: bdragon » Unassigned

Un-assigning because bdragon has not worked on this in 6 years.

ravi.shankar’s picture

ravi.shankar commented
StatusFileSize
new 2554641-34.patch10.8 KB
new reroll_diff_18-34.txt5.74 KB

Added reroll of patch #18 on Drupal 9.4.x.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

acbramley made their first commit to this issue’s fork.

acbramley opened merge request !11548

acbramley’s picture

acbramley commented
Status: Needs work » Needs review

Rolled into an MR, re #29 these services were marked as private so I don't think we need to do a deprecation dance here?

acbramley’s picture

acbramley commented
mstrelan’s picture

mstrelan commented

Let's update the issue summary to also mention the entity.image.preview route. I agree with #40, no need to deprecate.

acbramley’s picture

acbramley commented
Issue summary: View changes

Done

acbramley’s picture

acbramley commented
Issue summary: View changes
mstrelan’s picture

mstrelan commented
Status: Needs review » Reviewed & tested by the community

Looks great

  • catch committed d585d557 on 11.x 
    Issue #2554641 by acbramley, jeremy, bdragon, duaelfr, mstrelan, wim...
catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Nice find. Committed/pushed to 11.x, thanks!

catch closed merge request !11548

Status: Fixed » Closed (fixed)

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