Please consider all contributors to #2701829: Get rid of '#theme' => '' in \Drupal\views\Form\ViewsExposedForm::buildForm for credit.

Actually there also may be other classes implementing Serializable interface. The \Drupal\views\ViewExecutable is just the first known one to cause the trouble.

Problem/Motivation

There is a PHP bug in the unserialize() function - https://bugs.php.net/bug.php?id=66052 . Precisely in nested unserialization.

When one serialized string contains object which implements Serializable interface, and this object calls another unserialize() with string containing references (serialized as r:NUMBER;), these references are substituted by values from the first string upon unserialization, not from the current one. This behavior leads to corrupted data in runtime and to fatal errors in some cases, and that data may accidentally be saved to persistent storage if not validated explicitly.

The ViewExecutable::execute() method is known to call unserialize() during its job. Examples of Fatals caused by this nested unserialization: #2750463: Fatal error: Call to a member function getFieldStorageDefinition() on array, #2701829: Get rid of '#theme' => '' in \Drupal\views\Form\ViewsExposedForm::buildForm. There may be more. (Other code from ViewExecutable::unserialize() was not tested by me.)

A simple test case, which can be run in /devel/php form on any D8 dev site using the content view (/admin/content):

$field_manager = \Drupal::service('entity_field.manager');

// 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).
$nid_definition = $field_manager->getBaseFieldDefinitions('node')['nid']
  ->getItemDefinition()
  ->getFieldDefinition();
dpm($nid_definition, 'Expected:');

// Load and execute the view.
$view_entity = \Drupal\views\Entity\View::load('content');
$view = \Drupal::service('views.executable')->get($view_entity);
$view->execute('page_1');

// Reset static cache...
$field_manager->useCaches(FALSE);
// ... but leave possibility to use database cache.
$field_manager->useCaches(TRUE);

// Magic line.
unserialize(serialize(['SOMETHING UNEXPECTED', $view]));

// The same code as in the beginning of the test.
$nid_definition = $field_manager->getBaseFieldDefinitions('node')['nid']
  ->getItemDefinition()
  ->getFieldDefinition();
dpm($nid_definition, 'After nested unserialization:');

Proposed resolution

As the bug is still not fixed in PHP, we need to provide conditions to avoid its appearance in Drupal.

I propose to implement some kind of lazy execution in ViewExecutable, if that possible. Ideally, unserialize() method should simply assign values to object properties.

For impatient I'm attaching a patch with a very rough workaround for a limited use case - it covers only ViewExecutable class and the Database cache backend. But it may slow down your project, so don't use it on production.

Remaining tasks

  • Examine other classes with methods unserialize() and __wakeup().
CommentFileSizeAuthor
#88 2849674-88.patch7.44 KBLendude
#88 interdiff-2849674-80-88.txt663 bytesLendude
#80 2849674-80.patch7.3 KBLendude
#80 2849674-80-TEST_ONLY.patch2.03 KBLendude
#80 interdiff-2849674-76-80.txt626 bytesLendude
#76 2849674-76.patch7.19 KBLendude
#77 2849674-77-test-only.patch1.92 KBandypost
#76 interdiff-2849674-64-76.txt5.09 KBLendude
nested-unserialization-workaround.patch1.86 KBpingwin4eg
#2 2849674-view-executable-might-break-serialization.patch4.59 KBmxh
#6 2849674-view-executable-might-break-serialization-5.patch5.03 KBmxh
#9 2849674-view-executable-might-break-serialization-9.patch5.18 KBmxh
#29 can_ne_null.png10.24 KBpodarok
#30 680.diff848 bytespodarok
#38 2849674-view-executable-might-break-serialization-38.patch5.22 KBmxh
#43 2849674-view-executable-might-break-serialization-43.patch5.35 KBmxh
#44 interdiff-2849674-38-44.txt6.83 KBLendude
#44 2849674-44.patch10.89 KBLendude
#49 2849674-49-KERNEL_TEST_ONLY.patch2.03 KBLendude
#49 2849674-49.patch12.97 KBLendude
#49 interdiff-2849674-44-49.txt2.26 KBLendude
#61 interdiff-2849674-49-61.txt719 bytesLendude
#61 2849674-61.patch12.97 KBLendude
#63 2849674-63-only-test.patch8.15 KBtacituseu
#64 interdiff-2849674-61-64.txt627 bytesLendude
#64 2849674-64.patch12.83 KBLendude
#66 2849674-66-test-batch-only-config-and-plugins.patch4.64 KBtacituseu
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

pingwin4eg created an issue. See original summary.

mxh’s picture

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.

mxh’s picture

Status: Active » Needs review
axel.rutz’s picture

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;
  }
mxh’s picture

Status: Needs review » Needs work

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...

mxh’s picture

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.

axel.rutz’s picture

+++ 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?

axel.rutz’s picture

Status: Needs review » Needs work
mxh’s picture

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.

Manuel Garcia’s picture

Priority: Normal » Major
axel.rutz’s picture

Status: Needs review » Needs work

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.

