BatchStorage fails to serialize/deserialize input batch object with FormState

Attaching the patch to a comment so the test bot can run it. This is the same patch as uploaded in the body.

Uploading a patch for @Wangchun he is having issues because of his account:

"As a part of our continued efforts to make Drupal.org a productive place to visit and work, we’ve made it harder for spammers to post on the site. People who post valuable content will be confirmed and won’t see this message."

Testbot doesn't have an issue w/ it moving to needs review.

There are a few problems here:

1. To even start discussion here, we need a way to reliably reproduce the issue. (The issue summary does not adequately explain how to produce the issue.) Ideally you'd start with an automated test that will consistently reproduce the issue, which will both (1) help give a better understanding of the issue as well as (2) prove that a potential fix is actually working when it gets attached.
2. The provided patch is, at best, a workaround for one use case. If the problem really is at a lower level with serialization, there are likely multiple places that need to be adjusted to adequately cope with the issue, and we'd want to make a change that is generic instead of a hacky one-off for the FormState class.
3. And that kind of leads to the last issue, which is that if this is really a problem with the native PHP serialization, then the problem needs to be fixed in PHP, not in Drupal. (But it's still not clear what the actual issue is yet.)

Also, please read the issue tagging guidlines before adding tags to issues. Thanks!

1. Yes, a test would be great. The test we have that identified the issue is all based on custom code, one of the main drivers to putting the ticket on D.O was for the test bot in core to run the tests around batch and hopefully to spark some conversation that anyone else saw around serialization of the form_state. I will see if I can carve out some time to write a test based only on core code that reproduces the issue.
2. Yes, this is a super specific patch. Luckily the other workd-arounds for for PHP 73. I actually think this post shows something similarly wrong w/ batch -> https://www.drupal.org/project/drupal/issues/3011684
3. Its a known PHP change, they "fixed" a bug in 7.3 that made unserialize stricter, from the changelog (https://www.php.net/ChangeLog-7.php):

Fixed unserialize(), to disable creation of unsupported data structures through manually crafted strings.

there isn't an issue associated w/ that change so don't really know what spawned the change. If you read through some of the other issues here about serialize and unserialize you can see find a link to the conversation on the PHP email list where the PHP maintainers explains some of the issues w/ serialize unserialize:

I plan to propose and implement a new custom object serialization mechanism for PHP 7.4, to replace the Serializable interface and all the problems that come with it.
> For now, all I can suggest is to rewrite your code in a way that does not use parent::serialize(). I don’t think there is anything we can do to fix Serializable itself, unfortunately.

Can we do it more like:

$batch_serialized = $this->serialization($batch);

And use the value of $batch_serialized in the insert/update query.
The method testSerialization() should not do the inserting or updating.
Let the method serialization() do everything that testSerialization() without the insert/update.

Update patch to 8.8.x

Tagging against topical issue tag for PHP 7.3 as this issue impacts sites running PHP 7.3.0 or later. Updating to version to 8.8.x-dev

We may also want to link this solution to other PHP 7.3 Workarounds #3001920: Investigate PHP 7.3 workarounds

The first step should be to write a test that can reproduce the problem.

The solution patch provided so far reads as too specific and hacky. It might be better to follow the patterns used when solving this in #3049437: SharedTempStore not serializable on PHP 7.3 and #3011684: Properly serialize \Drupal\Core\TempStore\PrivateTempStore where they leverage the DependencySerializationTrait to serialize/unserialize

Find attached a new solution that is allows FormState to be serialized intact, so it can be used in batch Form submissions. This solution implements the Drupal\Core\DependencyInjection\DependencySerializationTrait

When returning the list of properties to serialize, we remove the build_info as this contains Symfony Routes and Requests and other services that shouldn't be serialized and unserialized again. By removing them for serialization it seems to have fixed the problem.

I've yet to find a simple way to write a test for this. Was looking at how FormState unit tests are set-up for clues.

@chOP

Thanks for patch #14. It seems to solve the issue I was having with runnint batch processes from a form on PHP 7.3.
However, previewing a node errors out with:
The form object has not yet been stored on the form state by the form builder

Realized that the error message I mentioned was from a patch on Issue 2897557

Here is a patch for this issue that fixed the preview error on my local. Please test it.

Thanks.

Updated patch to use array_diff instead of unset-ing. Aka going back to what @chOP had in their patch.

Thank you Chopper.
I've updated the patch for fixing error in preview content
Cheer

Thanks for working on this, as its a bug, we need a new test demonstrating the bug

1. +++ b/core/lib/Drupal/Core/Form/FormState.php
   @@ -449,6 +454,24 @@ class FormState implements FormStateInterface {
   +    if ($obj_vars['build_info']['args'] && is_object($obj_vars['build_info']['args'][0])) {
   

   shouldn't we be using array_filter with 'is_object' on $obj_vars['build_info']['args'] in case the second, third or subsequent argument is an object?

2. +++ b/core/lib/Drupal/Core/Form/FormState.php
   @@ -449,6 +454,24 @@ class FormState implements FormStateInterface {
   +      return $vars;
   

   no need for the else here as we return earlier

Hello,

this is the simplest form I could write to reproduce the problem. Maybe someone can write a test with that. Or this isn't correlated and is a bug in requestStack?

<?php

namespace Drupal\qd_mailing_entity\Form;


use Drupal\Core\Form\ConfigFormBase;
use Drupal\Core\Form\FormStateInterface;

class FailedMailingRequeueForm extends ConfigFormBase {

  public $oRequest;

  protected function getEditableConfigNames() {
    return [];
  }

  public function getFormId() {
    return 'failed_mailing_requeue_form';
  }

  public function buildForm(array $form, FormStateInterface $form_state) {
    $this->oRequest = \Drupal::requestStack()->getCurrentRequest();
    return parent::buildForm($form, $form_state);
  }

  public function submitForm(array &$form, FormStateInterface $form_state) {
    unserialize(serialize($form_state));
   // batch_set() would fail here, too
  }

}

This works when you use the form normally in the browser, but when you serialize it won't work.

To fix serialize for this form you could use either \Drupal::request() or set $oRequest to private. But I just wrote this form to help reproduce the problem. Hope someone more knowledgeable can understand the underlying problem :)

I discovered that injecting the logger.factory service into a class also causes batch to fail because of serialization. Digging a bit deeper, logger uses the RequestStack, which is at the root of the problem.

In looking at the serialized data in the batch table, the unserialize is failing because the request parameter _access_result value is r:xxx where the xxx reference is not defined (I don't fully understand the r syntax so I cannot give greater detail.

I hope this helps.

Confirm that removing logger.factory service fix the problem

Just to confirm that the patch from #20 works.

I have just installed a fresh latest version of drupal 9.3X andi am getting this message on opening and logging into the site. anyone know why this is still here in the latest version and is there a fix. thanks

Daniel

The last path (#20) not is working for me,

I have need add another condition for remove build_info field, so:

  /**
   * {@inheritdoc}
   */
  public function __sleep() {
    $obj_vars = get_object_vars($this);
    $vars = $this->defaultSleep();
    if (
      $obj_vars['build_info']['args'] && is_object($obj_vars['build_info']['args'][0])
      || ($obj_vars['build_info']['callback_object'])
    ) {
      $unserializable = [
        'build_info',
      ];
      return array_diff($vars, $unserializable);
    }
    else {
      return $vars;
    }

  }

In Drupal 9 all work ok about this issue.

RE: #26 also confirming that's the cause -- removing the logger factory service fixed my issue as well. I've been struggling with it for longer than I care to admit, thank you @refman1073.

With the logger factory in (and since PHP7.3 and up) the form_state on my Configurable Action (buildConfigurationForm) errors on serialization. Without it, no errors!

Last patch (#20) with changes from comment #32 worked for me on project with Drupal 9.
Also decision from #33 worked, looks like problem in logger factory.