views_ui_ajax_form() checks $view->form_cache['key'] to decide wether the form cache should be invalidated. Unfortunately, when retrieving the top item from the stack, the variable $key that stores the form key gets overwritten with the stack index.

This breaks the subsequent key check: when a string key is compared to an integer key, the string gets implicitely converted to an integer prior to the comparison. Given $view->form_cache['key'] = 'test' and $key = 0, the condition $view->form_cache['key'] != $key evaluates to false, skipping the form cache invalidation.

Proposed solution:

  1. Do not reuse $key when pulling from the stack.
  2. Use a type comparison when checking the form key
CommentFileSizeAuthor
#6 views-form_cache_key_check-2716593-6.patch1.06 KBciss
#3 views-form_cache_key_check-2716593-3.patch1.04 KBciss
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

ciss created an issue. See original summary.

ciss’s picture

ciss CreditAttribution: ciss as a volunteer commented

It looks like this bug is also present in Drupal 8: http://cgit.drupalcode.org/drupal/tree/core/modules/views_ui/src/Form/Aj...

ciss’s picture

ciss CreditAttribution: ciss as a volunteer commented
Status: Active » Needs review
FileSize
views-form_cache_key_check-2716593-3.patch1.04 KB
Chris Matthews’s picture

Chris Matthews CreditAttribution: Chris Matthews as a volunteer commented
Status: Needs review » Needs work
Issue tags: +Needs reroll

The 3 year old patch in #3 to admin.inc does not apply to the latest views 7.x-3.x-dev and if still relevant needs to be rerolled.

Checking patch includes/admin.inc...
error: while searching for:
    // Retrieve the first form from the stack without changing the integer keys,
    // as they're being used for the "2 of 3" progress indicator.
    reset($view->stack);
    list($key, $top) = each($view->stack);
    unset($view->stack[$key]);

    if (array_shift($top) != $identifier) {
      $view->stack = array();

error: patch failed: includes/admin.inc:2915
error: includes/admin.inc: patch does not apply
ciss’s picture

ciss CreditAttribution: ciss as a volunteer commented

@Chris2 Why did you hide the patch file? If you don't have your automated patch checks under control then please refrain from spamming issues.

ciss’s picture

ciss CreditAttribution: ciss as a volunteer commented
Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
views-form_cache_key_check-2716593-6.patch1.06 KB

Rerolled against 2ae973e (2019-01-14).

Chris Matthews’s picture

Chris Matthews CreditAttribution: Chris Matthews as a volunteer commented

Hi Fabian, I hide the patch file in #3 because it did not apply to the latest views 7.x-3.x-dev and therefore needs to be rerolled into a new patch file. Is that not correct? I wasn't spamming, just attempting to revive some older views issues but please let me know if I've done something wrong.

ciss’s picture

ciss CreditAttribution: ciss as a volunteer commented

I hide the patch file in #3 because it did not apply to the latest views 7.x-3.x-dev and therefore needs to be rerolled into a new patch file. Is that not correct?

@Chris2 Please don't hide any patches at all, because:

  • Only obsolete patches should be hidden. Hiding the most recent patch makes it difficult for people to spot it, especially in longer issues.
  • Some issues contain patches for different targets. Some of these may not apply because they are meant for a different branch, but are still used by people.
Chris Matthews’s picture

Chris Matthews CreditAttribution: Chris Matthews as a volunteer commented

Thank you for the help and clarification, understood!