Problem

  • When calling file_unmanaged_*() functions in a test, exceptions are thrown, because custom stream wrappers are not properly registered.

Cause

  1. All registered custom stream wrappers of the parent site (test runner) are still registered when executing a (simple)test → they leak into the test environment.
  2. Tests do not register their own stream wrappers specific to the executed test.

Details

  • file_get_stream_wrappers() only works for WebTestBase, after static variables have been reset.
  • The private and temporary stream wrappers can only work if System module and its configuration has been installed.
  • Raw unit tests should not have access to any custom stream wrappers, but currently they do, because the stream wrappers of the test runner environment are leaking into unit tests, too.
  • Unlike all other global state in Drupal and tests, stream wrappers are a native global state construct of PHP core.

    → The state of registered stream wrappers has to properly managed and maintained manually.

Proposed solution

  1. In TestBase::prepareEnvironment(), all custom stream wrappers of the parent site are unregistered/removed.
  2. UnitTestBase does not get any stream wrappers.
  3. DrupalUnitTestBase gets dedicated methods for managing stream wrappers and it ensures to clean up after itself in tearDown().
  4. WebTestBase is not changed. file_get_stream_wrappers() returns the new set of custom stream wrappers after the test site has been installed.

.

Notes

  1. As stated in #2028109-108: Convert hook_stream_wrappers() to tagged services. (and following), Drupal's Stream Wrapper API needs a major overhaul for D8 to properly maintain the state of registered stream wrappers for a particular DrupalKernel boot, build, and rebuild.

    The concept required for DrupalKernel essentially has to follow the concept that is introduced for DrupalUnitTestBase here.

    → The admittedly ugly workarounds for file_get_stream_wrappers() should not hold up this patch, because this entire functionality has to be moved into DrupalKernel anyway.

  2. This issue blocks #2176141: Add a return value to file_save_htaccess()

    → which in turn blocks #2171683: Remove all Simpletest overrides and rely on native multi-site functionality instead

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sun’s picture

Attached patch should expose the test failures.

sun’s picture

@beejeebus made me aware of the fact that all of these inherit from FileTestCase ;)

sun’s picture

1) Stream wrappers should be registered in a full bootstrap, but setUp() eliminates pretty much everything that has been set up during bootstrap.

2) Even if some stuff would be retained, that wouldn't solve this problem. That is, because those File API tests attempt to directly use native PHP file functions on file URIs using a custom stream wrapper. The stream wrapper is registered through file_test.module only. And, circling back into 1), setUp() does not perform a full bootstrap after resetting everything and before executing the test.

Not sure why attached patch does not work. Technically, it should run a total bootstrap again, but the second debug() yields a NULL for me.

Anonymous’s picture

Issue tags: +DrupalWTF

sooo. the reason these tests pass with the bloat standard profile is:

1. we blow away all statics in setUp().

2. we have code in the standard profile that calls in to file_get_stream_wrappers():

standard_install --> theme_disable --> menu_rebuild --> menu_router_build --> call_user_func --> image_menu --> file_stream_wrapper_get_instance_by_scheme --> file_stream_wrapper_get_class --> file_get_stream_wrappers

3. file_get_stream_wrappers, which has no static cache anymore, *registers the 'dummy' and 'dummy-remote' wrappers with php*. obvious, yeah?

the upshot is: enabling a module does not register any stream wrappers it implements, until file_get_stream_wrappers' static cache is cleared AND it is called again.

hello drupalwtf.

sun’s picture

Priority: Normal » Major
Status: Needs work » Needs review
FileSize
5.87 KB

So let's perform a proper, clean, and reliable bootstrap into the testing environment.

Apparently, the entire current testing environment is primed with data from the parent site.

That's mostly because we're resetting $conf and statics way too early; the subsequent functions calls are priming the statics again -- before we install the system.

A proper bootstrap also eliminates the resetAll() within the setUp() - as there's nothing anymore that could be reset!

sun’s picture

Title: Drupal stream wrappers are not necessarily registered when calling file_unmanaged_*() functions » Tests are executed in incomplete environment (was: Stream wrappers not registered when calling file_unmanaged_*())
Component: file system » simpletest.module
Status: Needs work » Needs review
Issue tags: +Testing system
FileSize
7.11 KB

Backup of my latest code; contains some @todos.

