Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Steps to reproduce:
- Enable dynamic page cache
- Edit a node
- Preview it
- Change something, e.g. the title
- Preview it again
Expected behavior:
- Second change is visible in preview
Actual behavior
- First preview is cached by dynamic page cache, 2nd change is not reflected in the preview
Comment | File | Size | Author |
---|---|---|---|
#30 | interdiff_3127250_24-30.txt | 642 bytes | ankithashetty |
#30 | 3127250-30.patch | 1.94 KB | ankithashetty |
#24 | interdiff_21_24.txt | 2.69 KB | anmolgoyal74 |
#24 | 3127250-24.patch | 2.03 KB | anmolgoyal74 |
#21 | D8_and_D9_with_tests_Node_preview-21.patch | 2.08 KB | joseph.olstad |
Comments
Comment #2
fagoAttached patch fixes the problem.
Comment #3
fagoThis is probably mitigated by #2949457: Enhance Toolbar's subtree caching so that menu links with CSRF token do not need one subtree cache item per session - so mostly no one has a working dynamic page cache in the admin backend ;-)
Comment #5
fagoqueued a test run against 8.9.x
Comment #6
joseph.olstadThis patch applies cleanly to Drupal 9 also
Comment #7
joseph.olstadComment #8
joseph.olstadthis patch is correct and definately works
Comment #9
jamesyao CreditAttribution: jamesyao commentedReviewed and tested by the community:
the patch is tested in Drupal 9.0.9. It works perfectly.
Thanks @fago for providing the patch.
Comment #10
jamesyao CreditAttribution: jamesyao commentedthe patch is tested in Drupal 9.0.9. It also works perfectly for the anonymous preview node behavior
Thanks @fago for providing the patch.
Comment #11
joseph.olstadGreat fix, hopefully an easy adjustment to the tests can be made to cover this.
Comment #12
catchYes this needs some test coverage, but the change makes sense.
Comment #13
anmolgoyal74 CreditAttribution: anmolgoyal74 at OpenSense Labs for DrupalFit commentedAdded test coverage.
Comment #14
anmolgoyal74 CreditAttribution: anmolgoyal74 at OpenSense Labs for DrupalFit commentedComment #15
joseph.olstadThank you so much for the tests!
Patch Has tests,
RTBC, verified this ourselves, solution is working perfectly, cache is expiring as expected, automatically handled now thanks to this patch.
Was very tricky to debug this and find that we needed this fix, so I highly recommend the fix.
It should be a safe fix for 9.0, 9.1.x , 9.2.x, we've tested it extensively on 9.0.x and 8.8.x
Comment #16
joseph.olstadoh one last thing, someone should upload the tests-only patch to make sure it's failing.
Comment #17
joseph.olstadGreat job @anmolgoyal74 and @fago for the original patch and JamesYao also for his great detective work on this.
Comment #18
alexpottThe test alone does not fail for me.
Comment #19
joseph.olstadhmm,
Comment #21
joseph.olstadnew patch, tests checks both title and body field.
Comment #22
anmolgoyal74 CreditAttribution: anmolgoyal74 at OpenSense Labs for DrupalFit commentedThe test alone does not fail for me as well.
Comment #23
joseph.olstadThe trick to the test is we need to edit , preview, edit AGAIN, then preview, IRL without the patch the second preview shows first preview values
needs work on tests.
Comment #24
anmolgoyal74 CreditAttribution: anmolgoyal74 at OpenSense Labs for DrupalFit commentedI am facing this issue with an anonymous user only. And with node_test enabled, the test only patch does not fail.
Comment #26
joseph.olstadNice work anmolgoyal74 , yes that is our use case also.
Comment #28
joseph.olstadtests only patch works, just hiding it so that it doesn't re-trigger automatically.
Comment #29
catchThis should just be one line, something like 'Tests node preview with dynamic_page_cache and anonymous users'.
Comment #30
ankithashettyUpdated the patch in #24 with the changes suggested in #29. Thank you!
Comment #31
anmolgoyal74 CreditAttribution: anmolgoyal74 at OpenSense Labs for DrupalFit commentedsuggestions in #29 addressed in #30.
I think it is good now for RTBC.
Comment #32
alexpottCommitted and pushed ab9919c356 to 9.2.x and bee62f645e to 9.1.x. Thanks!
Backported to 9.1.x as a container rebuild will happen on the next 9.1.x release and that'll allow this to be fixed with no update function.
Comment #35
joseph.olstadThanks, nice one-liner!
Comment #36
joseph.olstadMy browser
Comment #37
joseph.olstad