[Module]Bundle is registered too late in WebTestBase::setUp() [#1786990]

#1

Posted by tim.plunkett on October 2, 2012 at 4:09pm

Priority:	normal	» major
Issue tags:		+VDC

We've been having this problem in Views, as well as #1708692: Simpletest slow and other problems following Bundle services aren't available in the request that enables the module, I think we just assumed it was all the other issue.

Login or register to post comments

#2

Posted by effulgentsia on October 2, 2012 at 10:48pm

@tim.plunkett: Can you post a small patch here that demonstrates the bug with a failing test? I just tried a fresh checkout of Views 8.x-3.x, and tests seem to be passing without the hack in #1708692-19: Simpletest slow and other problems following Bundle services aren't available in the request that enables the module.

Login or register to post comments

Attachment	Size	Status	Test result	Operations
views-1786990-3.patch	2.49 MB	Idle	FAILED: [[SimpleTest]]: [MySQL] 43,291 pass(es), 15 fail(s), and 1 exception(s).	View details

#4

Posted by sun on October 3, 2012 at 1:13am

I don't get that. As for the OP, the service is registered by SystemBundle, which is (or should be) available at all times, regardless of install profile.

Login or register to post comments

Here's the relevant call stack for one such occurence: https://privatepaste.com/ddf8081988
Note lines 13 and 14. l() is being called, which is calling theme_get_registry(), and then everything goes to hell.

So, I tried putting this into ViewTestBase:

  /**
   * Overrides TestBase::changeDatabasePrefix().
   *
   * This is the dirtiest hack. It is the only method called in
   * WebTestBase::setUp() that is after it resets $conf and before it calls
   * module_enable().
   */
  protected function changeDatabasePrefix() {
    variable_set('theme_link', FALSE);
    return parent::changeDatabasePrefix();
  }

And it all worked out.

Login or register to post comments

#6

Posted by System Message on October 3, 2012 at 1:43am

Status:

needs review

» needs work

The last submitted patch, views-1786990-3.patch, failed testing.

Login or register to post comments

#7

Posted by effulgentsia on October 8, 2012 at 10:47pm

Status:

needs work

» needs review

From #1805996-8: [META] Views in Drupal Core:

5. Include Views and Views UI in the standard install profile.
Blocked on #1786990: [Module]Bundle is registered too late in WebTestBase::setUp()

Here's a patch that's identical to that one, but with views and views_ui added as dependencies to standard.info. I'm curious what breaks.

Attachment	Size	Status	Test result	Operations
vdc-test.patch	2.5 MB	Idle	FAILED: [[SimpleTest]]: [MySQL] 43,540 pass(es), 16 fail(s), and 0 exception(s).	View details

Login or register to post comments

#8

Posted by System Message on October 8, 2012 at 11:24pm

Status:

needs review

» needs work

The last submitted patch, vdc-test.patch, failed testing.

Login or register to post comments

#9

Posted by effulgentsia on October 9, 2012 at 12:16am

Status:

needs work

» needs review

Will the exact same tests fail on just this?

Attachment	Size	Status	Test result	Operations
bundle-registration-1786990-9.patch	1.79 KB	Idle	FAILED: [[SimpleTest]]: [MySQL] 40,283 pass(es), 12 fail(s), and 0 exception(s).	View details

Login or register to post comments

#10

Posted by System Message on October 9, 2012 at 1:08am

Status:

needs review

» needs work

The last submitted patch, bundle-registration-1786990-9.patch, failed testing.

Login or register to post comments

#11

Posted by xjm on October 9, 2012 at 1:34am

That has got to be the single best example of narrowing scope I've ever seen. 2.5 MB to less than 2K. ;)

And yep, looks like they're all croaking with:

exception 'Symfony\Component\DependencyInjection\Exception\InvalidArgumentException' with message 'The service definition "views.bundle" does not exist.' in /var/lib/drupaltestbot/sites/default/files/checkout/core/vendor/symfony/dependency-injection/Symfony/Component/DependencyInjection/ContainerBuilder.php:690

Login or register to post comments

#12

Posted by effulgentsia on October 9, 2012 at 1:41am

Looks like there's 4 ModuleAPI failures in #7 that aren't in #9. Trying to find out what's causing them.

Login or register to post comments

#13

Posted by effulgentsia on October 9, 2012 at 2:03am

Ah, those were a red herring, just due to standard.info not including 'config', a Views dependency. Ok, looks like registration of [Module]Bundle happening after hook_theme() is the key scope of this issue, as demonstrated in #9.

Login or register to post comments

#14

Posted by sun on October 9, 2012 at 2:18am

I suspect we need to turn the conclusion around. Possible causes:

1) theme() may be invoked before full bootstrap, or, before the kernel container is instantiated.

