// This form's cacheability may have been changed as it is now based
    // on the current user and permissions.
    // TODO: is this line enough?
    $access->addCacheableDependency($form);

I'm not actually sure what this line does.

I do suspect though that we should be adding cachePerUser() and cachePerPermissions() here.

Comments

joachim created an issue. See original summary.

joachim’s picture

joachim commented
Title: access result cache contexts » fix access result caching 
    $access->addCacheableDependency($form);

On further reading, I don't think this can be right, since AccessResult::addCacheableDependency() comes from
https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Cache%21R... and looking at that code, if you pass in a non-object (such as the $form array), then you get this:

    // Not a cacheable dependency, this can not be cached.
    $this->cacheMaxAge = 0;
joachim’s picture

joachim commented

Instead of 

    $access->addCacheableDependency($form);

should maybe be:

    \Drupal::service('renderer')->addCacheableDependency($form, $access);

?

Which would give us overall:

    $access->cachePerUser();
    $access->cachePerPermissions();
    \Drupal::service('renderer')->addCacheableDependency($form, $access);

I'm still fairly mystified by the whole cache context system though, so this is a bit of a guess.

zerolab’s picture

zerolab commented

This whole bit happens in commerce_product_permissions_by_type_form_commerce_order_item_add_to_cart_form_alter()
It does not return any access result. Instead $form['#access'] is changed

<?php
function commerce_product_permissions_by_type_form_commerce_order_item_add_to_cart_form_alter(&$form, FormStateInterface $form_state, $form_id) {
  $storage = $form_state->getStorage();
  if (isset($storage['product'])) {
    /** @var \Drupal\commerce_product\Entity\ProductInterface $product */
    $product = $storage['product'];
    $account = Drupal::currentUser();

    $permission = 'add ' . $product->bundle() . ' commerce_product to cart';
    $access = AccessResult::allowedIfHasPermission($account, $permission);

    // Hide the entire add to cart form if appropriate.
    if (!$access->isAllowed()) {
      $form['#access'] = FALSE;
    }

    // This form's cacheability may have been changed as it is now based
    // on the current user and permissions.
    // TODO: is this line enough?
    $access->addCacheableDependency($form);
  }
}

should become just:

<?php

function commerce_product_permissions_by_type_form_commerce_order_item_add_to_cart_form_alter(&$form, FormStateInterface $form_state, $form_id) {
  $storage = $form_state->getStorage();
  if (isset($storage['product'])) {
    /** @var \Drupal\commerce_product\Entity\ProductInterface $product */
    $product = $storage['product'];
    $permission = 'add ' . $product->bundle() . ' commerce_product to cart';
    $form['#access'] = \Drupal::currentUser()->hasPermission($permission);

    // IMHO this is optional.
    $cacheable_metadata = (new CacheableMetadata())
      ->addCacheContexts(['user.permissions']);

    $cacheable_metadata->applyTo($form);
  }
}

  • erik.erskine committed 60da0d8 on 8.x-1.x 
    Issue #2911461 by joachim, zerolab: fix access result caching
joachim’s picture

joachim commented

Last commit added a debug call to ksm().

erik.erskine credited erik.erskine.

erik.erskine’s picture

erik.erskine commented

Removed the ksm()!

damienmckenna’s picture

damienmckenna
Location TN, USA
commented
Version: » 8.x-1.x-dev

Is there anything remaining on this or is it working correctly now?