Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Status: Needs review » Needs work

The last submitted patch, mollom.cache_.0.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
2.67 KB

Quite a bunch of the test failures were expected, since the enforced caching also has an impact on the fake/testing server implementation.

Now, attached is a pretty radical patch, which attempts to eliminate the entire dependency on the form cache.

Btw, #type 'hidden' is totally weird. I did not know that it does not process user input. (There's no form_type_hidden_value() #value_callback function for it.) I don't know why that is, but Drupal core must have thought it is a good idea... (?!)

Status: Needs review » Needs work

The last submitted patch, mollom.cache_.2.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
4.4 KB

Resolved a couple of problems caused by #type 'hidden'.

But now with a pretty challenging @todo about tracking state without the luxury of the form cache. Perhaps something along the lines of a token might work?

Status: Needs review » Needs work

The last submitted patch, mollom.cache_.4.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
32.4 KB

Spent the past days with debugging this to death. Let's see how far we get.

Note: Patch contains tons of commented-out debugging code.

Status: Needs review » Needs work

The last submitted patch, mollom.cache_.6.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
19.56 KB

Exciting!

Now without all of the debugging code.

Status: Needs review » Needs work

The last submitted patch, mollom.cache_.8.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
19.28 KB

If this one comes back green, then we're ready for production/live site testing.

That said, this still needs a lot of code documentation, which I'll work on in parallel.

sun’s picture

Title: Test reverse-proxy/HTTP-level page caches » Text analysis is incompatible with enabled page cache/reverse proxy
Component: Tests » Code
Category: task » bug
Priority: Normal » Critical
FileSize
48.5 KB

Added tons of documentation and final code clean-ups.

sun’s picture

Test validation.

Status: Needs review » Needs work

The last submitted patch, mollom.cache_.test-only.12.patch, failed testing.

sun’s picture

Version: 7.x-2.x-dev » 6.x-2.x-dev
Status: Needs work » Patch (to be ported)

Excellent. Committed and pushed to 7.x-2.x.

That said, these (major) changes are being manually tested on various production sites currently. Based on my own testing, I continue to expect to receive positive confirmations over the next few days.

Meanwhile, we can move forward with backporting these changes to 6.x-2.x. In case any issues will crop up, we can always get back to D7.

sun’s picture

Status: Patch (to be ported) » Needs review
FileSize
49.91 KB

Attached patch is a full backport of the D7 patch. It appears to work identically based on my own testing.

sun’s picture

FileSize
52.7 KB

A couple of additional fixes for D6... but first patch in this serial is still identical to #15.

sun’s picture

Version: 6.x-2.x-dev » 7.x-2.x-dev
Status: Needs review » Fixed

Committed and pushed to 6.x-2.x.

For now, I'm calling this fixed. If anyone experiences any kind of issues through manual testing prior to the new release, please re-open.

sun’s picture

Status: Fixed » Needs review
FileSize
17.7 KB

We identified a couple of minor bugs in manual testing — e.g., the CAPTCHA form element is not hidden under certain circumstances.

Status: Needs review » Needs work

The last submitted patch, mollom.cache_.18.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
18.52 KB

Fixed bogus tests.

sun’s picture

FileSize
16.85 KB

Reverted the CAPTCHA URL changes and commented out test assertions, deferred to #1959904: Retrieve only one CAPTCHA per form submission attempt

sun’s picture

Version: 7.x-2.x-dev » 6.x-2.x-dev
FileSize
17.41 KB
sun’s picture

Version: 6.x-2.x-dev » 7.x-2.x-dev
FileSize
14.38 KB

Postponed #22 and the backport to D6 for now.

Manual testing on production sites discovered some problems that can occur for anonymous users with enabled page + form caching, when repetitively re-validating a form submission. Such form submission flows happen rarely, but yet, they are possible.

Attached patch quadruples the test coverage to reliably catch all of these scenarios.

Status: Needs review » Needs work

The last submitted patch, mollom.cache_.23.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
15.17 KB

Adjusted the Comment form integration test expectations accordingly. This patch should come back green.

sun’s picture

FileSize
10.56 KB

Additionally massively hardening CAPTCHA-only test coverage.

sun’s picture

Version: 7.x-2.x-dev » 6.x-2.x-dev
FileSize
67.16 KB

Committed #25 + #26 to 7.x-2.x.

Now back to the backport... (which caused the 7.x-2.x follow-up patches in the first place)

Let's see what the testbot has to say on this cumulative patch serial.

Status: Needs review » Needs work

The last submitted patch, mollom.cache_.27.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
71.63 KB

In D6, Comment module triggers the form validation stack from within an #after_build callback, before the form is fully built, and without going through drupal_process_form().

Catering for that case required some advanced measures. As far as I can see, this should come back green.

Status: Needs review » Needs work

The last submitted patch, mollom.cache_.29.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
72.15 KB

Hopefully for real now.

sun’s picture

Status: Needs review » Fixed

Thanks for reporting, reviewing, and testing! Committed to all branches.

A new development snapshot will be available within the next 12 hours. This improvement will be available in the next official release.

Status: Fixed » Closed (fixed)

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

aminebourkadi’s picture

does that patch work for D7 version?

  • Commit 34fea77 on 7.x-2.x, 8.x-2.x, fbajs, actions by sun:
    - #1938768 by sun: Hardened test coverage for CAPTCHA-only protected...
  • Commit 89fda7a on 7.x-2.x, 8.x-2.x, fbajs, actions by sun:
    - #1938768 by sun: Fixed text analysis is incompatible with enabled page...
  • Commit a477640 on 7.x-2.x, 8.x-2.x, fbajs, actions by sun:
    - #1938768 by sun: Fixed text analysis validation does not always (re-)...
  • Commit e017f36 on 7.x-2.x, 8.x-2.x, fbajs, actions by sun:
    - #1938768 by sun: Fixed CAPTCHA form element is not always hidden when...

  • Commit 34fea77 on 7.x-2.x, 8.x-2.x, fbajs, actions by sun:
    - #1938768 by sun: Hardened test coverage for CAPTCHA-only protected...
  • Commit 89fda7a on 7.x-2.x, 8.x-2.x, fbajs, actions by sun:
    - #1938768 by sun: Fixed text analysis is incompatible with enabled page...
  • Commit a477640 on 7.x-2.x, 8.x-2.x, fbajs, actions by sun:
    - #1938768 by sun: Fixed text analysis validation does not always (re-)...
  • Commit e017f36 on 7.x-2.x, 8.x-2.x, fbajs, actions by sun:
    - #1938768 by sun: Fixed CAPTCHA form element is not always hidden when...