Problem/Motivation

Currently, the cache is not properly cleared (and the test isn't sufficient to test this). To reproduce: check the headers in the test, after clearing the cache and logging out, there is a header X-Drupal-Cache which shouln't have a HIT result but it does have one.

Proposed resolution

In captcha_element_process Line 147f, use the page_cache_kill_switch Service to properly disable caching.

Remaining tasks

Review the patch

User interface changes

None

API changes

None

Comments

LKS90’s picture

LKS90 CreditAttribution: LKS90 at MD Systems GmbH commented
Status: Active » Needs review

Pull request for the issue: https://github.com/chuva-inc/captcha/pull/14

Seems like the pull request also updates the sandboxed github version with the recent rollback from my changes. In case you want some parts removed, changed or whatever, just tell me.

LKS90’s picture

LKS90 CreditAttribution: LKS90 at MD Systems GmbH commented

https://github.com/chuva-inc/captcha/pull/15

I just didn't update my base correctly, new pull request without the rollback.

hass’s picture

hass CreditAttribution: hass commented

We may not need to clear / disable cache for some captchas. Recaptcha as example does not require this at all I think as the html code is static until the global configuration changes.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented
Title: Properly delete cache for modified results » Properly disable page and render cache
Category: Task » Bug report
Priority: Normal » Major
LKS90’s picture

LKS90 CreditAttribution: LKS90 at MD Systems GmbH commented

Take a look at the pull request for an updated version.
Some other issues which get fixed by the pull request:

  • image_captcha has no schema
  • .travis.yml passes with test fails
Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented
Status: Needs review » Reviewed & tested by the community

This looks good to me now. Pretty extensive test coverage that displaying a captcha somewhere on a page (the test explicitly uses a block to test block/render caching) now correctly disabled the page cache.

And it is implemented for each captcha type, so if one doesn't need render caching, then they can decide that for themself.

  • elachlan committed 32948a6 on 8.x-1.x authored by LKS90 
    Issue #2492681 by LKS90, Berdir: Properly disable page and render cache
elachlan’s picture

elachlan CreditAttribution: elachlan commented
Status: Reviewed & tested by the community » Fixed

Thanks!

hass’s picture

hass CreditAttribution: hass commented
Status: Fixed » Needs work

This patch need to be rolled back.

Please only disable cache if a specific captcha requires it. There is no need to disable the cache if recaptcha is in place. As core - we should only disable cache if code needs it, otherwise leave it enabled. A module maintainer need to disable the cache if his module requires this. In this case image captcha need to disable the cache and ONLY if image captcha is shown on the specific page.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented
Status: Needs work » Fixed

Did you actually look at the commit?

That's exactly what is done. image, math and test disable the cache.

Status: Fixed » Closed (fixed)

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

Anybody’s picture

Anybody
Primary language German
Location Porta Westfalica
CreditAttribution: Anybody at DROWL.de commented

Follow-ups:

#3311443: [PP-1][2.x] Expose information about cacheability for each selectable captcha type to inform regular Drupal Site-Owners (non developers) about cacheability of available CAPTCHA types.
and
#3311447: Find a way to allow page caching (including by CDNs) with captcha. AJAX, Lazy, ...? to discuss how to avoid disabling the page cache when placing a regular captcha

Please help to fix these!