Cache is disabled for all view modes instead only for "full" view mode.

this is happening in recently_read_entity_view
Only "full" view module should be disabled in cache.

CommentFileSizeAuthor
#24 3001966-recently_read-caching-fix-v2.patch2.97 KBkazajhodo
#23 recently_read-caching-fix-3001966.patch6.75 KBkazajhodo
#21 recently_read-caching-fix-3001966.patch6.76 KBkazajhodo
#17 interdiff_15-17.txt1.3 KBnmatja
#17 3001966-17.patch1.96 KBnmatja
#15 3001966-15.patch1.04 KBjjtoyas
#11 0001-Use-js-to-add-recently-read-record-on-node-detail.patch7.38 KBmarysmech
#9 reroll_diff_8_9.txt3.93 KBbzoks
#9 3001966-9.patch6.27 KBbzoks
#8 3001966-8.patch6.64 KBeric.vvf
#7 3001966-7.patch6.65 KBjeroent
#5 cache-is-disabled-for-all-view-modes-3001966-5.patch1.86 KBafi13
#3 recently_read_fix_cache_disabled_3001966_3.patch1.91 KBaalin
recently_read_fix_cache_disabled_2954824_1.patch1.7 KBaalin

Comments

aalin created an issue. See original summary.

aalin’s picture

aalin commented
Status: Active » Needs review
aalin’s picture

aalin commented
StatusFileSize
new recently_read_fix_cache_disabled_3001966_3.patch1.91 KB

a more generic patch, to disable the cache only when the service 'recently_read' is used

nkoporec’s picture

nkoporec
Primary language English
Location Slovenia
commented
Status: Needs review » Needs work

Latest patch doesn't apply. Marking it as needs work.

afi13’s picture

afi13 commented
StatusFileSize
new cache-is-disabled-for-all-view-modes-3001966-5.patch1.86 KB
afi13’s picture

afi13 commented
Status: Needs work » Needs review
jeroent’s picture

jeroent
Primary language Dutch
Location 🇧🇪
commented
StatusFileSize
new 3001966-7.patch6.65 KB

What about keeping Drupal caching intact and using javascript to add a record. Just like the statistics module in core.

eric.vvf’s picture

eric.vvf commented
StatusFileSize
new 3001966-8.patch6.64 KB

Just a small change on @JeroenT's patch:

$build['cache']['tags'][] = 'recently_read_type_list';

to 

$build['#cache']['tags'][] = 'recently_read_type_list';

Adds '#' so it will be used as meta.

bzoks’s picture

bzoks commented
StatusFileSize
new 3001966-9.patch6.27 KB
new reroll_diff_8_9.txt3.93 KB

I rerolled patch #8 to 8.x-1.x-dev (it applied cleanly only on released 8.x-1.x).
It seems to work OK.

strozx’s picture

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

Tested and it appears to work

marysmech’s picture

marysmech commented
Status: Reviewed & tested by the community » Needs review
StatusFileSize
new 0001-Use-js-to-add-recently-read-record-on-node-detail.patch7.38 KB

I was unable to apply patch #9 with latest develop. So I just reformated patch from #9 with latest develop. After reformatting everything seems to work nicely.

Status: Needs review » Needs work

The last submitted patch, 11: 0001-Use-js-to-add-recently-read-record-on-node-detail.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

nkoporec’s picture

nkoporec
Primary language English
Location Slovenia
commented

Tests are failing + need's a re-roll.

boshtian’s picture

boshtian commented

I'm looking at this issue and would like to go back to the initial solution of fixing this without changing the way visits are saved.

I'm not saying, that the Javascript approach is bad, I just don't think it's ready, because we currently don't have a fallback for users with javascript disabled. I would rather open a separate issue for moving things to javascript (and discuss the correct approach there) and leave this open if anyone wants to add a patch for existing codebase.

jjtoyas’s picture

jjtoyas commented
StatusFileSize
new 3001966-15.patch1.04 KB

Disable cache for "full" view mode, without JavaScript.

jjtoyas’s picture

jjtoyas commented
Status: Needs work » Needs review
nmatja’s picture

nmatja commented
StatusFileSize
new 3001966-17.patch1.96 KB
new interdiff_15-17.txt1.3 KB

I have tested the patch. The patch fixes the problem and it's working without any problems. I spotted and fixed some minor code standards issues.

  • boshtian committed 9b5cc10 on 8.x-1.x authored by jjtoyas 
    Issue #3001966 by aalin, bzoks, nmatja, jjtoyas, afi13, JeroenT, eric....
boshtian’s picture

boshtian commented
Status: Needs review » Fixed

Status: Fixed » Closed (fixed)

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

kazajhodo’s picture

kazajhodo commented
StatusFileSize
new recently_read-caching-fix-3001966.patch6.76 KB

WHOOPS, don't use this, it has a yml syntax error. Worked locally but not on the server. I can't edit the file, so I'm uploading a new one I guess.

-----
Not exactly sure how I'm supposed to submit this, I see everyone is submitting patch files, so I'll just do that.

This is a reroll of 0001-Use-js-to-add-recently-read-record-on-node-detail.patch against the dev. Had a big site where the caching was broken, found the patch and rerolled it.

kazajhodo’s picture

kazajhodo commented
kazajhodo’s picture

kazajhodo commented
StatusFileSize
new recently_read-caching-fix-3001966.patch6.75 KB

Ok, I think this one is good.

A reroll of #11 by @marysmech.

kazajhodo’s picture

kazajhodo commented
StatusFileSize
new 3001966-recently_read-caching-fix-v2.patch2.97 KB

Reroll for for 1.5.