Problem/Motivation

Some issues with D8 theme system do not allow to call batch operations (batch_set) from bulk actions. Error happens when empty array is passed as a theme to twig_init() from \Drupal\Core\Theme\ThemeInitialization::loadActiveTheme.

D8 codebase inherits hack that makes no sense

  public function buildForm(array $form, FormStateInterface $form_state) {
    // Don't show the form when batch operations are in progress.
    if ($batch = batch_get() && isset($batch['current_set'])) {
      return array(
        // Set the theme callback to be nothing to avoid errors in template_preprocess_views_exposed_form().
        '#theme' => '',
      );
    }

But template_preprocess_views_exposed_form has nothing about theme name for form
This disallows to use custom bulk actions that needs to implement own batching without confirm form

Reproduced by using any VBO action plugin that sets batch (no such one in core)

Proposed resolution

The hack is causing a recursive serialization issue with the active theme cache. The problem is that currently the active theme contains references to its extension object and also other active theme objects representing the base themes - which also contain references to their extension objects. The results in a recursive store of objects that in certain situations appears to get corrupted.

This issue is compounded in Drupal 8.7.x where the addition of Dependency objects to the module list causes a similar situation there.

The solution is that Extension objects should not implement \Serializable

Also the following from @pingwin4eg in #2849674: Complex job in ViewExecutable::unserialize() causes data corruption is very relevant -

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.

Remaining tasks

User interface changes

None

API changes

Extension object do not implement \Serializable but use the magic __sleep() and __wakeUp() methods to provide the same functionality.

Data model changes

None

Release notes snippet

I don't think the change here is high enough level for the release notes because I don't think any site is going to have to change because of it or even custom code.

CommentFileSizeAuthor
#112 2701829-8.6.x-112.patch10.08 KBalexpott
#112 2701829-8.6.x-112.test-only.patch7.45 KBalexpott
#101 2701829-3-101.patch11.54 KBalexpott
#101 97-101-interdiff.txt866 bytesalexpott
#97 2701829-3-97.patch11.18 KBalexpott
#97 2701829-3-97.test-only.patch8.58 KBalexpott
#91 2701829-2-91.patch24.36 KBalexpott
#91 88-91-interdiff.txt2.6 KBalexpott
#88 2701829-2-88.patch24.37 KBalexpott
#88 85-88-interdiff.txt7.08 KBalexpott
#85 2701829-2-85.patch20.59 KBalexpott
#85 83-85-interdiff.txt9.53 KBalexpott
#83 2701829-2-83.patch10.09 KBalexpott
#83 80-83-interdiff.txt1.12 KBalexpott
#80 2701829-2-80.patch9.36 KBalexpott
#80 78-80-interdiff.txt4.78 KBalexpott
#78 2701829-2-78.patch5.54 KBalexpott
#74 2701829-74.patch1 KBalexpott
#66 2701829-66.patch781 bytesEduardo Morales Alberti
#60 drupal-2701829-60.patch6.47 KBGraber
#45 2701829-45-clean.patch776 bytesandypost
#45 2701829-45.patch783 bytesandypost
#18 2701829-18-testonly.patch5.67 KBandypost
#17 user_batch_action_test.tar_.gz1.85 KBandypost
#10 Example-for-batch-call-from-bulk-actions-2701829-10.patch4.33 KBSoul88
#5 backtrace.png96.36 KBSoul88
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

andypost created an issue. See original summary.

andypost’s picture

Soul88’s picture

Priority: Normal » Major

This issue disables using batches with bulk operations. Here is some code that will reproduce the issue:

drupal8.loc\www\modules\smartling\config\install\system.action.smartling_test_bulk_action.yml

langcode: en
status: true
dependencies:
  module:
    - smartling
id: smartling_test_bulk_action_id
label: 'Test bulk action'
type: node
plugin: smartling_test_bulk_action

drupal8.loc\www\modules\smartling\src\Plugin\Action\TestBulk.php

<?php

/**
 * @file
 * Contains \Drupal\smartling\Plugin\Action\TranslateNode.
 */

namespace Drupal\smartling\Plugin\Action;

use Symfony\Component\DependencyInjection\ContainerInterface;
use Drupal\Core\Action\ActionBase;
use Drupal\Core\Plugin\ContainerFactoryPluginInterface;

use Drupal\Core\Entity\ContentEntityInterface;
use Drupal\Core\Session\AccountInterface;


/**
 * Translate entity.
 *
 * @Action(
 *   id = "smartling_test_bulk_action",
 *   label = @Translation("Test bulk operation"),
 *   type = "node",
 * )
 */
class TestBulk extends ActionBase implements ContainerFactoryPluginInterface {

  /**
   * Creates TranslateNode action.
   *
   * @param array $configuration
   *   A configuration array containing information about the plugin instance.
   * @param string $plugin_id
   *   The plugin ID for the plugin instance.
   * @param mixed $plugin_definition
   *   The plugin implementation definition.
   */
  public function __construct(array $configuration, $plugin_id, $plugin_definition) {
    parent::__construct($configuration, $plugin_id, $plugin_definition);
  }

  /**
   * {@inheritdoc}
   */
  public static function create(ContainerInterface $container, array $configuration, $plugin_id, $plugin_definition) {
    return new static($configuration, $plugin_id, $plugin_definition);
  }

  /**
   * {@inheritdoc}
   */
  public function executeMultiple(array $entities) {
    $operations = [];

    foreach ($entities as $entity) {

      if (isset($entity)) {
        $item_data = [
          'entity_type' => $entity->getEntityTypeId(),
          'entity_id' => $entity->id(),
        ];

        $operations[] = [
          [get_class($this), 'processBatch'],
          [$item_data],
        ];
      }
    }

    if ($operations) {
      $batch = [
        'title' => t('Sending to Smartling'),
        'operations' => $operations,
        'finished' => [get_class($this), 'finishBatch'],
      ];
      batch_set($batch);
    }
  }


  /**
   * Processes the sending batch.
   *
   * @param array $data
   *   Keyed array of data to send.
   * @param array $context
   *   The batch context.
   */
  public static function processBatch($data, &$context) {
    echo 'hi';

  }

  /**
   * Finish batch.
   */
  public static function finishBatch($success, $results, $operations) {
    drupal_set_message(\Drupal::translation()
      ->formatPlural($results['count'], 'One entity has been processed successfully.', '@count entities have been processed successfully.'));
  }

  /**
   * {@inheritdoc}
   */
  public function execute(ContentEntityInterface $entity = NULL) {
    $this->executeMultiple([$entity]);
  }

  /**
   * {@inheritdoc}
   */
  public function access($object, AccountInterface $account = NULL, $return_as_object = FALSE) {
    return TRUE;
  }
}

The fatal error will be triggered after execution of "executeMultiple" method and before "processBatch". It will happen in "function twig_init(Extension $theme)" because an empty array will be passed to it as a parameter.

Recoverable fatal error: Argument 1 passed to twig_init() must be an instance of Drupal\Core\Extension\Extension, array given in \drupal8.loc\www\core\themes\engines\twig\twig.engine on line 34

I'm also bumping it to major, as I think batches are pretty important for the vbo.

andypost’s picture

Category: Task » Bug report

Then it's a bug

Soul88’s picture

FileSize
96.36 KB

Backtrace of the error in attachment

dawehner’s picture

Before we remove this bit, we should better understand why this was implemented/added in the first place.

Soul88’s picture

I believe that the piece of code from 1st message was added on 1.01.2014 by webchick. Here is the commit: http://cgit.drupalcode.org/drupal/commit/?id=42ac4e5

Though I don't know the history behind it.

Soul88’s picture

One more useful link: https://gist.github.com/andypost/1cb961c849c005e8b37260060273571c for the issue

views$ git blame -L 1981,1989 views.module
2dd76fe0 (Earl Miles              2009-11-02 22:01:27 +0000 1981) function views_exposed_form($form, &$form_state) {
31dd0540 (The Great Git Migration 2009-10-05 22:49:04 +0000 1982)   // Don't show the form when batch operations are in progress.
4f09d3fd (Earl Miles              2010-10-14 20:04:09 +0000 1983)   if ($batch = batch_get() && isset($batch['current_set'])) {
31dd0540 (The Great Git Migration 2009-10-05 22:49:04 +0000 1984)     return array(
31dd0540 (The Great Git Migration 2009-10-05 22:49:04 +0000 1985)       // Set the theme callback to be nothing to avoid errors in template_preprocess_views_exposed_form().
31dd0540 (The Great Git Migration 2009-10-05 22:49:04 +0000 1986)       '#theme' => '',
31dd0540 (The Great Git Migration 2009-10-05 22:49:04 +0000 1987)     );
31dd0540 (The Great Git Migration 2009-10-05 22:49:04 +0000 1988)   }
31dd0540 (The Great Git Migration 2009-10-05 22:49:04 +0000 1989) 
andypost’s picture

Issue summary: View changes
Soul88’s picture

Priority: Major » Critical
FileSize
4.33 KB

Some update from my side.

I tried deleting lines from the first post

  public function buildForm(array $form, FormStateInterface $form_state) {
    // Don't show the form when batch operations are in progress.
    if ($batch = batch_get() && isset($batch['current_set'])) {
      return array(
        // Set the theme callback to be nothing to avoid errors in template_preprocess_views_exposed_form().
        '#theme' => '',
      );
    }

But it doesn't solve the issue.

What I found during debug is that Drupal dies after the call of batch_set() in the following lines:

\Drupal\Core\Theme\ThemeInitialization::loadActiveTheme

      if (function_exists($theme_engine . '_init')) {
        foreach ($active_theme->getBaseThemes() as $base) {
          call_user_func($theme_engine . '_init', $base->getExtension()); //this call causes fatal
        }
        call_user_func($theme_engine . '_init', $active_theme->getExtension());
      }

$active_theme->getBaseThemes() returns 2 themes: "classy" and "stable". "Classy" works fine and "stable" has empty "extension" property which doesn't match twig_init(Extension $theme) argument type check.

Any ideas on what might be the cause for this issue are welcome. Because I'm a bit stuck right now.

Soul88’s picture

Issue summary: View changes
Soul88’s picture

Issue summary: View changes
Soul88’s picture

After some further investigation I found out that we're "loosing" extension property during deserialization of the theme object in \Drupal\Core\Cache\DatabaseBackend::getMultiple (when we extract it from cache_bootstrap). Before batch_set() call $item->baseThemes['stable']->extension is an object. And after "batch_set()" call it's an empty array.

andypost’s picture

Priority: Critical » Major
Issue tags: +Contributed project blocker

It's not critical according https://www.drupal.org/core/issue-priority

andypost’s picture

VBO called from \Drupal\views\Plugin\views\display\DisplayPluginBase::elementPreRender()
So exposed form executed much early before
Going to debug that deeper

Soul88’s picture

andypost, the issue here is cache-related. If you comment out the code that retrieves theme from cache - error disappears.

andypost’s picture

Yep, the problem in batch deserialization in bertik and seven themes, here's a module to reproduce bug (test included)

andypost’s picture

Here's a test only patch
Suppose we should get rid of this code and prevent building exposed form early in \Drupal\views\ViewExecutable::build()

EDIT the code returns
Argument 1 passed to twig_init() must be an instance of Drupal\Core\Extension\Extension, integer given

jibran’s picture

+++ b/core/modules/views/src/Tests/UserBatchActionTest.php
@@ -0,0 +1,49 @@
+      debug($theme, 'default theme');

debug eh!

Status: Needs review » Needs work

The last submitted patch, 18: 2701829-18-testonly.patch, failed testing.

andypost’s picture

Also that happens only when view has exposed form.
Backtrace in #5 points that when view is deserialized from batch it fails

About issue title - it still makes sense, in case of deserialization the view is executed but batch_get() still empty, so this code is useless

httang12’s picture

Seems like this happens the object is reference in the serialized form. Testing with 8.1 release, the serialized Extension is in form of r:xx where xx is the object reference. Using deserialize is not able to reconstruct the object.

andypost’s picture

Assigned: andypost » Unassigned
Issue tags: -Needs tests

yep, the problem with serialization here, so maybe makes sense to split removal of the hunk and buggy filters

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

alexpott’s picture

Title: Get rid of '#theme' => '' in \Drupal\views\Form\ViewsExposedForm::buildForm » Cannot set batches in Views preventing port of VBO
Priority: Major » Normal

Discussed with @timplunkett, @cilefen, @dawehner, @xjm - are there any workarounds for this in VBO or is it completely impossible to batch operations.

andypost’s picture

Priority: Normal » Major

Completely impossible if any filters are exposed

dawehner’s picture

Completely impossible if any filters are exposed

Can you clarify that statement? Is there a bug in Drupal itself you can trigger easily?

andypost’s picture

@dawehner I know no batches in core for views, but the views itself broken to support that, see patch #18 and #21

Soul88’s picture

@andypost, we had some discussion with @dawehner and @Berdir in Dublin and had an idea, that theoretically it could be error of PHP interpreter. Unfortunately I didn't manage to isolate this behaviour outside outside of views module.

andypost’s picture

Affected all form submissions of exposed form (test in #18 just using admin/people)

Then I last time debugged it I found that set batch in submission makes somehow view or view executable to be serialized (batch needs sandbox context) and the form (or form state) serialized also...
And exactly this deserialization fails to restore exposed form(or state)

Soul88’s picture

Andrey, it happens during theme deserialization. But, in theory some other serialization might influence the process.

dawehner’s picture

I've debugging this with @Soul88 on multiple Drupalcons already. This is just crazy

The last submitted patch, 18: 2701829-18-testonly.patch, failed testing.

hansfn’s picture

I just discovered this problem when testing my module Views Send (using batch API) and exposing the number of items in the pager. I spent too much time finding this issue - adding some search terms:

Drupal\Core\StringTranslation\TranslatableMarkup and twig_init ;-)

Soul88’s picture

hansfn, I wrote a blog article on our workaround for this issue a while ago. Maybe it can save you some time: https://tech.smartling.com/how-to-overcome-the-limitations-of-the-vbo-mo...

andypost’s picture

FYI I send it to retest with latest 7.0 & 7.1

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

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

pingwin4eg’s picture

I found this issue too late. Recently I tried exactly the same, the batch inside an action plugin. But I got another Error, so found another issue in the queue.

The root cause of both issues is exactly the unserialization in PHP. You can read more at #2750463-18: Fatal error: Call to a member function getFieldStorageDefinition() on array.

pingwin4eg’s picture

I supplied the mentioned issue with a very rough workaround - #2750463-21: Fatal error: Call to a member function getFieldStorageDefinition() on array.

You can try it just to understand if we experience the same issue.

pingwin4eg’s picture

XerraX’s picture

I just discovered this problem when testing my module Views Send (using batch API) and exposing the number of items in the pager. I spent too much time finding this issue - adding some search terms:

Drupal\Core\StringTranslation\TranslatableMarkup and twig_init ;-)

i had the same issue with an exposed filter

Graber’s picture

I found another issue that might be related.

When I write a field plugin with a form (like Drupal\system\Plugin\views\field\BulkForm) and set a batch in any viewsForm submit handler, the batch callback is behaving really strange:

\Drupal::service('entity_type.manager') has some unidentified errors, entitles are loaded but there's something wrong with fields.

This is a big obstacle in porting VBO to Drupal 8.

Reproduce:

  • Add any Views field plugin (even extending Drupal\system\Plugin\views\field\BulkForm and overriding viewsFormSubmit method)
  • Set any batch in the submit handler
  • Try to load any node in the batch callback.

I see from previous comments it's probably related to the serialization issue.

Lendude’s picture

Added @andyposts test to #2849674: Complex job in ViewExecutable::unserialize() causes data corruption to see if that passes with the fix proposed there, and it does, so that might be a good way forward for this.

andypost’s picture

Title: Cannot set batches in Views preventing port of VBO » Get rid of '#theme' => '' in \Drupal\views\Form\ViewsExposedForm::buildForm
Category: Bug report » Task
Priority: Major » Normal
Issue tags: -Contributed project blocker +Needs issue summary update

I just RTBCed #2849674: Complex job in ViewExecutable::unserialize() causes data corruption to fix the bug

So this issue should back to original purpose (see #25)

andypost’s picture

Errors only could appear when #info is not array

So here 2 patches
- initialize #info as array to prevent warnings
- "clean" - removes useless #theme = ''

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.

AardWolf’s picture

Is there at least one chance that problem will be fixed?

dawehner’s picture

@andypost
Do you think you can roll a patch with a test again?

andypost’s picture

@dawehner no reason for test, kinda was commited irr, so just cleanup here of unused code

arosboro’s picture

Status: Needs review » Reviewed & tested by the community

I can confirm https://www.drupal.org/files/issues/2701829-45.patch resolves the error message.

catch’s picture

Status: Reviewed & tested by the community » Needs review

The 'clean' patch in 45 looks better to me, but that's not what's been RTBCed, so moving back to needs review. Would also be good if the issue summary linked to the test coverage for this.

joelpittet’s picture

Status: Needs review » Reviewed & tested by the community

RTBC for the clean version: 2701829-45-clean.patch Tried it with VBO and an exposed filter, there was no error during batch before or after but we should remove that exceptional case.

Graber’s picture

Thanks @joelpitet, if this will be fixed in core we'll be able to have 1 redirect less in VBO in every action without confirmation / configuration step (most actions).

alexpott’s picture

Category: Task » Bug report
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

How come we're not going to add a test? If this code produces errors for contrib then I think we should have a test for that. So we can prove which ever version of #45 goes in works. And given that we're making a change to prevent errors surely this is a bug fix?

Graber’s picture

I see in many cases automated tests requirement becomes an issue resolution blocker as a test may consume relatively large amounts of dev time comparing to the actual issue resolution and devs posting patches are not always familiar with maintainers testing plans.

Automated tests are very important but I think they should be treated as a whole for each larger component and created and maintained according to a plan based on selected critical points. I personally add tests if I see an issue has a broader scope or has a tendency of coming back, in general making them a time-saver, not a time-consumer. If there's an evident bug in code there's no need to test for that bug only.

I don't think that mandatory tests for single issues without a bigger plan is a good way to go, case-by-case decisions seem to be better.

Probably this is not a place for such a general discussion but it's definitely something to think about.

@alexpott: do you think that a test in this particular case will save us time in the future?

alexpott’s picture

@Graber yes because the whole thing about this issue is that we don't have test coverage of this in core. I.e. we have nothing that sets a batch as part of a bulk action. Because we're missing that we have the problem. Without test coverage we are not proving it is fixed and not ensuring we don't break it again in the future.

alexpott’s picture

@Graber also in this case hasn't the test coverage also been written already in #18?

Graber’s picture

@alexpott Looks like it. I misunderstood your "needs tests" tag. You need that in one patch, right?

alexpott’s picture

@Graber ideally two patches - one that shows the test still fails and one combined the shows the fix fixes.

Graber’s picture

FileSize
6.47 KB

Right. I applied #18 and 2701829-45.patch and new tests still fail (uploaded the resulting patch - without the cleanup).

There are 2 things I don't like apart from the fact that the test fails:

  1. Test uses the deprecated simpletest.
  2. Test is in the action group but it's located in views folder (Drupal\views\Tests\UserBatchActionTest instead of Drupal\Tests\action\UserBatchActionTest).

Apart from that I tested direct batch execution from a views plugin form and I can also confirm that the 2701829-45.patch solves the issue and we'll be able to skip the evil extra redirect workaraound in cases like VBO.

Now the question is: can we commit the 5-line fix only and create a new issue for the test or should we wait another year until someone will find time (or sponsorship ;) to write it the proper way?

Graber’s picture

I tested VBO without the redirect workaround and without 2701829-45.patch applied to core and there are no issues anymore. Either the issue got solved by some other fix in views or some other changes in VBO code made the issue not appear anymore. Worth checking if this is still reproducible and if it actually needs a fix.

andypost’s picture

Issue tags: -Needs tests

I'm sure batch should work now cos it was fixed in #2849674: Complex job in ViewExecutable::unserialize() causes data corruption

In #45 I said that issue should be kinda cleanup but notice in preprocess "may" happen

So making a test to cover wrong data pass to preprocess imo bad idea

Somehow summary needs update about it

alexpott’s picture

+++ b/core/modules/views/src/Form/ViewsExposedForm.php
@@ -55,8 +55,9 @@ public function buildForm(array $form, FormStateInterface $form_state) {
+        // Set required property to avoid errors in
+        // template_preprocess_views_exposed_form().
+        '#info' => [],

@andypost but if a notice is caused by not having this code then we should be able to have a test that proves that.

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

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

EugenMayer’s picture

Yet not entirely sure this is related, On 8.5.0-alpha1 with vbo 2.1 - creating an action using

class MyAction extends ViewsBulkOperationsActionBase

will work until batch is activated, then it errors with:

app_1           | [19-Mar-2018 08:10:11] WARNING: [pool www] child 1532 said into stderr: "NOTICE: PHP message: TypeError: Argument 1 passed to twig_init() must be an instance of Drupal\Core\Extension\Extension, array given in /var/www/web/core/themes/engines/twig/twig.engine on line 34 #0 [internal function]: twig_init(Array)"
app_1           | [19-Mar-2018 08:10:11] WARNING: [pool www] child 1532 said into stderr: "#1 /var/www/web/core/lib/Drupal/Core/Theme/ThemeInitialization.php(144): call_user_func('twig_init', Array)"
app_1           | [19-Mar-2018 08:10:11] WARNING: [pool www] child 1532 said into stderr: "#2 /var/www/web/core/lib/Drupal/Core/Theme/ThemeInitialization.php(74): Drupal\Core\Theme\ThemeInitialization->loadActiveTheme(Object(Drupal\Core\Theme\ActiveTheme))"
app_1           | [19-Mar-2018 08:10:11] WARNING: [pool www] child 1532 said into stderr: "#3 /var/www/web/core/lib/Drupal/Core/Theme/ThemeManager.php(406): Drupal\Core\Theme\ThemeInitialization->initTheme('adminimal_theme')"
app_1           | [19-Mar-2018 08:10:11] WARNING: [pool www] child 1532 said into stderr: "#4 /var/www/web/core/lib/Drupal/Core/Theme/ThemeManager.php(96): Drupal\Core\Theme\ThemeManager->initTheme(Object(Drupal\Core\Routing\RouteMatch))"
app_1           | [19-Mar-2018 08:10:11] WARNING: [pool www] child 1532 said into stderr: "#5 /var/www/web/core/lib/Drupal/Core/Render/ElementInfoManager.php(74): Drupal\Core\Theme\ThemeManager->getActiveTheme()"
app_1           | [19-Mar-2018 08:10:11] WARNING: [pool www] child 1532 said into stderr: "#6 /var/www/web/core/lib/Drupal/Core/Form/FormBuilder.php(806): Drupal\Core\Render\ElementInfoManager->get..."

Adding implements PluginFormInterface and implementing a dummy like this

   public function buildConfigurationForm(array $form, FormStateInterface $form_state)
    {
        return $form;
    }

does fix this / works as a workaround. Sounds like this issue, but i am not entirely sure

Eduardo Morales Alberti’s picture

I'm using views_send, and a subtheme that extend from seven, Drupal failed on getExtension function because it returns a string instead of a Extension instance.

foreach ($active_theme->getBaseThemes() as $base) {
          if(is_object($base->getExtension())){
            call_user_func($theme_engine . '_init', $base->getExtension());
          }
        }

I tried the #45 patches and it doesn't solve my problem, so I did another change that works for me.
I submit the patch that check that the function returns a Extension type object.

Version: 8.6.x-dev » 8.7.x-dev

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

sun’s picture

With patch #45 / #60, the batch executes, but right afterwards, there is a fatal error when coming back to the originating page /admin/content in my case:

The website encountered an unexpected error. Please try again later.
TypeError: Argument 1 passed to twig_init() must be an instance of Drupal\Core\Extension\Extension, string given in twig_init() (line 34 of core/themes/engines/twig/twig.engine).

twig_init('Left sidebar')
call_user_func('twig_init', 'Left sidebar') (Line: 144)
Drupal\Core\Theme\ThemeInitialization->loadActiveTheme(Object) (Line: 74)
Drupal\Core\Theme\ThemeInitialization->initTheme('thunder_admin') (Line: 406)
Drupal\Core\Theme\ThemeManager->initTheme(Object) (Line: 96)
Drupal\Core\Theme\ThemeManager->getActiveTheme() (Line: 43)
Drupal\Core\Cache\Context\ThemeCacheContext->getContext(NULL) (Line: 118)
Drupal\Core\Cache\Context\CacheContextsManager->convertTokensToKeys(Array) (Line: 307)
Drupal\Core\Render\RenderCache->createCacheID(Array) (Line: 66)
Drupal\Core\Render\RenderCache->get(Array) (Line: 109)
Drupal\Core\Render\PlaceholderingRenderCache->get(Array) (Line: 263)
Drupal\Core\Render\Renderer->doRender(Array, ) (Line: 195)
Drupal\Core\Render\Renderer->render(Array, ) (Line: 226)
Drupal\Core\Render\MainContent\HtmlRenderer->Drupal\Core\Render\MainContent\{closure}() (Line: 582)
Drupal\Core\Render\Renderer->executeInRenderContext(Object, Object) (Line: 227)
Drupal\Core\Render\MainContent\HtmlRenderer->prepare(Array, Object, Object) (Line: 117)
Drupal\Core\Render\MainContent\HtmlRenderer->renderResponse(Array, Object, Object) (Line: 90)
Drupal\Core\EventSubscriber\MainContentViewSubscriber->onViewRenderArray(Object, 'kernel.view', Object)
call_user_func(Array, Object, 'kernel.view', Object) (Line: 111)
Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher->dispatch('kernel.view', Object) (Line: 156)
Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object, 1) (Line: 68)
Symfony\Component\HttpKernel\HttpKernel->handle(Object, 1, 1) (Line: 57)
Drupal\Core\StackMiddleware\Session->handle(Object, 1, 1) (Line: 47)
Drupal\Core\StackMiddleware\KernelPreHandle->handle(Object, 1, 1) (Line: 99)
Drupal\page_cache\StackMiddleware\PageCache->pass(Object, 1, 1) (Line: 78)
Drupal\page_cache\StackMiddleware\PageCache->handle(Object, 1, 1) (Line: 47)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle(Object, 1, 1) (Line: 50)
Drupal\Core\StackMiddleware\NegotiationMiddleware->handle(Object, 1, 1) (Line: 23)
Stack\StackedHttpKernel->handle(Object, 1, 1) (Line: 664)
Drupal\Core\DrupalKernel->handle(Object) (Line: 19)

The error /admin/content disappears after running drush cr.

davidwhthomas’s picture

Had this same issue with VBO and the fatal error:

TypeError: Argument 1 passed to twig_init() must be an instance of Drupal\Core\Extension\Extension, string given in twig_init() (line 34 of core/themes/engines/twig/twig.engine).
twig_init('css/block/block.admin.css')
call_user_func('twig_init', 'css/block/block.admin.css') (Line: 145)

Odd that twig_init is being called with a string CSS file path.
It's from the related parent base theme:

/core/themes/stable/stable.info.yml

libraries-override:
  block/drupal.block.admin:
    css:
      theme:
        css/block.admin.css: css/block/block.admin.css

While it would be good if the extra checking was not needed, the patch in #66, to check the object is returned, fixed the issue from here

wturrell’s picture

+1 for EduardoMadrid's symptoms and patch in #66 - in my case with the views_send module rather than VBO, but again for an admin sub theme extending seven.

andypost’s picture

Would be great to debug how "region name" may appear as extension...

khiminrm’s picture

Patch from #66 helped me to fix TypeError: Argument 1 passed to twig_init() must be an instance of Drupal\Core\Extension\Extension, string given in twig_init( with VBO and admin theme based on adminimal_theme.

alexpott’s picture

Debugging as shown that this is being caused by recursive references being cached in \Drupal\Core\Theme\ThemeInitialization::getActiveThemeByName(). So everything gets very confused.

alexpott’s picture

Status: Needs work » Needs review
FileSize
1 KB

Patch attached fixes the problem but I can't for the life of me reproduce the fail in a test. I can however with Thunder and this does fix the issue.

Also discovered #3019482: ActiveTheme::getBaseThemes() can return the wrong themes whilst work on this. Unfortunately just fixing that does not fix this.

Status: Needs review » Needs work

The last submitted patch, 74: 2701829-74.patch, failed testing. View results

andypost’s picture

@alexpott how Thunder reptofuce the issue?
Looks related issue is not about a stub used here for #info which is #45 thying to cleanup

alexpott’s picture

What I'm seeing is the same stack trace as in #68 but yeah looking at this this issue maybe it has been side-tracked by a separate issue since #65.

That said this issue is caused by a corruption of the active theme cache that results in the extension becoming an array which is exactly the same problem being reported in #3 so perhaps these are completely related.

Here's how with thunder to reproduce the issue I'm seeing:

  • Install latest Thunder
  • Install VBO 8.x-2.4 (note not the dev version)
  • Add VBO to admin/content and remove core's bulk operations and configure VBO to use a batch
  • Try using VBO on admin/content -> breaks
alexpott’s picture

Status: Needs work » Needs review
FileSize
5.54 KB

I've rerolled the test in #18. Things are current even worse in 8.7.x because we have more serialization problems :(

Status: Needs review » Needs work

The last submitted patch, 78: 2701829-2-78.patch, failed testing. View results

alexpott’s picture

Status: Needs work » Needs review
FileSize
4.78 KB
9.36 KB

Here's a fix that is green - at least for the new test.

alexpott’s picture

+++ b/core/lib/Drupal/Core/Extension/ModuleHandler.php
@@ -234,7 +234,15 @@ public function buildModuleDependencies(array $modules) {
+      foreach ($modules[$module_name]->required_by as $key => $required_by) {
+        // Ensure we don't have circular object references.
+        $modules[$module_name]->required_by[$key] = clone $required_by;
+      }

The required_by key is interesting. Nothing in core ever uses the value. That's because it is not that interesting. It is just a dependency object representing the extension that is required - ie. it is all the same value. I think we should field a followup to make this do something like

$modules[$module_name]->required_by = array_combine(array_keys($modules[$module_name]->required_by), array_keys($modules[$module_name]->required_by));
alexpott’s picture

:( this doesn't fix Thunder on 8.7.x - need to do #81 to fix it there. Something feels very very brittle.

alexpott’s picture

Priority: Normal » Major
FileSize
1.12 KB
10.09 KB

Yay here is a failing test which fails exactly the same way as Thunder. Just needed one more theme in the chain like thunder_admin which inherit from seven. I think adminimal might inherit form seven too.

I think this is a major bug - by using common modules - views_bulk_operations - and a common install profile - thunder you can produce a WSOD that results in a corrupted theme cache and WSODs for any other uncached page.

Status: Needs review » Needs work

The last submitted patch, 83: 2701829-2-83.patch, failed testing. View results

alexpott’s picture

Status: Needs work » Needs review
FileSize
9.53 KB
20.59 KB

So here is fix. It deprecates \Drupal\Core\Theme\ActiveTheme::getBaseThemes() in favour of \Drupal\Core\Theme\ActiveTheme::getBaseThemeExtensions(). At the moment in HEAD the active theme cache is super complex because it stores an active theme but all of the base themes are also active themes... and if any of them have base themes they're active themes too. I'm not sure this information is needed by core and it appears that unserializing this recursive thing is very problematic.

alexpott’s picture

Also this is likely to be a perfomance boost because

Before

select length(data) from cache_bootstrap where cid = 'theme.active_theme.seven';
+--------------+
| length(data) |
+--------------+
|        90370 |
+--------------+

After

mysql> select length(data) from cache_bootstrap where cid = 'theme.active_theme.seven';
+--------------+
| length(data) |
+--------------+
|        43590 |
+--------------+

Status: Needs review » Needs work

The last submitted patch, 85: 2701829-2-85.patch, failed testing. View results

alexpott’s picture

Status: Needs work » Needs review
FileSize
7.08 KB
24.37 KB

Here's an updated patch with:

  • The unit tests fixed
  • full BC layer test coverage
  • All @todo's added resolved

Created the change record - https://www.drupal.org/node/3019948

Now to update the issue summary.

alexpott’s picture

Title: Get rid of '#theme' => '' in \Drupal\views\Form\ViewsExposedForm::buildForm » Reduce the complexity of the active theme and extension list caches
Issue summary: View changes

Updated the issue summary.

jibran’s picture

Given #86 do need actual profiling here?

+++ b/core/lib/Drupal/Core/Theme/ActiveTheme.php
@@ -51,9 +51,21 @@ class ActiveTheme {
   protected $baseThemes;
...
+  protected $baseThemesExtensions;

FWIW, I know for BC reasons we are adding new property but I prefer $baseThemes over $baseThemesExtensions as far as naming is concerned.

alexpott’s picture

@jibran the problem is we need a different value to pass in and the property matching the value makes sense. That said it should be baseThemeExtensions. Also we have the problem of the old method getBaseThemes() vs the new method getBaseThemeExtensions() - so I don't think we can use the old property unfortunately.

I don't think we need to profile per se - we can see the cache key size is dramatically reducing that is not going to have a performance cost - that is going to have a performance benefit or be a wash.

alexpott’s picture

Here's a profile of cold caches showing the improvement from storing less stuff and doing less stuff - https://blackfire.io/profiles/compare/e21a0d28-9f1e-4488-b094-f78bec83b8...
Here's a profile of warm caches showing the improved memory usage - https://blackfire.io/profiles/compare/a4045574-796a-421b-a518-a49369d63c...

The profile were made by calling \Drupal::service('theme.initialization')->initTheme('seven'); where the method had been altered to add instrumentation:

  /**
   * {@inheritdoc}
   */
  public function initTheme($theme_name) {
    $blackfire = new \Blackfire\Client();
    $probe = $blackfire->createProbe(null, false);
    $probe->enable();
    $active_theme = $this->getActiveThemeByName($theme_name);
    $this->loadActiveTheme($active_theme);
    $probe->disable();

    // end the profiling session
    $profile = $blackfire->endProbe($probe);
    return $active_theme;
  }
jibran’s picture

Status: Needs review » Reviewed & tested by the community

Thanks, for clearing that up and profiling. All the changes, in the patch, have tests and they make sense. I don't think we need a test only patch here so setting it to RTBC.

heddn’s picture

That's a lot fewer calls to Extension::getPath and dirname. And half as much memory. Great to see the drop.

I went through the patch pretty closely on a code only review. It seems to all be in order. Going to bump to RTBC, but if someone else who is more closely familiar with the theme engine wants to review and find things, by all means do so.

alexpott’s picture

There are test-only-(ish) patches on the issue already. #78 is a test-only patch for the extension list problems and #83 is a test-only patch for the active theme problems.

alexpott’s picture

Issue summary: View changes
  • Crediting @dawehner for issue comments and debugging at Drupalcons.
  • Crediting @pingwin4eg because of comments pointing to serialization issues.
  • Crediting @jibran for a review that resulting fixing a property name.
  • Crediting @httang12 as their comment also points to the problem area.
alexpott’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
8.58 KB
11.18 KB

So reading through all the comments has made me realise that we probably have a much much smaller fix. We can open some of the performance improvements as separate fixes.

alexpott’s picture

Title: Reduce the complexity of the active theme and extension list caches » Extension objects should not implement \Serializable
Issue summary: View changes

Issue summary updated.

alexpott’s picture

alexpott’s picture

alexpott’s picture

Hmmm ok testServiceAppRouteUsage was weirdly calling the ->serialize() method directly. That's not how it was ever called. It was always done as the result of the serialize() PHP in-built method.

The last submitted patch, 97: 2701829-3-97.test-only.patch, failed testing. View results

The last submitted patch, 97: 2701829-3-97.patch, failed testing. View results

Berdir’s picture

+++ b/core/lib/Drupal/Core/Extension/Extension.php
@@ -4,8 +4,13 @@
+ *
+ * This class does not implement the Serializable interface since problems
+ * occurred when using the serialize method.
+ *
...
  */
-class Extension implements \Serializable {
+class Extension {
 

Weirdly, the libraries.module used to have a ExtensionInterface + subclass of this so that would break if someone still used that old version.

That was changed two years ago, not sure if anyone still uses that, it was only a dev version then. I just noticed that because I still had that version in my local D8 checkout.

Berdir’s picture

Status: Needs review » Reviewed & tested by the community

Just wanted to point that out, e.g. in case someone is ends up here if their site breaks after an update.

I think this is ready, great how _sleep()/__wakeup() result in much cleaner code, while we might still have those properties being set, we don't have to worry about them anymore with this implementation.

Berdir’s picture

We can likely even backport this implementation to 8.6.x as it might be less common to explode there but could still happen?

alexpott’s picture

Yeah it totally does explode in 8.6.x with Thunder + views_bulk_operations when triggering a batch - which as we know involves lots of serialisation and unserialisation. It looks like this has been blowing up since at least 8.2.x!

The current patch does not apply to 8.6.x because of changes to the Extension object in #2881981: Mitigate Extension dependency on DRUPAL_ROOT - arguably we could backport that issue and then this issue backports cleanly.

alexpott’s picture

Ah no there's also #2659940: Extension System, Part III: ThemeExtensionList and ThemeEngineExtensionList which means that backporting #2881981: Mitigate Extension dependency on DRUPAL_ROOT is not enough so it would be a re-roll for 8.6.x but I think that that is worth it given the unexpected issues this can cause.

catch’s picture

I don't think we need a CR here, feels like unwanted noise in the CR list. However I'm also cautious about an 8.6.x backport so think we should commit to 8.7.x and let it be in there for a few weeks before backporting.

catch’s picture

Version: 8.7.x-dev » 8.6.x-dev
Status: Reviewed & tested by the community » Needs work

Committed 451a7ba and pushed to 8.7.x. Thanks!

Back to 8.6.x for a re-roll, let's not commit this until just after the next 8.6 patch release though.

  • catch committed 451a7ba on 8.7.x
    Issue #2701829 by alexpott, andypost, Soul88, Graber, EduardoMadrid,...
alexpott’s picture

Here's the patch re-rolled on top of 8.6.x

The last submitted patch, 112: 2701829-8.6.x-112.test-only.patch, failed testing. View results

chr.fritsch’s picture

Status: Needs review » Reviewed & tested by the community

I tested the patch with 8.6.x, Thunder and VBO and it fixed our issues.

  • catch committed 236623f on 8.6.x
    Issue #2701829 by alexpott, andypost, Soul88, Graber, Eduardo Morales,...
catch’s picture

Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

oknate’s picture

I tested the reroll for 8.6 and I'm getting many many notices:

Warning: Class Drupal\Core\Extension\Extension has no unserializer in Drupal\Component\FileCache\ApcuFileCacheBackend->fetch() (line 14 of core/lib/Drupal/Component/FileCache/ApcuFileCacheBackend.php).
Drupal\Component\FileCache\ApcuFileCacheBackend->fetch(Array) (Line: 107)
Drupal\Component\FileCache\FileCache->getMultiple(Array) (Line: 70)
Drupal\Component\FileCache\FileCache->get('/home/vagrant/docroot/web/core/modules/update/update.info.yml') (Line: 451)
Drupal\Core\Extension\ExtensionDiscovery->scanDirectory('core', ) (Line: 205)
Drupal\Core\Extension\ExtensionDiscovery->scan('profile') (Line: 102)
Drupal\Core\Extension\ModuleExtensionList->getProfileDirectories(Object) (Line: 85)
Drupal\Core\Extension\ModuleExtensionList->getExtensionDiscovery() (Line: 290)
Berdir’s picture

Yes, you need to clear caches, possibly restart apache or clear the APCU cache in a different way, that should fix that.

oknate’s picture

Thanks! Restarting vagrant after applying the patch in my case made those warnings go away. Thanks.

I came across the error mentioned above where a subtheme of seven was getting an ActiveTheme with a string where the extension should be, throwing an error from twig_init.

AardWolf’s picture

Thank you guys for your fix.
It's working good on D8.7.
I've checked it in my module only now.