Problem/Motivation

In #2702423: Commerce Add to Cart Form ajax and ESI / Varnish fails a change has been made that makes it possible to have different expire values in {cache_form} for $form an $form_state. This is obviously bad:

  1. If we have only $form_state but no $form in {cache_form} then we simply get an "Invalid form POST data" from ajax_get_form(). (Note that this is the case if the $form and $form_state cache_form entries expire at the same time, so it's not a big deal.)
  2. The bigger problem occurs if we have only $form but no $form_state. In this case some contrib modules might unexpectedly break (e.g. the add to cart form AJAX), see commerce_cart_add_to_cart_form_validate().

Proposed resolution

Enforce equal expire timestamps for $form and $form_state in cache_form.

CommentFileSizeAuthor
#7 authcache-2825813-7-cache_form_expiry.patch7.13 KBznerol
#7 2825813-7-TEST-ONLY.patch6.44 KBznerol
#5 2825813-4-TEST-ONLY.patch5.14 KBznerol
#2 authcache-2825813-2-cache_form_expiry.patch710 bytesczigor
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

czigor created an issue. See original summary.

czigor’s picture

czigor CreditAttribution: czigor at Liip for FREITAG lab. AG commented
Status: Active » Needs review
FileSize
authcache-2825813-2-cache_form_expiry.patch710 bytes
czigor’s picture

czigor CreditAttribution: czigor at Liip for FREITAG lab. AG commented
Priority: Normal » Major

Increasing priority as this is actually breaking ajax forms.

znerol’s picture

znerol CreditAttribution: znerol commented
Status: Needs review » Needs work

Thanks for reporting this problem and for providing a patch, and sorry for the late answer (was AFK for a looong time).

This is definitely a very ugly problem. I will try to come up with some test-cases in order to lower the risk of more screw-ups in the future :/

znerol’s picture

znerol CreditAttribution: znerol commented
Status: Needs work » Needs review
FileSize
2825813-4-TEST-ONLY.patch5.14 KB

Simple test case. I'd like to add another test for forms which were rebuilt (i.e., forms without the immutable flag). I'd like to ensure that on those cache entries the expiry time is not increased.

Status: Needs review » Needs work

The last submitted patch, 5: 2825813-4-TEST-ONLY.patch, failed testing.

znerol’s picture

znerol CreditAttribution: znerol commented
Status: Needs work » Needs review
FileSize
2825813-7-TEST-ONLY.patch6.44 KB
authcache-2825813-7-cache_form_expiry.patch7.13 KB

The last submitted patch, 7: 2825813-7-TEST-ONLY.patch, failed testing.

The last submitted patch, 7: 2825813-7-TEST-ONLY.patch, failed testing.

The last submitted patch, 7: 2825813-7-TEST-ONLY.patch, failed testing.

The last submitted patch, 7: 2825813-7-TEST-ONLY.patch, failed testing.

  • znerol committed e6fc683 on 7.x-2.x authored by czigor 
    Issue #2825813 by znerol, czigor: $form and $form_state get different...
znerol’s picture

znerol CreditAttribution: znerol commented
Status: Needs review » Fixed

Thanks @czigor for reporting the issue and submitting a patch.

Status: Fixed » Closed (fixed)

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

ordermind’s picture

ordermind CreditAttribution: ordermind commented

I'm still seeing the problem even with the patch applied, so I'd like this issue to be reopened.

znerol’s picture

znerol CreditAttribution: znerol commented

@ordermind please do not comment on closed issue. Instead open a new support request describing the problem and the steps to reproduce.

For the record, this patch has been included into 7.x-2.1 a year ago. Please upgrade your installation to at least that version.