Summary:

  1. Some of the failing tests are the same as in #1373142: Use the Testing profile. Speed up testbot by 50%. This is important. It means that some of the failing tests actually do not pass. They only pass due to some weird intermixing of environment data from the parent site executing the test; e.g., as outlined in #4.
  2. We can resolve this by fixing setUp() to perform a clean and full bootstrap into the testing environment. A full bootstrap sets up various data; among that happens to be the registration of stream wrappers defined by modules. Essentially, the goal is to perform a bootstrap as in Drupal's installer, just without the installer.
  3. We are only touching the surface of #1215104: Use the non-interactive installer in WebTestBase::setUp() here. But it makes sense to continue with this patch, because the errors and exceptions we're bumping into seem to be the same I encountered for the patch over there. In other words, this patch will help us to unblock and do the other work.
sun’s picture

Status: Needs work » Needs review
+++ b/core/modules/simpletest/drupal_web_test_case.php
@@ -1363,6 +1363,15 @@ class DrupalWebTestCase extends DrupalTestCase {
+    // @todo Theme is not reset after test bootstrap yet. Why?

See #1213536: Non-resettable theme_get_registry() cache causes problems for non-interactive installations

catch’s picture

Issue tags: +Needs backport to D7

Tagging.

sun’s picture

sun’s picture

Holy cow. It's possible that #12 revealed some tests that false-positively pass in HEAD. (check the review log)

sun’s picture

Priority: Major » Normal
Status: Needs work » Needs review
FileSize
4.75 KB

Re-rolled against HEAD.

It's possible that #1215104: Use the non-interactive installer in WebTestBase::setUp() might resolve parts of this patch though.

Anonymous’s picture

Issue summary: View changes

Updated issue summary.

sun’s picture

Title: Tests are executed in incomplete environment (was: Stream wrappers not registered when calling file_unmanaged_*()) » Stream wrappers of parent site are leaking into all tests
Assigned: Unassigned » sun
Priority: Normal » Major
Issue summary: View changes
Status: Needs work » Needs review
Parent issue: » #2171683: Remove all Simpletest overrides and rely on native multi-site functionality instead
Related issues: +#2176141: Add a return value to file_save_htaccess()
FileSize
11.01 KB
sun’s picture

Status: Needs work » Needs review
FileSize
11.92 KB

Hopefully this one resolves most of the test failures. Cherry-tested a few locally and they passed.

sun’s picture

Status: Needs work » Needs review
FileSize
36.65 KB

Fixed a whole bunch of tests.

Also fixed File Managed API tests (File module) to no longer use the File API test base class (System module).

sun’s picture

Status: Needs work » Needs review
FileSize
35.32 KB
1.61 KB

Reverted TestBase::$public_files_directory path change → that's only possible with #2171683: Remove all Simpletest overrides and rely on native multi-site functionality instead

sun’s picture

FileSize
36.33 KB
5.58 KB
  1. Clarified and polished new documentation about stream wrappers in tests.
  2. Reverted unnecessary relocation of test files directory deletion (does not use a stream wrapper).
tstoeckler’s picture

Is there any way to get a test-only patch to fail here a la #12? That would be pretty cool IMO to understand and prove what is currently broken.

Other than that I only have nitpicks.

  1. +++ b/core/modules/file/lib/Drupal/file/Tests/FileManagedTestBase.php
    @@ -90,6 +90,51 @@ function assertFileHookCalled($hook, $expected_count = 1, $message = NULL) {
    +   * timestamp.
    ...
    +   * Check that two files have the same values for all fields other than the
    

    Check -> Checks

    Also should be on one line. I would suggest leaving the timestamp info out of the one-line description and sticking that into a separate line.

  2. +++ b/core/modules/simpletest/lib/Drupal/simpletest/DrupalUnitTestBase.php
    @@ -64,6 +64,15 @@
    +  private $stream_wrappers = array();
    

    Since this seems to be introduced here, it should be $streamWrappers

  3. +++ b/core/modules/simpletest/lib/Drupal/simpletest/TestBase.php
    @@ -963,6 +963,19 @@ protected function prepareEnvironment() {
    +    // - WebTestBase re-initializes Drupal stream wrappers are installation.
    

    "after" installation?

sun’s picture

Issue summary: View changes
FileSize
36.34 KB
5.03 KB

Fixed coding style and comment issues.

Updating issue summary to properly reflect the original bug report and proposed solution.

dawehner’s picture

  1. +++ b/core/includes/file.inc
    @@ -191,12 +191,16 @@ function file_get_stream_wrappers($filter = STREAM_WRAPPERS_ALL) {
    +      drupal_alter('stream_wrappers', $wrappers);
    

    Nitpick: We could just use the alter method on the module handler instead.

  2. +++ b/core/modules/file/lib/Drupal/file/Tests/DownloadTest.php
    @@ -17,7 +17,7 @@ public static function getInfo() {
     
    
    +++ b/core/modules/file/lib/Drupal/file/Tests/CopyTest.php
    @@ -15,7 +15,7 @@ public static function getInfo() {
           'description' => 'Tests the file copy function.',
    -      'group' => 'File API',
    +      'group' => 'File Managed API',
         );
    
    +++ b/core/modules/file/lib/Drupal/file/Tests/DeleteTest.php
    @@ -15,7 +15,7 @@ public static function getInfo() {
       }
    
    +++ b/core/modules/file/lib/Drupal/file/Tests/DownloadTest.php
    @@ -17,7 +17,7 @@ public static function getInfo() {
           'name' => 'File download',
           'description' => 'Tests for file download/transfer functions.',
    -      'group' => 'File API',
    +      'group' => 'File Managed API',
         );
    
    +++ b/core/modules/file/lib/Drupal/file/Tests/DeleteTest.php
    @@ -15,7 +15,7 @@ public static function getInfo() {
    -      'group' => 'File API',
    +      'group' => 'File Managed API',
         );
    

    These changes are a bit out of scope but yeah do whatever you think is good.

  3. +++ b/core/modules/system/lib/Drupal/system/Tests/File/UrlRewritingTest.php
    @@ -95,6 +97,11 @@ function testRelativeFileURL() {
    +    // Create a mock Request for file_url_transform_relative().
    +    $request = Request::create($GLOBALS['base_url']);
    +    $this->container->set('request', $request);
    

    This is a bit odd, have you tried using preparerRequestForGenerator instead?

  4. +++ b/core/modules/system/lib/Drupal/system/Tests/File/RemoteFileDirectoryTest.php
    @@ -19,6 +19,9 @@ class RemoteFileDirectoryTest extends DirectoryTest {
    +  protected $classname = 'Drupal\file_test\DummyRemoteStreamWrapper';
    ...
    +  protected $scheme = 'dummy-remote';
    

    Can we document these two variables on the baseclass?

sun’s picture

FileSize
36.57 KB
1.27 KB
  1. Use ModuleHandler::alter() in file_get_stream_wrappers() while being there.
  2. Added phpDoc for preexisting FileTestBase::$scheme and $classname properties while being there.

re: group changes:

There was a really weird intermixing of classes going on — File module's FileManagedTestBase extended System module's FileTestBase class, whereas both serve completely different purposes. Not even the System\FileTestBase class methods were used by System's File API tests.

Additionally, only the tests based on FileManagedTestBase are web tests. All of the File API tests are low-level tests and hence converted to DrupalUnitTestBase here → massive speedup.

That said, FileTestBase is primarily converted to DUTB to ensure that the stream wrapper management facility works correctly for those tests.

re: mock Request in UrlRewritingTest:

The preparerRequestForGenerator() method only exists on WebTestBase (while this is DUTB) and only makes sense there. The mock Request is merely needed, because file_url_transform_relative() needs one, whereas DUTB tests don't have one (and probably shouldn't have one).

tstoeckler’s picture

Status: Needs review » Reviewed & tested by the community

Looked through this again and looks fine to me.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 30: test.streams.30.patch, failed testing.

sun’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
36.6 KB

Merged 8.x. Simple diff context conflict, so no interdiff, and back to RTBC.

sun’s picture

36: test.streams.36.patch queued for re-testing.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/includes/file.inc
    @@ -202,12 +202,16 @@ function file_get_stream_wrappers($filter = STREAM_WRAPPERS_ALL) {
    +    $container = \Drupal::getContainer();
    +    if (is_object($container) && $container->has('module_handler')) {
    

    Considering that \Drupal::getContainer() is currently deprecated perhaps we should add Drupal::has() that checks both the container's and the service's existence?

  2. +++ b/core/modules/file/lib/Drupal/file/Tests/FileManagedTestBase.php
    @@ -7,14 +7,14 @@
      * hooks.
    
    +++ b/core/modules/simpletest/lib/Drupal/simpletest/DrupalUnitTestBase.php
    @@ -132,10 +142,28 @@ protected function setUp() {
    +    // The private and temporary stream wrappers depend on the system.file
    +    // configuration. If System module was enabled, the test will most likely
    +    // install System module's configuration, so we can register them here.
    

    That is an interesting assumption that I think is often not true.

  3. +++ b/core/modules/system/lib/Drupal/system/Tests/File/RemoteFileUnmanagedSaveDataTest.php
    @@ -19,6 +19,9 @@ class RemoteFileUnmanagedSaveDataTest extends UnmanagedSaveDataTest {
    +  protected $scheme = 'dummy-remote';
    +  protected $classname = 'Drupal\file_test\DummyRemoteStreamWrapper';
    
    +++ b/core/modules/system/lib/Drupal/system/Tests/File/RemoteFileUnmanagedMoveTest.php
    @@ -19,6 +19,9 @@ class RemoteFileUnmanagedMoveTest extends UnmanagedMoveTest {
    +  protected $scheme = 'dummy-remote';
    +  protected $classname = 'Drupal\file_test\DummyRemoteStreamWrapper';
    
    +++ b/core/modules/system/lib/Drupal/system/Tests/File/RemoteFileUnmanagedDeleteTest.php
    @@ -19,6 +19,9 @@ class RemoteFileUnmanagedDeleteTest extends UnmanagedDeleteTest {
    +  protected $scheme = 'dummy-remote';
    +  protected $classname = 'Drupal\file_test\DummyRemoteStreamWrapper';
    
    +++ b/core/modules/system/lib/Drupal/system/Tests/File/RemoteFileUnmanagedDeleteRecursiveTest.php
    @@ -19,6 +19,9 @@ class RemoteFileUnmanagedDeleteRecursiveTest extends UnmanagedDeleteRecursiveTes
    +  protected $scheme = 'dummy-remote';
    +  protected $classname = 'Drupal\file_test\DummyRemoteStreamWrapper';
    
    +++ b/core/modules/system/lib/Drupal/system/Tests/File/RemoteFileUnmanagedCopyTest.php
    @@ -19,6 +19,9 @@ class RemoteFileUnmanagedCopyTest extends UnmanagedCopyTest {
    +  protected $scheme = 'dummy-remote';
    +  protected $classname = 'Drupal\file_test\DummyRemoteStreamWrapper';
    
    +++ b/core/modules/system/lib/Drupal/system/Tests/File/RemoteFileScanDirectoryTest.php
    @@ -19,6 +19,9 @@ class RemoteFileScanDirectoryTest extends ScanDirectoryTest {
    +  protected $scheme = 'dummy-remote';
    +  protected $classname = 'Drupal\file_test\DummyRemoteStreamWrapper';
    
    +++ b/core/modules/system/lib/Drupal/system/Tests/File/RemoteFileDirectoryTest.php
    @@ -19,6 +19,9 @@ class RemoteFileDirectoryTest extends DirectoryTest {
    +  protected $scheme = 'dummy-remote';
    +  protected $classname = 'Drupal\file_test\DummyRemoteStreamWrapper';
    

    Missing doc blocks

sun’s picture

Status: Needs work » Needs review
FileSize
39.78 KB
12.03 KB
  1. Not going to address that here, since #2028109: Convert hook_stream_wrappers() to tagged services. will remove that entire function anyway later on.
  2. Removed assumption about private stream wrapper + System module config from DrupalUnitTestBase.
  3. Added phpDoc for preexisting $scheme and $classname test properties.

Given how many other major/critical issues are currently blocked on this fix, it would be great to move forward here as quickly as possible.

tim.plunkett’s picture

twistor’s picture

I agree about #38.1. That code won't be living long.

Everything else has been addressed. RTBC assuming this comes back green.

Status: Needs review » Needs work

The last submitted patch, 39: test.streams.39.patch, failed testing.

alexpott’s picture

39: test.streams.39.patch queued for re-testing.

sun’s picture

Status: Needs work » Reviewed & tested by the community

Back to RTBC, I guess.

alexpott’s picture

Version: 8.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Committed 856d6e1 and pushed to 8.x. Thanks!

sun’s picture

Assigned: sun » Unassigned

Hm. Given that there are a few necessary adjustments to individual tests, I'm not sure whether it is safe to backport this change.

We can certainly try, of course.

A backport is only acceptable if it is possible with zero changes to individual test classes.

sun’s picture

  • alexpott committed 856d6e1 on 8.3.x
    Issue #1376122 by sun: Stream wrappers of parent site are leaking into...

  • alexpott committed 856d6e1 on 8.3.x
    Issue #1376122 by sun: Stream wrappers of parent site are leaking into...

  • alexpott committed 856d6e1 on 8.4.x
    Issue #1376122 by sun: Stream wrappers of parent site are leaking into...

  • alexpott committed 856d6e1 on 8.4.x
    Issue #1376122 by sun: Stream wrappers of parent site are leaking into...