Complex job in ViewExecutable::unserialize() causes data corruption

As already written in https://www.drupal.org/node/2750463#comment-12081985, I suspect that the "trigger" of the problem is calling the serialize() function inside the ViewExecutable::serialize() method.

Attaching a patch which replaces the Serializable implementation by an alternative using magic methods.
By this, I also removed the usage of DependencySerializationTrait, because it seems that it had no use anyway (__sleep() and __wakeup() aren't used anymore when a class implements the \Serializable interface). I haven't found any other point where this might be relevant, thus removed.

Code looks straightforward to me.

By this, I also removed the usage of DependencySerializationTrait, because it seems that it had no use anyway (__sleep() and __wakeup() aren't used anymore when a class implements the \Serializable interface). I haven't found any other point where this might be relevant, thus removed.

OK storage ID is serialized, but what about the other injected services (routeProvider) and data (user, viewsData)? My gut feeling is, add viewsData and the IDs of user and routeProvider.

  public function __construct(ViewEntityInterface $storage, AccountInterface $user, ViewsData $views_data, RouteProviderInterface $route_provider) {
    // Reference the storage and the executable to each other.
    $this->storage = $storage;
    $this->storage->set('executable', $this);
    $this->user = $user;
    $this->viewsData = $views_data;
    $this->routeProvider = $route_provider;
  }

OK storage ID is serialized, but what about the other injected services (routeProvider) and data (user, viewsData)? My gut feeling is, add viewsData and the IDs of user and routeProvider.

Yes, while __wakeup() already loads the user data, it's still missing the views.views_data and router.route_provider service. Seems that this was already broken before. Writing new patch...

Now attaching all necessary services on the executable, plus added circular referencing from storage to executable just like in the constructor method.

Needs some manual serialization tests for proper reviewing.

+++ b/core/modules/views/src/ViewExecutable.php
@@ -2466,52 +2475,64 @@ public function getDependencies() {
       $this->user = \Drupal::currentUser();
+      $this->viewsData = \Drupal::service('views.views_data');
+      $this->routeProvider = \Drupal::service('router.route_provider');
 

Can we thoroughly assume these objects can not change? Use cases for this might sound esoteric but what can be thought will be done... Storing the IDs imho does not cost us much, are are there disadvantages?

Agree, except for the current user object. Since the executable makes some access checks on it, it should be ensured that the object actually represents the current user.

Agree, except for the current user object. Since the executable makes some access checks on it, it should be ensured that the object actually represents the current user.

Imho to the contrary. There are legitimate use cases where the view user is *not* the current user. I remember working on a D7 commerce issue where the payment gateway made a callback, bootstrapped as anonymous user, and had to execute a view for the transaction's user. That's an ideal case for executing a serialized view for another user with their access context. What do you think?

EDIT: And another viewpoint: If we don't respect the original user, the user property of the view does not make any sense and it should simply use the global current user.

Disagree with the following arguments:

• The whole executable object expects the user property to be the current user. Other objects which use the executable object would adopt this expectation. Exceptional use cases should handle the change of this behavior on their own.
• Assuming that the user (id) property would be serialized and unserialized: A user who is able to use unserialized view executables, which have been serialized by other users, might then bypass access restrictions.

EDIT: And another viewpoint: If we don't respect the original user, the user property of the view does not make any sense and it should simply use the global current user.

It does make sense, since it's there to let other objects being able to use the object, without the need of dependency injection or global container usage.

Hmm...

> The whole executable object expects the user property to be the current user.

Is there any evidence for that? I see evidence for the opposite, besides the user property, read comments in e.g. node_query_node_access_alter().

> A user who is able to use unserialized view executables, ... might then bypass access restrictions.

To unserialize arbitrary objects you already need execute-php, which is far stronger. So i see no privilege escalation potential here.

But hey no problem, we can give the component maintainer both patches to decide. It's anyway up to them.

> The whole executable object expects the user property to be the current user.

Is there any evidence for that? I see evidence for the opposite, besides the user property, read comments in e.g. node_query_node_access_alter().

Documentation of the ViewExecutable property user, ::__construct() as well as ::getUser():

<?php
/**
   * The current user.
   *
   * @var \Drupal\Core\Session\AccountInterface
   */
  protected $user;

// ...

  /**
   * Constructs a new ViewExecutable object.
   *
   * @param \Drupal\views\ViewEntityInterface $storage
   *   The view config entity the actual information is stored on.
   * @param \Drupal\Core\Session\AccountInterface $user
   *   The current user.
   * @param \Drupal\views\ViewsData $views_data
   *   The views data.
   * @param \Drupal\Core\Routing\RouteProviderInterface $route_provider
   *   The route provider.
   */
  public function __construct(ViewEntityInterface $storage, AccountInterface $user, ViewsData $views_data, RouteProviderInterface $route_provider) {

// ...

/**
   * Gets the current user.
   *
   * Views plugins can receive the current user in order to not need dependency
   * injection.
   *
   * @return \Drupal\Core\Session\AccountInterface
   *   The current user.
   */
  public function getUser() {
    return $this->user;
  }
?>

Besides there's also no setUser() method which allows to change the user object, this should indicate that the executable view always expects the current(ly logged in) user.

> A user who is able to use unserialized view executables, ... might then bypass access restrictions.

To unserialize arbitrary objects you already need execute-php, which is far stronger. So i see no privilege escalation potential here.

Sorry, I might have not explained it correctly. I meant the permission to use / access executables which have been serialized before. The permission to access unserialized data doesn't require a permission to unserialize.

But hey no problem, we can give the component maintainer both patches to decide. It's anyway up to them.

Yes, and since there might be something different which needs to be done before it's RTBC, I'd wait for a response from them. But feel free to provide an alternative patch which contains the solution you'd prefer.

I see evidence for the opposite, besides the user property, read comments in e.g. node_query_node_access_alter().

I've read the comments in node_query_node_access_alter(), but I'm not sure which comment is related to this issue and would indicate a different behavior for the ViewExecutable. Could you please point to a concrete example / comment?

Regarding node_query_node_access_alter(), i meant that it is prepared for non-current-user queries:

  // Read meta-data from query, if provided.
  if (!$account = $query->getMetaData('account')) {
    $account = \Drupal::currentUser();
  }

But in fact i did not check that all ViewExecutable is prepared for that. Well, if not we can add a feature issue for that.

But important for this is the objection you stated about permissions:

Sorry, I might have not explained it correctly. I meant the permission to use / access executables which have been serialized before.

Is there a user permission to "use any serialized view" in any *.permissions.yml? I did not get that if.

Is there a user permission to "use any serialized view" in any *.permissions.yml? I did not get that if.

I brought in the term "permission" but that wasn't intended to focus the discussion on permission definitions. Although there doesn't exist a concrete permission in core about using any serialized view, there might be use cases for it.

The problem I see here is the general possibility that a user might get valid access to a view, which might have been serialized before by another user (e.g. maybe guest or admin). The ViewExecutable class provides the current user object, and this object must not differ from \Drupal::currentUser() IMO.

I'd like to have at least the error itself getting fixed in core, thus changing status to try gaining some attention of the maintainers.

Please review the patch and give feedback whether it solves (at least partially) the serialization problem for you.
What the serialization process should include like it has been discusses above, may be of course discussed further, although it's not really part of this issue.

#9 fixed issue with site stability for 8.3.5 core. It applied without the need to reroll.

We had an issue when cold cache page was loading, but cached page was throwing error 500

I have manually queued #9 for re-testing for 8.4.x, the test now passes (again).

@podarok thanks for your feedback. I'd like to have additional feedback from other people as well though, since this issue seems to be a complicated one and addresses multiple symptoms of this problem. Thus I'm switching to needs review (again :).

@andypost I think #2750463: Fatal error: Call to a member function getFieldStorageDefinition() on array is strongly related to this issue. I originally created the patch for this issue, thus the source of the problem would be the Views module there as well. Feel free to also change to proper status on this one. I still might be wrong of course.

@mxh This looks proper fix, also I attached related issues that affected by the change

+++ b/core/modules/views/src/ViewExecutable.php
@@ -18,8 +17,7 @@
-class ViewExecutable implements \Serializable {
-  use DependencySerializationTrait;
+class ViewExecutable {

Why Serializable removed?
I think it needs inline comment explaining why we can't use DST here to prevent future errors

+++ b/core/modules/views/src/ViewExecutable.php
@@ -2466,52 +2475,66 @@ public function getDependencies() {
+      // Restore the state of this executable.
+      $this->setRequest(\Drupal::request());
+      $this->setDisplay($this->serializationData['current_display']);

There are cases when Drupal::request() returns NULL
Which causes the

fatal error: Argument 1 passed to Drupal\views\ViewExecutable::setRequest() must be an instance of Symfony\Component\HttpFoundation\Request, null given

@andypost Is following comment in from patch #9 wrong (or maybe misplaced)? :

+   * Holds all necessary data for proper unserialization.
+   *
+   * This class does not implement the Serializable interface,
+   * since there have been problems occurred when using the serialize method.
+   * @see https://www.drupal.org/node/2849674
+   *
+   * @var array
+   */
+  protected $serializationData;

There are cases when Drupal::request() returns NULL
Which causes the

fatal error: Argument 1 passed to Drupal\views\ViewExecutable::setRequest() must be an instance of Symfony\Component\HttpFoundation\Request, null given

Can you tell a concrete scenario? Drush / testing maybe? What should happen in case the request is missing? Not sure if the view is able to work without the request object, need to check.

      $request = \Drupal::request();
      if ($request != NULL) {
        $this->setRequest($request);
      }

I've changed the line to get rid of https://www.drupal.org/node/2849674#comment-12164812

@mxh
See docroot/vendor/symfony/http-foundation/RequestStack.php:61

@mxh
Attaching workaround applied over #9
It is helping me to fix issue with request null so far

Nope
After hard testing of https://www.drupal.org/node/2849674#comment-12164887 found

PHP Fatal error: Call to undefined function Drupal\views\Plugin\views\query\db_and() in docroot/core/modules/views/src/Plugin/views/query/Sql.php

So the issue not only with request here

How do you test / which unit test is affected? I'd like to reproduce.

btw Sometimes it happens, when there's no request see #2650434: Clearing cache via UI in translated language resets config translation of field labels to default language

But I'm not sure that ViewExecutable require request to make some job

@podarok can you dump backtrace to get when it happens, and why there's no request?

@andypost

#1 /mnt/www/html/sitedir/docroot/core/modules/views/src/ViewExecutable.php(1799): _drupal_error_handler(4096, 'Argument 1 pass...', '/mnt/www/html/y...', 1799, Array)
#2 /mnt/www/html/sitedir/docroot/core/modules/views/src/ViewExecutable.php(2520): Drupal\views\ViewExecutable->setRequest(NULL)
#3 [internal function]: Drupal\views\ViewExecutable->__wakeup()
#4 /mnt/www/html/sitedir/docroot/modules/contrib/memcache/src/DrupalMemcache.php(89): MemcachePool->get(Array)
#5 /mnt/www/html/sitedir/docroot/modules/contrib/memcache/src/MemcacheBackend.php(101): Drupal\memcache\DrupalMemcache->getMulti(Array)
#6 /mnt/www/html/sitedir/docroot/modules/contrib/memcache/src/MemcacheBackend.php(89): Drupal\memcache\MemcacheBackend->getMultiple(Array, false)
#7 /mnt/www/html/sitedir/docroot/core/modules/page_cache/src/StackMiddleware/PageCache.php(317): Drupal\memcache\MemcacheBackend->get('http://sitedir...', false)
#8 /mnt/www/html/sitedir/docroot/core/modules/page_cache/src/StackMiddleware/PageCache.php(117): Drupal\page_cache\StackMiddleware\PageCache->get(Object(Symfony\Component\HttpFoundation\Request))
#9 /mnt/www/html/sitedir/docroot/core/modules/page_cache/src/StackMiddleware/PageCache.php(75): Drupal\page_cache\StackMiddleware\PageCache->lookup(Object(Symfony\Component\HttpFoundation\Request), 1, true)
#10 /mnt/www/html/sitedir/docroot/core/modules/ban/src/BanMiddleware.php(50): Drupal\page_cache\StackMiddleware\PageCache->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true)
#11 /mnt/www/html/sitedir/docroot/modules/contrib/shield/src/ShieldMiddleware.php(57): Drupal\ban\BanMiddleware->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true)
#12 /mnt/www/html/sitedir/docroot/core/lib/Drupal/Core/StackMiddleware/ReverseProxyMiddleware.php(47): Drupal\shield\ShieldMiddleware->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true)
#13 /mnt/www/html/sitedir/docroot/core/lib/Drupal/Core/StackMiddleware/NegotiationMiddleware.php(50): Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true)
#14 /mnt/www/html/sitedir/docroot/vendor/stack/builder/src/Stack/StackedHttpKernel.php(23): Drupal\Core\StackMiddleware\NegotiationMiddleware->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true)
#15 /mnt/www/html/sitedir/docroot/core/lib/Drupal/Core/DrupalKernel.php(656): Stack\StackedHttpKernel->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true)
#16 /mnt/www/html/sitedir/docroot/index.php(19): Drupal\Core\DrupalKernel->handle(Object(Symfony\Component\HttpFoundation\Request))
#17 {main} request_id="v-76114d28-66ea-11e7-a8d7-0e2839910d74"
[12-Jul-2017 10:11:40 UTC] Recoverable fatal error: Argument 1 passed to Drupal\views\ViewExecutable::setRequest() must be an instance of Symfony\Component\HttpFoundation\Request, null given, called in /mnt/www/html/sitedir/docroot/core/modules/views/src/ViewExecutable.php on line 2520 and defined in /mnt/www/html/sitedir/docroot/core/modules/views/src/ViewExecutable.php on line 1799 #0 /mnt/www/html/sitedir/docroot/core/includes/bootstrap.inc(566): _drupal_error_handler_real(4096, 'Argument 1 pass...', '/mnt/www/html/y...', 1799, Array)
#1 /mnt/www/html/sitedir/docroot/core/modules/views/src/ViewExecutable.php(1799): _drupal_error_handler(4096, 'Argument 1 pass...', '/mnt/www/html/y...', 1799, Array)
#2 /mnt/www/html/sitedir/docroot/core/modules/views/src/ViewExecutable.php(2520): Drupal\views\ViewExecutable->setRequest(NULL)
#3 [internal function]: Drupal\views\ViewExecutable->__wakeup()
#4 /mnt/www/html/sitedir/docroot/modules/contrib/memcache/src/DrupalMemcache.php(89): MemcachePool->get(Array)
#5 /mnt/www/html/sitedir/docroot/modules/contrib/memcache/src/MemcacheBackend.php(101): Drupal\memcache\DrupalMemcache->getMulti(Array)
#6 /mnt/www/html/sitedir/docroot/modules/contrib/memcache/src/MemcacheBackend.php(89): Drupal\memcache\MemcacheBackend->getMultiple(Array, false)
#7 /mnt/www/html/sitedir/docroot/core/modules/page_cache/src/StackMiddleware/PageCache.php(317): Drupal\memcache\MemcacheBackend->get('http://sitedir...', false)
#8 /mnt/www/html/sitedir/docroot/core/modules/page_cache/src/StackMiddleware/PageCache.php(117): Drupal\page_cache\StackMiddleware\PageCache->get(Object(Symfony\Component\HttpFoundation\Request))
#9 /mnt/www/html/sitedir/docroot/core/modules/page_cache/src/StackMiddleware/PageCache.php(75): Drupal\page_cache\StackMiddleware\PageCache->lookup(Object(Symfony\Component\HttpFoundation\Request), 1, true)
#10 /mnt/www/html/sitedir/docroot/core/modules/ban/src/BanMiddleware.php(50): Drupal\page_cache\StackMiddleware\PageCache->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true)
#11 /mnt/www/html/sitedir/docroot/modules/contrib/shield/src/ShieldMiddleware.php(57): Drupal\ban\BanMiddleware->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true)
#12 /mnt/www/html/sitedir/docroot/core/lib/Drupal/Core/StackMiddleware/ReverseProxyMiddleware.php(47): Drupal\shield\ShieldMiddleware->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true)
#13 /mnt/www/html/sitedir/docroot/core/lib/Drupal/Core/StackMiddleware/NegotiationMiddleware.php(50): Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true)
#14 /mnt/www/html/sitedir/docroot/vendor/stack/builder/src/Stack/StackedHttpKernel.php(23): Drupal\Core\StackMiddleware\NegotiationMiddleware->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true)
#15 /mnt/www/html/sitedir/docroot/core/lib/Drupal/Core/DrupalKernel.php(656): Stack\StackedHttpKernel->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true)
#16 /mnt/www/html/sitedir/docroot/index.php(19): Drupal\Core\DrupalKernel->handle(Object(Symfony\Component\HttpFoundation\Request))
#17 {main} request_id="v-7f4f9e12-66ea-11e7-a0ad-0e2839910d74"

Without memcache error is another
https://www.drupal.org/node/2849674#comment-12164892

So indeed request lost in page cache deserialization

#7 /mnt/www/html/sitedir/docroot/core/modules/page_cache/src/StackMiddleware/PageCache.php(317): Drupal\memcache\MemcacheBackend->get('http://sitedir...', false)
#8 /mnt/www/html/sitedir/docroot/core/modules/page_cache/src/StackMiddleware/PageCache.php(117): Drupal\page_cache\StackMiddleware\PageCache->get(Object(Symfony\Component\HttpFoundation\Request))

#32 tells about missing db_and() - that means again kernel is not bootstraped properly (includes/database.inc is not loaded yet)

So all this points to wrong config for site

Wrapped \Drupal::request() call with an if (addresses #26), placed the explanation why \Serializable is not being used to the class description (addresses #25).

@mxh not enough, in case if request is empty would be nice to create it from Globals as in https://www.drupal.org/node/2849674#comment-12164887

@mxh not enough, in case if request is empty would be nice to create it from Globals as in https://www.drupal.org/node/2849674#comment-12164887

In which case would this happen? I'd assume that, since index.php / update.php etc. already creates from globals: When \Drupal::request() isn't returning any request, there won't be any, except some other component of the system is either misconfigured or behaving wrong by changing RequestStack::requests.

Whatever, this issue isn't actually addressing how the ViewExecutable should behave on deserialization. It's about solving the deserialization bug itself, which patch from #38 apparently does. Feel free to create and discuss another issue regarding how ViewExecutable objects should behave during (de-)serialization.

> In which case would this happen?
I'd be a GOD when catched this issue, but no luck so far

Atm to get this issue I have to apachebench the page with a view embedded via reference to get cache broken and to get errors from my above messages
Also memcached version way less stable in comparison to DB one...

Feel free to vote wich one you'd prefer (atm I'd still go for #38).

Just a side note: The exception added in #30 (680.diff) would never be thrown, because Request::createFromGlobals would always return an object.

This is the patch in #38 merged with @andyposts test from #2701829: Extension objects should not implement \Serializable, both updated/cleaned up a bit.

Since this issue was split off from #2701829: Extension objects should not implement \Serializable, it feel a bit weird to close that as a duplicate, but it does feel like both are working on the same issue at the moment. But since there is a bit more activity here, lets start here.

Yay! Let's get this in and re-purpose #2701829: Extension objects should not implement \Serializable to it's original intent to get rid of legacy code

Known random fail, #2828143: Stop tests like LocaleConfigTranslationImportTest from failing if l.d.o becomes unavailable

Could we add an @see to https://bugs.php.net/bug.php?id=66052 as well as the link to here?

Also I wonder whether we could add a kernel or unit test similar to the test case in the issue summary? Relying on batch triggering this seems a bit indirect in the current test, although probably fine to have both.

@catch yes that sounds like much better explicit coverage for this.

Here we go, the test case from the IS converted to a kernel test. Plus a @see to the php bug

#48 addressed so back to RTBC

Committed/pushed to 8.5.x and cherry-picked to 8.4.x. Thanks!

I've reverted this for now, due to #2898721: Random segfault currently in FileFieldWidgetTest::testMultiValuedWidget(). This is based on probability, not sure if this issue was the commit that did it, queueing some test runs to see if we can flush it out here.

And here they are:
8.4.x https://www.drupal.org/pift-ci-job/729321
8.5.x https://www.drupal.org/pift-ci-job/729861
otherwise testbots are stable after the revert.

Don't mess with \Drupal in a test ......

Has to be something like this right? This has no other overlap with FileFieldWidgetTest::testMultiValuedWidget()

Did some testing and it turns out it isn't even the 'patch' part that is causing this, the added test coverage is enough to trigger it (see: pift-ci-job/731299)

@tacituseu yeah I figured the same thing, so lets strip out some more stuff that might carry over into another test.....also turns out isn't needed to make the test green.

So adding user_batch_action_test_action is enough to trigger it. (see pift-ci-job/731329)

Running out of ideas, but noticed couple of nits:
1.

+ * @Action(
+ *   id = "user_batch_action_test_action",
+ *   label = @Translation("Process user in batch"),
+ *   type = "user",
+ * )

there should be no comma after "user"
2.

+  public function access($object, AccountInterface $account = NULL, $return_as_object = FALSE) {
+    return TRUE;
+  }

can't return bool if called with $return_as_object set to TRUE

68.1

There can be a trailing commas in annotations now

Note that we changed the patch testing default branch to PHP 7 (see #2607222: [policy, no patch] Default to PHP 7 for Drupal core patch testing) so patches to untangle that segfault will need to be manually queued against the 5.6 environment.

From @vaplas in #2898721: Random segfault currently in FileFieldWidgetTest::testMultiValuedWidget():

with php 5.6 it works very badly (https://www.drupal.org/pift-ci-job/730043)
with php 7.1 it while works perfect (https://dispatcher.drupalci.org/job/drupal_patches/25019/console, https://www.drupal.org/pift-ci-job/730031)

Maybe commit this with kernel test only, and leave figuring out why the Action test segfaults to a follow-up.
@Lendude found in the testing issue that it could be trimmed even further, and the sheer fact of registering the test module is enough to trigger it (pift-ci-job/732110).

@xjm: quote in #71 is from @vaplas.

For those interested, we are running a ton of tests in #2879048: Ignore: patch testing issue for #2919863

Note worthy stuff:

- Going overboard on garbage collection makes the problem go away https://www.drupal.org/node/2879048#comment-12202416
- Just adding an empty module will break that test, but changing the info file slightly makes it pass https://www.drupal.org/node/2879048#comment-12202738

So the problem doesn't really seem to be with this patch, it seems to be more FileFieldWidgetTest being really unstable.

I'd be for #72. Remove the batch test for now and only add the kerneltest, that gives good coverage for this change and should be stable.

Re: #73: Oops, fixed, thanks @tacituseu.

I agree with #72; let's split the action test coverage off into a separate followup issue where we can investigate and work around the segfault, rather than holding up a bugfix on a PHP 5.6 bug that (as far as I understand) won't surface under normal site operation.

Here we go, the fix with just the kernel test, slightly cleaned up.

test only patch

Something wrong with list of patches

Looks like the static cache clear is needed for kernel test to work, changed in #64.

@andypost++, @tacituseu++ nice catch, completely failed to check that the test-only was still red!

Here we go, this should be better.

Looks great! Just not clear why this kind of cache clear used

+++ b/core/modules/views/tests/src/Kernel/ViewExecutableTest.php
@@ -487,6 +487,35 @@ public function testSerialization() {
+    // Reset the static cache.
+    $field_manager->useCaches(FALSE);
+    $field_manager->useCaches(TRUE);

Why not use clearCachedFieldDefinitions()?

I'd add a comment documenting that, so someone else doesn't try to refactor it.

Another approach to do that would be to do a set(..., NULL) on the container for that service, then the whole service is re-instantiated. This only works well if the service isn't access indirectly through another that is still instantiated, then you'd have to unset the whole dependency chain.

Agreed about adding a comment either way, so setting to needs work for that.

Actually doing that now ;)

We also still need a followup for that Action test. It should be a major (since it triggers a segfault) and we should mention it on the random fail meta.

Created follow up for adding the full batch bulk update test coverage #2900399: Adding test coverage for batch bulk updates, the fix for the cause of the seg fault is being explored in #2842393: Discover why gc_disable() improves update.php stability

Extended a comment to mention clearCachedFieldDefinitions , not sure about the wording, let me know if it needs some polishing.

Splitting the test off is good, although I have a feeling the eventual resolution to these gc/segfault issues is going to be 'require PHP 7' when that happens.

Committed/pushed to 8.5.x and cherry-picked to 8.4.x. Thanks!