Issue #2328777 by tim.plunkett: Refactor FAPI getCache()/setCache() into a standalone class.

core/core.services.yml

@@ -160,13 +160,17 @@ services:
     arguments: [default]
   form_builder:
     class: Drupal\Core\Form\FormBuilder
-    arguments: ['@form_validator', '@form_submitter', '@module_handler', '@keyvalue.expirable', '@event_dispatcher', '@request_stack', '@class_resolver', '@theme.manager', '@?csrf_token', '@?http_kernel']
+    arguments: ['@form_validator', '@form_submitter', '@form_cache', '@module_handler', '@event_dispatcher', '@request_stack', '@class_resolver', '@theme.manager', '@?csrf_token', '@?http_kernel']
   form_validator:
     class: Drupal\Core\Form\FormValidator
     arguments: ['@request_stack', '@string_translation', '@csrf_token', '@logger.channel.form']
   form_submitter:
     class: Drupal\Core\Form\FormSubmitter
     arguments: ['@request_stack', '@url_generator']
+  form_cache:
+    class: Drupal\Core\Form\FormCache
+    arguments: ['@keyvalue.expirable', '@module_handler', '@current_user', '@csrf_token']
+    public: false  # Private to form_builder
   keyvalue:
     class: Drupal\Core\KeyValueStore\KeyValueFactory
     arguments: ['@service_container', '%factory.keyvalue%']

core/includes/form.inc

@@ -23,7 +23,7 @@
  * @deprecated in Drupal 8.x-dev, will be removed before Drupal 8.0.
  *   Use \Drupal::formBuilder()->getCache().
  *
