Problem/Motivation

Our PhpStorage tests currently write directly to the computers temporary directory. This is problematic and we don't even tear down and clean up so we leave files laying around. Personally this also means I can't run threaded tests against issues I'm watching because tests running in another process can remove the file from underneath a test running in a separate process.

History, and review help, the tests where done this way because tempnam doesn't work well with filestreams http://php.net/tempnam#97086. Sadly we can't use our implementation of tempnam that does support filestreams because it requires a bunch of Core services. Instead, we can just brute force this.

Proposed resolution

Use vfsStream to test this. Its why we added the library to core!

Remaining tasks

Review

User interface changes

Nada

API changes

Nil

CommentFileSizeAuthor
#3 use_vfs_for_filestorage-2453399-3.patch10.54 KBneclimdul
#3 use_vfs_for_filestorage-2453399-3.interdiff.txt7.94 KBneclimdul
#1 use_vfs_for_filestorage-2453399-1.patch5.81 KBneclimdul
#1 use_vfs_for_filestorage-2453399-1.interdiff.txt3.13 KBneclimdul
PhpStorageVFS.patch5.65 KBneclimdul
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

neclimdul’s picture

neclimdul
He/Him
CreditAttribution: neclimdul commented
FileSize
use_vfs_for_filestorage-2453399-1.interdiff.txt3.13 KB
use_vfs_for_filestorage-2453399-1.patch5.81 KB

Woops, I wrote some of this last night and apparently it was too late. Better documentation and such.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented 
+++ b/core/lib/Drupal/Component/PhpStorage/MTimeProtectedFastFileStorage.php
@@ -184,4 +184,20 @@ protected function getUncachedMTime($directory) {
+      $path = $directory . '/' . $prefix . substr(str_shuffle(md5(time())), 0, 10);

As said on IRC we should better use sha256 here.

neclimdul’s picture

neclimdul
He/Him
CreditAttribution: neclimdul commented
FileSize
use_vfs_for_filestorage-2453399-3.interdiff.txt7.94 KB
use_vfs_for_filestorage-2453399-3.patch10.54 KB

Done. Touched up some tests too. Microtime because if the first loop fails it'd take until the next second before we actually got a different result out of hash().

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs review » Reviewed & tested by the community

I like it!

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Reviewed & tested by the community » Fixed

I like it too. I've fought will this before and solved the threaded thing but this is better. This issue is a normal bug fix, and doesn't include any disruptive changes, so it is allowed per https://www.drupal.org/core/beta-changes. Committed 98b74d2 and pushed to 8.0.x. Thanks!

  • alexpott committed 98b74d2 on 8.0.x 
    Issue #2453399 by neclimdul: Use VFS for FileStorage tests

Status: Fixed » Closed (fixed)

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