mxh’s picture

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.

axel.rutz’s picture

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.

mxh’s picture

> 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.

mxh’s picture

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?

axel.rutz’s picture

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.

mxh’s picture

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.

mxh’s picture

Status: Needs work » Needs review

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.

podarok’s picture

Version: 8.4.x-dev » 8.3.x-dev
Status: Needs review » Reviewed & tested by the community
Issue tags: +stability

#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

Status: Reviewed & tested by the community » Needs work
mxh’s picture

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 :).

mxh’s picture

Status: Needs work » Needs review
andypost’s picture

Version: 8.3.x-dev » 8.4.x-dev
Component: base system » views.module
mxh’s picture

@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.

andypost’s picture

@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

podarok’s picture

Status: Needs review » Needs work
+++ 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

mxh’s picture

@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.

podarok’s picture

      $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

podarok’s picture

FileSize
10.24 KB

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

podarok’s picture

Status: Needs work » Needs review
FileSize
848 bytes

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

Status: Needs review » Needs work

The last submitted patch, 30: 680.diff, failed testing. View results

podarok’s picture

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

mxh’s picture

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

andypost’s picture

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

andypost’s picture

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

podarok’s picture

@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

andypost’s picture

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

mxh’s picture

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’s picture

Status: Needs work » Needs review
podarok’s picture

Status: Needs review » Needs work

@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’s picture

Status: Needs work » Needs review

@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.

podarok’s picture

> 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...

mxh’s picture

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.

Lendude’s picture

This is the patch in #38 merged with @andyposts test from #2701829: Get rid of '#theme' => '' in \Drupal\views\Form\ViewsExposedForm::buildForm, both updated/cleaned up a bit.

Since this issue was split off from #2701829: Get rid of '#theme' => '' in \Drupal\views\Form\ViewsExposedForm::buildForm, 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.

andypost’s picture

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 44: 2849674-44.patch, failed testing. View results

Lendude’s picture

Status: Needs work » Reviewed & tested by the community
catch’s picture

Status: Reviewed & tested by the community » Needs review

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.

The last submitted patch, 49: 2849674-49-KERNEL_TEST_ONLY.patch, failed testing. View results

andypost’s picture

Status: Needs review » Reviewed & tested by the community

#48 addressed so back to RTBC

  • catch committed 43c568a on 8.5.x
    Issue #2849674 by mxh, Lendude, podarok, pingwin4eg, andypost, axel.rutz...

  • catch committed 6cec75b on 8.4.x
    Issue #2849674 by mxh, Lendude, podarok, pingwin4eg, andypost, axel.rutz...
catch’s picture

Status: Reviewed & tested by the community » Fixed

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

  • catch committed bb23287 on 8.4.x
    Revert "Issue #2849674 by mxh, Lendude, podarok, pingwin4eg, andypost,...

  • catch committed cd0a65b on 8.5.x
    Revert "Issue #2849674 by mxh, Lendude, podarok, pingwin4eg, andypost,...
catch’s picture

Status: Fixed » Needs review

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.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Status: Needs review » Needs work

The last submitted patch, 49: 2849674-49.patch, failed testing. View results

tacituseu’s picture

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.

Lendude’s picture

Version: 8.5.x-dev » 8.4.x-dev
Status: Needs work » Needs review
FileSize
719 bytes
12.97 KB

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

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

Status: Needs review » Needs work

The last submitted patch, 61: 2849674-61.patch, failed testing. View results

tacituseu’s picture

Status: Needs work » Needs review
FileSize
8.15 KB

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)

Lendude’s picture

@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.

The last submitted patch, 63: 2849674-63-only-test.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

tacituseu’s picture

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

Status: Needs review » Needs work

The last submitted patch, 66: 2849674-66-test-batch-only-config-and-plugins.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

tacituseu’s picture

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

tim.plunkett’s picture

68.1

There can be a trailing commas in annotations now

xjm’s picture

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.

xjm’s picture

tacituseu’s picture

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).

tacituseu’s picture

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

Lendude’s picture

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

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.

xjm’s picture

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.

Lendude’s picture

andypost’s picture

andypost’s picture

Something wrong with list of patches

tacituseu’s picture

Status: Needs review » Needs work

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

Lendude’s picture

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

Here we go, this should be better.

The last submitted patch, 80: 2849674-80-TEST_ONLY.patch, failed testing. View results

andypost’s picture

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()?

pingwin4eg’s picture

@andypost because the clearCachedFieldDefinitions() clears persistent cache (e.g. from DB). And in the test we need to get that serialized cache data.

tim.plunkett’s picture

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

Berdir’s picture

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.

Berdir’s picture

Status: Needs review » Needs work

Actually doing that now ;)

xjm’s picture

Issue tags: +Needs followup

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.

Lendude’s picture

Created follow up for adding the full batch bulk update test coverage #2900399: Adding test coverage for batch bulk updates triggers a seg fault on the testbot, 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.

vaplas’s picture

Status: Needs review » Reviewed & tested by the community

Unstable part is moved to the separate issue. So back to RTBC.