- * @see \Drupal\Core\Form\FormBuilderInterface::getCache().
+ * @see \Drupal\Core\Form\FormCacheInterface::getCache().
  */
 function form_get_cache($form_build_id, FormStateInterface $form_state) {
   return \Drupal::formBuilder()->getCache($form_build_id, $form_state);
@@ -35,7 +35,7 @@ function form_get_cache($form_build_id, FormStateInterface $form_state) {
  * @deprecated in Drupal 8.x-dev, will be removed before Drupal 8.0.
  *   Use \Drupal::formBuilder()->setCache().
  *
- * @see \Drupal\Core\Form\FormBuilderInterface::setCache().
+ * @see \Drupal\Core\Form\FormCacheInterface::setCache().
  */
 function form_set_cache($form_build_id, $form, FormStateInterface $form_state) {
   \Drupal::formBuilder()->setCache($form_build_id, $form, $form_state);

core/lib/Drupal/Core/Form/FormBuilder.php

@@ -9,13 +9,11 @@
 
 use Drupal\Component\Utility\Crypt;
 use Drupal\Component\Utility\NestedArray;
-use Drupal\Component\Utility\SafeMarkup;
 use Drupal\Component\Utility\String;
 use Drupal\Component\Utility\UrlHelper;
 use Drupal\Core\Access\CsrfTokenGenerator;
 use Drupal\Core\DependencyInjection\ClassResolverInterface;
 use Drupal\Core\Extension\ModuleHandlerInterface;
-use Drupal\Core\KeyValueStore\KeyValueExpirableFactoryInterface;
 use Drupal\Core\Render\Element;
 use Drupal\Core\Site\Settings;
 use Drupal\Core\Theme\ThemeManagerInterface;
@@ -32,7 +30,7 @@
  *
  * @ingroup form_api
  */
-class FormBuilder implements FormBuilderInterface, FormValidatorInterface, FormSubmitterInterface {
+class FormBuilder implements FormBuilderInterface, FormValidatorInterface, FormSubmitterInterface, FormCacheInterface {
 
   /**
    * The module handler.
@@ -41,13 +39,6 @@ class FormBuilder implements FormBuilderInterface, FormValidatorInterface, FormS
    */
   protected $moduleHandler;
 
-  /**
-   * The factory for expirable key value stores used by form cache.
-   *
-   * @var \Drupal\Core\KeyValueStore\KeyValueExpirableFactoryInterface
-   */
-  protected $keyValueExpirableFactory;
-
   /**
    * The event dispatcher.
    *
@@ -107,6 +98,13 @@ class FormBuilder implements FormBuilderInterface, FormValidatorInterface, FormS
    */
   protected $formSubmitter;
 
+  /**
+   * The form cache.
+   *
+   * @var \Drupal\Core\Form\FormCacheInterface
+   */
+  protected $formCache;
+
   /**
    * Constructs a new FormBuilder.
    *
@@ -114,10 +112,10 @@ class FormBuilder implements FormBuilderInterface, FormValidatorInterface, FormS
    *   The form validator.
    * @param \Drupal\Core\Form\FormSubmitterInterface $form_submitter
    *   The form submission processor.
+   * @oaram \Drupal\Core\Form\FormCacheInterface $form_cache
+   *   The form cache.
    * @param \Drupal\Core\Extension\ModuleHandlerInterface $module_handler
    *   The module handler.
-   * @param \Drupal\Core\KeyValueStore\KeyValueExpirableFactoryInterface $key_value_expirable_factory
-   *   The keyvalue expirable factory.
    * @param \Symfony\Component\EventDispatcher\EventDispatcherInterface $event_dispatcher
    *   The event dispatcher.
    * @param \Symfony\Component\HttpFoundation\RequestStack $request_stack
@@ -128,14 +126,14 @@ class FormBuilder implements FormBuilderInterface, FormValidatorInterface, FormS
    *   The theme manager.
    * @param \Drupal\Core\Access\CsrfTokenGenerator $csrf_token
    *   The CSRF token generator.
-   * @param \Drupal\Core\HttpKernel $http_kernel
+   * @param \Symfony\Component\HttpKernel\HttpKernel $http_kernel
    *   The HTTP kernel.
    */
-  public function __construct(FormValidatorInterface $form_validator, FormSubmitterInterface $form_submitter, ModuleHandlerInterface $module_handler, KeyValueExpirableFactoryInterface $key_value_expirable_factory, EventDispatcherInterface $event_dispatcher, RequestStack $request_stack, ClassResolverInterface $class_resolver, ThemeManagerInterface $theme_manager, CsrfTokenGenerator $csrf_token = NULL, HttpKernel $http_kernel = NULL) {
+  public function __construct(FormValidatorInterface $form_validator, FormSubmitterInterface $form_submitter, FormCacheInterface $form_cache, ModuleHandlerInterface $module_handler, EventDispatcherInterface $event_dispatcher, RequestStack $request_stack, ClassResolverInterface $class_resolver, ThemeManagerInterface $theme_manager, CsrfTokenGenerator $csrf_token = NULL, HttpKernel $http_kernel = NULL) {
     $this->formValidator = $form_validator;
     $this->formSubmitter = $form_submitter;
+    $this->formCache = $form_cache;
     $this->moduleHandler = $module_handler;
-    $this->keyValueExpirableFactory = $key_value_expirable_factory;
     $this->eventDispatcher = $event_dispatcher;
     $this->requestStack = $request_stack;
     $this->classResolver = $class_resolver;
@@ -325,63 +323,15 @@ public function rebuildForm($form_id, FormStateInterface &$form_state, $old_form
   /**
    * {@inheritdoc}
    */
-  public function getCache($form_build_id, FormStateInterface &$form_state) {
-    if ($form = $this->keyValueExpirableFactory->get('form')->get($form_build_id)) {
-      $user = $this->currentUser();
-      if ((isset($form['#cache_token']) && $this->csrfToken->validate($form['#cache_token'])) || (!isset($form['#cache_token']) && $user->isAnonymous())) {
-        if ($stored_form_state = $this->keyValueExpirableFactory->get('form_state')->get($form_build_id)) {
-          // Re-populate $form_state for subsequent rebuilds.
-          $form_state->setFormState($stored_form_state);
-
-          // If the original form is contained in include files, load the files.
-          // @see form_load_include()
-          $form_state['build_info'] += array('files' => array());
-          foreach ($form_state['build_info']['files'] as $file) {
-            if (is_array($file)) {
-              $file += array('type' => 'inc', 'name' => $file['module']);
-              $this->moduleHandler->loadInclude($file['module'], $file['type'], $file['name']);
-            }
-            elseif (file_exists($file)) {
-              require_once DRUPAL_ROOT . '/' . $file;
-            }
-          }
-          // Retrieve the list of previously known safe strings and store it
-          // for this request.
-          // @todo Ensure we are not storing an excessively large string list
-          //   in: https://www.drupal.org/node/2295823
-          $form_state['build_info'] += array('safe_strings' => array());
-          SafeMarkup::setMultiple($form_state['build_info']['safe_strings']);
-          unset($form_state['build_info']['safe_strings']);
-        }
-        return $form;
-      }
-    }
+  public function getCache($form_build_id, FormStateInterface $form_state) {
+    return $this->formCache->getCache($form_build_id, $form_state);
   }
 
   /**
    * {@inheritdoc}
    */
   public function setCache($form_build_id, $form, FormStateInterface $form_state) {
-    // 6 hours cache life time for forms should be plenty.
-    $expire = 21600;
-
-    // Cache form structure.
-    if (isset($form)) {
-      if ($this->currentUser()->isAuthenticated()) {
-        $form['#cache_token'] = $this->csrfToken->get();
-      }
-      $this->keyValueExpirableFactory->get('form')->setWithExpire($form_build_id, $form, $expire);
-    }
-
-    // Cache form state.
-    // Store the known list of safe strings for form re-use.
-    // @todo Ensure we are not storing an excessively large string list in:
-    //   https://www.drupal.org/node/2295823
-    $form_state->addBuildInfo('safe_strings', SafeMarkup::getAll());
-
-    if ($data = $form_state->getCacheableArray()) {
-      $this->keyValueExpirableFactory->get('form_state')->setWithExpire($form_build_id, $data, $expire);
-    }
+    $this->formCache->setCache($form_build_id, $form, $form_state);
   }
 
   /**

core/lib/Drupal/Core/Form/FormBuilderInterface.php

@@ -111,16 +111,6 @@ public function buildForm($form_id, FormStateInterface &$form_state);
    */
   public function rebuildForm($form_id, FormStateInterface &$form_state, $old_form = NULL);
 
-  /**
-   * Fetches a form from the cache.
-   */
-  public function getCache($form_build_id, FormStateInterface &$form_state);
-
-  /**
-   * Stores a form in the cache.
-   */
-  public function setCache($form_build_id, $form, FormStateInterface $form_state);
-
   /**
    * Retrieves, populates, and processes a form.
    *

core/lib/Drupal/Core/Form/FormCache.php

+<?php
+
+/**
+ * @file
+ * Contains \Drupal\Core\Form\FormCache.
+ */
+
+namespace Drupal\Core\Form;
+
+use Drupal\Component\Utility\SafeMarkup;
+use Drupal\Core\Access\CsrfTokenGenerator;
+use Drupal\Core\Extension\ModuleHandlerInterface;
+use Drupal\Core\KeyValueStore\KeyValueExpirableFactoryInterface;
+use Drupal\Core\Session\AccountInterface;
+
+/**
+ * Encapsulates the caching of a form and its form state.
+ *
+ * @ingroup form_api
+ */
+class FormCache implements FormCacheInterface {
+
+  /**
+   * The factory for expirable key value stores used by form cache.
+   *
+   * @var \Drupal\Core\KeyValueStore\KeyValueExpirableFactoryInterface
+   */
+  protected $keyValueExpirableFactory;
+
+  /**
+   * The CSRF token generator to validate the form token.
+   *
+   * @var \Drupal\Core\Access\CsrfTokenGenerator
+   */
+  protected $csrfToken;
+
+  /**
+   * The current user.
+   *
+   * @var \Drupal\Core\Session\AccountInterface
+   */
+  protected $currentUser;
+
+  /**
+   * The module handler.
+   *
+   * @var \Drupal\Core\Extension\ModuleHandlerInterface
+   */
+  protected $moduleHandler;
+
+  /**
+   * Constructs a new FormCache.
+   *
+   * @param \Drupal\Core\KeyValueStore\KeyValueExpirableFactoryInterface $key_value_expirable_factory
+   *   The key value expirable factory, used to create key value expirable
+   *   stores for the form cache and form state cache.
+   * @param \Drupal\Core\Extension\ModuleHandlerInterface $module_handler
+   *   The module handler.
+   * @param \Drupal\Core\Session\AccountInterface $current_user
+   *   The current user.
+   * @param \Drupal\Core\Access\CsrfTokenGenerator $csrf_token
+   *   The CSRF token generator.
+   */
+  public function __construct(KeyValueExpirableFactoryInterface $key_value_expirable_factory, ModuleHandlerInterface $module_handler, AccountInterface $current_user, CsrfTokenGenerator $csrf_token = NULL) {
+    $this->keyValueExpirableFactory = $key_value_expirable_factory;
+    $this->moduleHandler = $module_handler;
+    $this->currentUser = $current_user;
+    $this->csrfToken = $csrf_token;
+  }
+
+  /**
+   * {@inheritdoc}
+   */
+  public function getCache($form_build_id, FormStateInterface $form_state) {
+    if ($form = $this->keyValueExpirableFactory->get('form')->get($form_build_id)) {
+      if ((isset($form['#cache_token']) && $this->csrfToken->validate($form['#cache_token'])) || (!isset($form['#cache_token']) && $this->currentUser->isAnonymous())) {
+        $this->loadCachedFormState($form_build_id, $form_state);
+        return $form;
+      }
+    }
+  }
+
+  /**
+   * Loads the cached form state.
+   *
+   * @param string $form_build_id
+   *   The unique form build ID.
+   * @param \Drupal\Core\Form\FormStateInterface $form_state
+   *   The current state of the form.
+   */
+  protected function loadCachedFormState($form_build_id, FormStateInterface $form_state) {
+    if ($stored_form_state = $this->keyValueExpirableFactory->get('form_state')->get($form_build_id)) {
+      // Re-populate $form_state for subsequent rebuilds.
+      $form_state->setFormState($stored_form_state);
+
+      // If the original form is contained in include files, load the files.
+      // @see form_load_include()
+      $form_state['build_info'] += array('files' => array());
+      foreach ($form_state['build_info']['files'] as $file) {
+        if (is_array($file)) {
+          $file += array('type' => 'inc', 'name' => $file['module']);
+          $this->moduleHandler->loadInclude($file['module'], $file['type'], $file['name']);
+        }
+        elseif (file_exists($file)) {
+          require_once DRUPAL_ROOT . '/' . $file;
+        }
+      }
+      // Retrieve the list of previously known safe strings and store it
+      // for this request.
+      // @todo Ensure we are not storing an excessively large string list
+      //   in: https://www.drupal.org/node/2295823
+      $form_state['build_info'] += array('safe_strings' => array());
+      SafeMarkup::setMultiple($form_state['build_info']['safe_strings']);
+      unset($form_state['build_info']['safe_strings']);
+    }
+  }
+
+  /**
+   * {@inheritdoc}
+   */
+  public function setCache($form_build_id, $form, FormStateInterface $form_state) {
+    // 6 hours cache life time for forms should be plenty.
+    $expire = 21600;
+
+    // Cache form structure.
+    if (isset($form)) {
+      if ($this->currentUser->isAuthenticated()) {
+        $form['#cache_token'] = $this->csrfToken->get();
+      }
+      $this->keyValueExpirableFactory->get('form')->setWithExpire($form_build_id, $form, $expire);
+    }
+
+    // Cache form state.
+    // Store the known list of safe strings for form re-use.
+    // @todo Ensure we are not storing an excessively large string list in:
+    //   https://www.drupal.org/node/2295823
+    $form_state->addBuildInfo('safe_strings', SafeMarkup::getAll());
+
+    if ($data = $form_state->getCacheableArray()) {
+      $this->keyValueExpirableFactory->get('form_state')->setWithExpire($form_build_id, $data, $expire);
+    }
+  }
+
+}

core/lib/Drupal/Core/Form/FormCacheInterface.php

+<?php
+
+/**
+ * @file
+ * Contains \Drupal\Core\Form\FormCacheInterface.
+ */
+
+namespace Drupal\Core\Form;
+
+/**
+ * Provides an interface for the caching of a form and its form state.
+ */
+interface FormCacheInterface {
+
+  /**
+   * Fetches a form from the cache.
+   *
+   * @param string $form_build_id
+   *   The unique form build ID.
+   * @param \Drupal\Core\Form\FormStateInterface $form_state
+   *   The current state of the form.
+   */
+  public function getCache($form_build_id, FormStateInterface $form_state);
+
+  /**
+   * Stores a form in the cache.
+   *
+   * @param string $form_build_id
+   *   The unique form build ID.
+   * @param array $form
+   *   The form to cache.
+   * @param \Drupal\Core\Form\FormStateInterface $form_state
+   *   The current state of the form.
+   */
+  public function setCache($form_build_id, $form, FormStateInterface $form_state);
+
+}

core/tests/Drupal/Tests/Core/Form/FormBuilderTest.php

@@ -336,21 +336,6 @@ public function testGetCache() {
       ->method('buildForm')
       ->will($this->returnValue($expected_form));
 
-    // The CSRF token is checked each time.
-    $this->csrfToken->expects($this->exactly(2))
-      ->method('get')
-      ->will($this->returnValue('csrf_token'));
-    // The CSRF token is validated only when retrieving from the cache.
-    $this->csrfToken->expects($this->once())
-      ->method('validate')
-      ->with('csrf_token')
-      ->will($this->returnValue(TRUE));
-    // The user is checked for authentication once for the form building and
-    // twice for each cache set.
-    $this->account->expects($this->exactly(3))
-      ->method('isAuthenticated')
-      ->will($this->returnValue(TRUE));
-
     // Do an initial build of the form and track the build ID.
     $form_state = new FormState();
     $form_state['build_info']['args'] = array();
@@ -363,13 +348,8 @@ public function testGetCache() {
     // The form cache, form_state cache, and CSRF token validation will only be
     // called on the cached form.
     $this->formCache->expects($this->once())
-      ->method('setWithExpire');
-    $this->formCache->expects($this->once())
-      ->method('get')
-      ->will($this->returnValue($cached_form));
-    $this->formStateCache->expects($this->once())
-      ->method('get')
-      ->will($this->returnValue($form_state->getCacheableArray()));
+      ->method('getCache')
+      ->willReturn($form);
 
     // The final form build will not trigger any actual form building, but will
     // use the form cache.

core/tests/Drupal/Tests/Core/Form/FormCacheTest.php

+<?php
+
+/**
+ * @file
+ * Contains \Drupal\Tests\Core\Form\FormCacheTest.
+ */
+
+namespace Drupal\Tests\Core\Form;
+
+use Drupal\Component\Utility\SafeMarkup;
+use Drupal\Core\Form\FormCache;
+use Drupal\Core\Form\FormState;
+use Drupal\Core\Session\AccountInterface;
+use Drupal\Tests\UnitTestCase;
+
+/**
+ * @coversDefaultClass \Drupal\Core\Form\FormCache
+ * @group Form
+ * @backupStaticAttributes enabled
+ */
+class FormCacheTest extends UnitTestCase {
+
+  /**
+   * The form cache object under test.
+   *
+   * @var \Drupal\Core\Form\FormCache
+   */
+  protected $formCache;
+
+  /**
+   * The expirable key value factory.
+   *
+   * @var \Drupal\Core\KeyValueStore\KeyValueExpirableFactoryInterface|\PHPUnit_Framework_MockObject_MockObject
+   */
+  protected $keyValueExpirableFactory;
+
+  /**
+   * The current user.
+   *
+   * @var \Drupal\Core\Session\AccountInterface|\PHPUnit_Framework_MockObject_MockObject
+   */
+  protected $account;
+
+  /**
+   * The CSRF token generator.
+   *
+   * @var \Drupal\Core\Access\CsrfTokenGenerator|\PHPUnit_Framework_MockObject_MockObject
+   */
+  protected $csrfToken;
+
+  /**
+   * The mocked module handler.
+   *
+   * @var \Drupal\Core\Extension\ModuleHandlerInterface|\PHPUnit_Framework_MockObject_MockObject
+   */
+  protected $moduleHandler;
+
+  /**
+   * The expirable key value store used by form cache.
+   *
+   * @var \Drupal\Core\KeyValueStore\KeyValueStoreExpirableInterface|\PHPUnit_Framework_MockObject_MockObject
+   */
+  protected $formCacheStore;
+
+  /**
+   * The expirable key value store used by form state cache.
+   *
+   * @var \Drupal\Core\KeyValueStore\KeyValueStoreExpirableInterface|\PHPUnit_Framework_MockObject_MockObject
+   */
+  protected $formStateCacheStore;
+
+  /**
+   * {@inheritdoc}
+   */
+  protected function setUp() {
+    parent::setUp();
+
+    $this->moduleHandler = $this->getMock('Drupal\Core\Extension\ModuleHandlerInterface');
+
+    $this->formCacheStore = $this->getMock('Drupal\Core\KeyValueStore\KeyValueStoreExpirableInterface');
+    $this->formStateCacheStore = $this->getMock('Drupal\Core\KeyValueStore\KeyValueStoreExpirableInterface');
+    $this->keyValueExpirableFactory = $this->getMock('Drupal\Core\KeyValueStore\KeyValueExpirableFactoryInterface');
+    $this->keyValueExpirableFactory->expects($this->any())
+      ->method('get')
+      ->will($this->returnValueMap([
+        ['form', $this->formCacheStore],
+        ['form_state', $this->formStateCacheStore],
+      ]));
+
+    $this->csrfToken = $this->getMockBuilder('Drupal\Core\Access\CsrfTokenGenerator')
+      ->disableOriginalConstructor()
+      ->getMock();
+    $this->account = $this->getMock('Drupal\Core\Session\AccountInterface');
+    $this->formCache = new FormCache($this->keyValueExpirableFactory, $this->moduleHandler, $this->account, $this->csrfToken);
+  }
+
+  /**
+   * @covers ::getCache
+   */
+  public function testGetCacheValidToken() {
+    $form_build_id = 'the_form_build_id';
+    $form_state = new FormState();
+    $cache_token = 'the_cache_token';
+    $cached_form = ['#cache_token' => $cache_token];
+
+    $this->formCacheStore->expects($this->once())
+      ->method('get')
+      ->with($form_build_id)
+      ->willReturn($cached_form);
+    $this->csrfToken->expects($this->once())
+      ->method('validate')
+      ->with($cache_token)
+      ->willReturn(TRUE);
+    $this->account->expects($this->never())
+      ->method('isAnonymous');
+
+    $form = $this->formCache->getCache($form_build_id, $form_state);
+    $this->assertSame($cached_form, $form);
+  }
+
+  /**
+   * @covers ::getCache
+   */
+  public function testGetCacheInvalidToken() {
+    $form_build_id = 'the_form_build_id';
+    $form_state = new FormState();
+    $cache_token = 'the_cache_token';
+    $cached_form = ['#cache_token' => $cache_token];
+
+    $this->formCacheStore->expects($this->once())
+      ->method('get')
+      ->with($form_build_id)
+      ->willReturn($cached_form);
+    $this->csrfToken->expects($this->once())
+      ->method('validate')
+      ->with($cache_token)
+      ->willReturn(FALSE);
+    $this->account->expects($this->never())
+      ->method('isAnonymous');
+
+    $form = $this->formCache->getCache($form_build_id, $form_state);
+    $this->assertNull($form);
+  }
+
+  /**
+   * @covers ::getCache
+   */
+  public function testGetCacheAnonUser() {
+    $form_build_id = 'the_form_build_id';
+    $form_state = new FormState();
+    $cached_form = ['#cache_token' => NULL];
+
+    $this->formCacheStore->expects($this->once())
+      ->method('get')
+      ->with($form_build_id)
+      ->willReturn($cached_form);
+    $this->account->expects($this->once())
+      ->method('isAnonymous')
+      ->willReturn(TRUE);
+    $this->csrfToken->expects($this->never())
+      ->method('validate');
+
+    $form = $this->formCache->getCache($form_build_id, $form_state);
+    $this->assertSame($cached_form, $form);
+  }
+
+  /**
+   * @covers ::getCache
+   */
+  public function testGetCacheAuthUser() {
+    $form_build_id = 'the_form_build_id';
+    $form_state = new FormState();
+    $cached_form = ['#cache_token' => NULL];
+
+    $this->formCacheStore->expects($this->once())
+      ->method('get')
+      ->with($form_build_id)
+      ->willReturn($cached_form);
+    $this->account->expects($this->once())
+      ->method('isAnonymous')
+      ->willReturn(FALSE);
+
+    $form = $this->formCache->getCache($form_build_id, $form_state);