Issue #2849674 by mxh, Lendude, podarok, pingwin4eg, andypost, axel.rutz, catch: Complex job in ViewExecutable::unserialize() causes data corruption

core/modules/views/src/ViewExecutable.php

@@ -4,7 +4,6 @@ namespace Drupal\views;
 
 use Drupal\Component\Utility\Html;
 use Drupal\Component\Utility\Tags;
-use Drupal\Core\DependencyInjection\DependencySerializationTrait;
 use Drupal\Core\Routing\RouteProviderInterface;
 use Drupal\Core\Session\AccountInterface;
 use Drupal\views\Plugin\views\display\DisplayRouterInterface;
@@ -17,9 +16,14 @@ use Symfony\Component\Routing\Exception\RouteNotFoundException;
  *
  * An object to contain all of the data to generate a view, plus the member
  * functions to build the view query, execute the query and render the output.
+ *
+ * This class does not implement the Serializable interface since problems
+ * occurred when using the serialize method.
+ *
+ * @see https://www.drupal.org/node/2849674
+ * @see https://bugs.php.net/bug.php?id=66052
  */
-class ViewExecutable implements \Serializable {
+class ViewExecutable {
-  use DependencySerializationTrait;
 
   /**
    * The config entity in which the view is stored.
@@ -434,6 +438,13 @@ class ViewExecutable implements \Serializable {
    */
   protected $baseEntityType;
 
+  /**
+   * Holds all necessary data for proper unserialization.
+   *
+   * @var array
+   */
+  protected $serializationData;
+
   /**
    * Constructs a new ViewExecutable object.
    *
@@ -2466,52 +2477,68 @@ class ViewExecutable implements \Serializable {
   }
 
   /**
-   * {@inheritdoc}
+   * Magic method implementation to serialize the view executable.
+   *
+   * @return array
+   *   The names of all variables that should be serialized.
    */
-  public function serialize() {
+  public function __sleep() {
-    return serialize([
+    // Limit to only the required data which is needed to properly restore the
-      // Only serialize the storage entity ID.
+    // state during unserialization.
-      $this->storage->id(),
+    $this->serializationData = [
-      $this->current_display,
+      'storage' => $this->storage->id(),
-      $this->args,
+      'views_data' => $this->viewsData->_serviceId,
-      $this->current_page,
+      'route_provider' => $this->routeProvider->_serviceId,
-      $this->exposed_input,
+      'current_display' => $this->current_display,
-      $this->exposed_raw_input,
+      'args' => $this->args,
-      $this->exposed_data,
+      'current_page' => $this->current_page,
-      $this->dom_id,
+      'exposed_input' => $this->exposed_input,
-      $this->executed,
+      'exposed_raw_input' => $this->exposed_raw_input,
-    ]);
+      'exposed_data' => $this->exposed_data,
+      'dom_id' => $this->dom_id,
+      'executed' => $this->executed,
+    ];
+    return ['serializationData'];
   }
 
   /**
-   * {@inheritdoc}
+   * Magic method implementation to unserialize the view executable.
    */
-  public function unserialize($serialized) {
+  public function __wakeup() {
-    list($storage, $current_display, $args, $current_page, $exposed_input, $exposed_raw_input, $exposed_data, $dom_id, $executed) = unserialize($serialized);
+    // There are cases, like in testing where we don't have a container
-
-    // There are cases, like in testing, where we don't have a container
     // available.
-    if (\Drupal::hasContainer()) {
+    if (\Drupal::hasContainer() && !empty($this->serializationData)) {
-      $this->setRequest(\Drupal::request());
+      // Load and reference the storage.
+      $this->storage = \Drupal::entityTypeManager()->getStorage('view')
+        ->load($this->serializationData['storage']);
+      $this->storage->set('executable', $this);
+
+      // Attach all necessary services.
       $this->user = \Drupal::currentUser();
+      $this->viewsData = \Drupal::service($this->serializationData['views_data']);
+      $this->routeProvider = \Drupal::service($this->serializationData['route_provider']);
 
-      $this->storage = \Drupal::entityManager()->getStorage('view')->load($storage);
+      // Restore the state of this executable.
-
+      if ($request = \Drupal::request()) {
-      $this->setDisplay($current_display);
+        $this->setRequest($request);
-      $this->setArguments($args);
+      }
-      $this->setCurrentPage($current_page);
+      $this->setDisplay($this->serializationData['current_display']);
-      $this->setExposedInput($exposed_input);
+      $this->setArguments($this->serializationData['args']);
-      $this->exposed_data = $exposed_data;
+      $this->setCurrentPage($this->serializationData['current_page']);
-      $this->exposed_raw_input = $exposed_raw_input;
+      $this->setExposedInput($this->serializationData['exposed_input']);
-      $this->dom_id = $dom_id;
+      $this->exposed_data = $this->serializationData['exposed_data'];
+      $this->exposed_raw_input = $this->serializationData['exposed_raw_input'];
+      $this->dom_id = $this->serializationData['dom_id'];
 
       $this->initHandlers();
 
       // If the display was previously executed, execute it now.
-      if ($executed) {
+      if ($this->serializationData['executed']) {
         $this->execute($this->current_display);
       }
     }
+    // Unset serializationData since it serves no further purpose.
+    unset($this->serializationData);
   }
 
 }

core/modules/views/tests/modules/user_batch_action_test/config/install/system.action.user_batch_action_test_action.yml

@@ -0,0 +1,10 @@
+langcode: en
+status: true
+dependencies:
+  module:
+    - user
+id: user_batch_action_test_action
+label: 'Process user in batch'
+type: user
+plugin: user_batch_action_test_action
+configuration: {  }

core/modules/views/tests/modules/user_batch_action_test/config/schema/user_batch_action_test.schema.yml

@@ -0,0 +1,3 @@
+action.configuration.user_batch_action_test_action:
+  type: action_configuration_default
+  label: 'Process user in batch'

core/modules/views/tests/modules/user_batch_action_test/src/Plugin/Action/BatchUserAction.php

@@ -0,0 +1,87 @@
+<?php
+
+namespace Drupal\user_batch_action_test\Plugin\Action;
+
+use Drupal\Core\Action\ActionBase;
+use Drupal\Core\Entity\ContentEntityInterface;
+use Drupal\Core\Session\AccountInterface;
+
+/**
+ * Provides action that sets batch precessing.
+ *
+ * @Action(
+ *   id = "user_batch_action_test_action",
+ *   label = @Translation("Process user in batch"),
+ *   type = "user",
+ * )
+ */
+class BatchUserAction extends ActionBase {
+
+  /**
+   * {@inheritdoc}
+   */
+  public function executeMultiple(array $entities) {
+    $operations = [];
+
+    foreach ($entities as $entity) {
+      $operations[] = [
+        [get_class($this), 'processBatch'],
+        [[
+          'entity_type' => $entity->getEntityTypeId(),
+          'entity_id' => $entity->id(),
+        ]],
+      ];
+    }
+
+    if ($operations) {
+      $batch = [
+        'operations' => $operations,
+        'finished' => [get_class($this), 'finishBatch'],
+      ];
+      batch_set($batch);
+    }
+  }
+
+  /**
+   * {@inheritdoc}
+   */
+  public function execute(ContentEntityInterface $entity = NULL) {
+    $this->executeMultiple([$entity]);
+  }
+
+  /**
+   * {@inheritdoc}
+   */
+  public function access($object, AccountInterface $account = NULL, $return_as_object = FALSE) {
+    return TRUE;
+  }
+
+  /**
+   * Processes the batch item.
+   *
+   * @param array $data
+   *   Keyed array of data to process.
+   * @param array $context
+   *   The batch context.
+   */
+  public static function processBatch($data, &$context) {
+    if (!isset($context['results']['processed'])) {
+      $context['results']['processed'] = 0;
+    }
+    $context['results']['processed']++;
+  }
+
+  /**
+   * Finish batch.
+   *
+   * @param bool $success
+   *   Indicates whether the batch process was successful.
+   * @param array $results
+   *   Results information passed from the processing callback.
+   */
+  public static function finishBatch($success, $results) {
+    drupal_set_message(\Drupal::translation()
+      ->formatPlural($results['processed'], 'One item has been processed.', '@count items have been processed.'));
+  }
+
+}

core/modules/views/tests/modules/user_batch_action_test/user_batch_action_test.info.yml

@@ -0,0 +1,9 @@
+name: 'User batch action test'
+type: module
+description: 'Support module for user batch action testing.'
+package: Testing
+version: VERSION
+core: 8.x
+dependencies:
+  - views
+  - user

core/modules/views/tests/src/Functional/UserBatchActionTest.php

@@ -0,0 +1,45 @@
+<?php
+
+namespace Drupal\Tests\views\Functional;
+
+use Drupal\Tests\BrowserTestBase;
+
+/**
+ * Tests the views bulk form with batch action.
+ *
+ * @group action
+ * @see \Drupal\system\Plugin\views\field\BulkForm
+ */
+class UserBatchActionTest extends BrowserTestBase {
+
+  /**
+   * {@inheritdoc}
+   */
+  public static $modules = ['user', 'user_batch_action_test'];
+
+  /**
+   * Tests user admin batch.
+   */
+  public function testUserAction() {
+    $this->drupalLogin($this->rootUser);
+
+    $themes = ['classy', 'seven', 'bartik'];
+
+    /** @var \Drupal\Core\Extension\ThemeInstallerInterface $theme_installer */
+    $theme_installer = $this->container->get('theme_installer');
+    $theme_installer->install($themes);
+
+    foreach ($themes as $theme) {
+      \Drupal::configFactory()->getEditable('system.theme')->set('default', $theme)->save();
+      $this->drupalGet('admin/people');
+      $edit = [
+        'user_bulk_form[0]' => TRUE,
+        'action' => 'user_batch_action_test_action',
+      ];
+      $this->drupalPostForm(NULL, $edit, t('Apply to selected items'));
+      $this->checkForMetaRefresh();
+      $this->assertSession()->pageTextContains('One item has been processed.');
+    }
+  }
+
+}

core/modules/views/tests/src/Kernel/ViewExecutableTest.php

@@ -487,6 +487,35 @@ class ViewExecutableTest extends ViewsKernelTestBase {
     $this->assertIdentical($unserialized->current_display, 'page_1', 'The expected display was set on the unserialized view.');
     $this->assertIdentical($unserialized->args, ['test'], 'The expected argument was set on the unserialized view.');
     $this->assertIdentical($unserialized->getCurrentPage(), 2, 'The expected current page was set on the unserialized view.');
+
+    // Get the definition of node's nid field, for example. Only get it not from
+    // the field manager directly, but from the item data definition. It should
+    // be the same base field definition object (the field and item definitions
+    // refer to each other).
+    // See https://bugs.php.net/bug.php?id=66052
+    $field_manager = \Drupal::service('entity_field.manager');
+    $nid_definition_before = $field_manager->getBaseFieldDefinitions('node')['nid']
+      ->getItemDefinition()
+      ->getFieldDefinition();
+
+    // Load and execute a view.
+    $view_entity = View::load('content');
+    $view_executable = $view_entity->getExecutable();
+    $view_executable->execute('page_1');
+
+    // Reset static cache, but leave possibility to use database cache.
+    $field_manager->useCaches(FALSE);
+    $field_manager->useCaches(TRUE);
+
+    // Magic line.
+    unserialize(serialize(['SOMETHING UNEXPECTED', $view_executable]));
+
+    // Make sure the serialisation of the ViewExecutable didn't influence the
+    // field definitions.
+    $nid_definition_after = $field_manager->getBaseFieldDefinitions('node')['nid']
+      ->getItemDefinition()
+      ->getFieldDefinition();
+    $this->assertEquals($nid_definition_before->getPropertyDefinitions(), $nid_definition_after->getPropertyDefinitions());
   }
 
 }