2) hook_theme() may be called by the installer, but the installer may not register bundles of modules.

3) Unlimited variations of 1) and 2) ;)

Login or register to post comments

#15

Posted by andypost on October 11, 2012 at 12:07am

Status:

needs work

» needs review

That's caused by filter_permission()

<?php
$format_name_replacement = user_access('administer filters') ? l($format->name, 'admin/config/content/formats/' . $format->format) : drupal_placeholder($format->name);
?>

and then common.inc l()

<?php
  if (!isset($use_theme) && function_exists('theme')) {
    // Allow edge cases to prevent theme initialization and force inline link
    // rendering.
    if (variable_get('theme_link', TRUE)) {
      drupal_theme_initialize();
      $registry = theme_get_registry(FALSE);
?>

Attachment	Size	Status	Test result	Operations
filter_broken.jpg	33.07 KB	Ignored: Check issue status.	None	None
bundle-registration-1786990-15.patch	2.72 KB	Idle	PASSED: [[SimpleTest]]: [MySQL] 42,146 pass(es).	View details
1786990-interdiff-9vs15.txt	868 bytes	Ignored: Check issue status.	None	None

Login or register to post comments

#16

Posted by tim.plunkett on October 11, 2012 at 12:15am

Status:

needs review

» needs work

That treats the symptom, yes.

But as sun points out in #14, this could happen in other ways as well.

Login or register to post comments

#17

Posted by effulgentsia on October 11, 2012 at 1:18am

Status:

needs work

» needs review

The problem is in WebTestBase::setUp(), we install_drupal() for the profile being tested and then module_enable() any additional modules needed by the test. The module_enable() invokes hooks which could end up calling l(), or any number of other functions that end up expecting services from modules enabled by install_drupal(). Here's a really ugly fix, though I hope we can fix this more robustly in other issues like #1708692: Simpletest slow and other problems following Bundle services aren't available in the request that enables the module and #1784312: Stop doing so much pre-kernel bootstrapping.

Attachment	Size	Status	Test result	Operations
bundle-registration-1786990-17.patch	5.41 KB	Idle	PASSED: [[SimpleTest]]: [MySQL] 42,152 pass(es).	View details
bundle-registration-1786990-17-fix-only.patch	3.48 KB	Idle	PASSED: [[SimpleTest]]: [MySQL] 42,122 pass(es).	View details

Login or register to post comments

#18

Posted by sun on October 11, 2012 at 1:25am

+++ b/core/modules/simpletest/lib/Drupal/simpletest/WebTestBase.php
@@ -716,24 +717,9 @@ protected function setUp() {
     if ($modules) {
       $success = module_enable($modules, TRUE);
       $this->assertTrue($success, t('Enabled modules: %modules', array('%modules' => implode(', ', $modules))));
+      $this->rebuildContainer();
     }

I think we need the rebuild in all cases, not just when additional modules to install have been specified. That is, because ->kernel and ->container are provided for test classes.

+++ b/core/modules/simpletest/lib/Drupal/simpletest/WebTestBase.php
@@ -779,6 +765,35 @@ protected function refreshVariables() {
+  protected function rebuildContainer() {

Given #1774388: Unit Tests Remastered™, we likely need to move this into TestBase.

Login or register to post comments

#19

Posted by effulgentsia on October 11, 2012 at 1:58am

Since both patches in #17 are green, I removed the mock Views example from here on out. I don't think we need any test case in this patch, because the real problem still needs to be dealt with in #1708692: Simpletest slow and other problems following Bundle services aren't available in the request that enables the module. The purpose of this issue is just to allow #1805996: [META] Views in Drupal Core to add Views to the Standard install profile.

This implements #18.2. #18.1 is already taken care of: notice there's 2 lines in WebTestBase :).

Attachment	Size	Status	Test result	Operations
bundle-registration-1786990-19.patch	3.93 KB	Idle	FAILED: [[SimpleTest]]: [MySQL] 1,294 pass(es), 487 fail(s), and 151 exception(s).	View details

Login or register to post comments

#20

Posted by tim.plunkett on October 11, 2012 at 2:01am

@sun, if this is what you meant in your first comment, then that's fine.

As to the second part, first to RTBC wins :)

EDIT: Ignore this, go with 19.

Attachment	Size	Status	Test result	Operations
drupal-1786990-18-with-views.patch	5.38 KB	Idle	PASSED: [[SimpleTest]]: [MySQL] 42,150 pass(es).	View details
drupal-1786990-18.patch	3.6 KB	Idle	PASSED: [[SimpleTest]]: [MySQL] 42,116 pass(es).	View details
interdiff.txt	707 bytes	Ignored: Check issue status.	None	None

Login or register to post comments

#21

Posted by effulgentsia on October 11, 2012 at 3:12am

Same as #19, but needed to copy the use statement to TestBase.

Attachment	Size	Status	Test result	Operations
bundle-registration-1786990-21.patch	4.13 KB	Idle	PASSED: [[SimpleTest]]: [MySQL] 42,127 pass(es).	View details

Login or register to post comments

#22

Posted by System Message on October 11, 2012 at 3:14am

Status:

needs review

» needs work

The last submitted patch, bundle-registration-1786990-21.patch, failed testing.

Login or register to post comments

#23

Posted by tim.plunkett on October 11, 2012 at 3:16am

Status:

needs work

» needs review

#21: bundle-registration-1786990-21.patch queued for re-testing.

Login or register to post comments

#24

Posted by sun on October 11, 2012 at 4:22am

Status:

needs review

» reviewed & tested by the community

I'm OK with this as a temporary stop-gap, so RTBC on the assumption that the testbot comes back green at some point.

#1809248: Enable apc.enable_cli php.ini setting on testbots was deployed on two testbots, right at the time when #21 was in process of testing, so if there won't be results in a 1-2 hours, I'd recommend to re-upload this patch (or manually send it for re-testing).

(@tim.plunkett: The same apparently applies to your VDC sandbox, FWIW.)

Login or register to post comments

#25

Posted by webchick on October 11, 2012 at 3:42pm

Status:

reviewed & tested by the community

» fixed

Looks straight-forward enough.

Committed and pushed to 8.x. Thanks!

Login or register to post comments

#26

Posted by tim.plunkett on October 11, 2012 at 4:00pm

Status:

fixed

» reviewed & tested by the community

I don't see this in the commit log, can you

git pull --rebase;
git push

please?

Login or register to post comments

#27

Posted by tim.plunkett on October 11, 2012 at 4:16pm

Status:

reviewed & tested by the community

» fixed

Pushed now, thanks

Login or register to post comments

#28

Posted by System Message on October 25, 2012 at 4:21pm

Status:

fixed

» closed (fixed)

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

Login or register to post comments

Project:	Drupal core
Version:	8.x-dev
Component:	base system
Category:	bug report
Priority:	major
Assigned:	Unassigned
Status:	closed (fixed)
Issue tags:	Dependency Injection (DI), VDC, WSCCI

[Module]Bundle is registered too late in WebTestBase::setUp()

Jump to:

Issue Summary

Comments

#1

#2

#3

#4

#5

#6

#7

#8

#9

#10

#11

#12

#13

#14

#15

#16

#17

#18

#19

#20

#21

#22

#23

#24

#25

#26

#27

#28