To see why, we just need to modernize it.

Which is a Good Idea Anyway™.

CommentFileSizeAuthor
#12 interdiff.txt4.42 KBsun
#12 file.delete-recursive.12.patch11.68 KBsun
#7 drupal8.file-delete-recursive.7.patch7.26 KBsun
drupal8.file-delete-recursive.0.patch6.96 KBsun

Comments

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Related issues: +#2145103: Provide non-configurable field types for entity created, changed and timestamp fields

Example test failure: https://qa.drupal.org/pifr/test/725328

Status: Needs review » Needs work

The last submitted patch, drupal8.file-delete-recursive.0.patch, failed testing.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

Test fail looks like this:

rmdir(sites/simpletest/863507): Directory not empty<pre class="backtrace">rmdir('sites/simpletest/863507') drupal_rmdir('sites/simpletest/863507') file_unmanaged_delete_recursive('sites/simpletest/863507', Array) Drupal\simpletest\TestBase->restoreEnvironment() Drupal\simpletest\TestBase->run() simpletest_script_run_one_test('269', 'Drupal\field\Tests\reEnableModuleFieldTest') </pre>

(Copying so that I can re-test that issue).

berdir’s picture

berdir
Primary language German
Location Switzerland
commented 
+++ b/core/modules/system/lib/Drupal/system/Tests/File/UnmanagedDeleteRecursiveTest.php
@@ -27,9 +27,16 @@ function testSingleFile() {
     $filepath = file_default_scheme() . '://' . $this->randomName();
     file_put_contents($filepath, '');
 
-    // Delete the file.
-    $this->assertTrue(file_unmanaged_delete_recursive($filepath), 'Function reported success.');
-    $this->assertFalse(file_exists($filepath), 'Test file has been deleted.');
+    // Try to "recursively delete" the file.
+    $message = 'Caught InvalidArgumentException in attempt to delete a file with file_unmanaged_delete_recursive().';
+    try {
+      file_unmanaged_delete_recursive($filepath);
+      $this->fail($message);
+    }
+    catch (\InvalidArgumentException $e) {
+      $this->pass($message);
+    }
+    $this->assertTrue(file_exists($filepath), 'Test file has not been deleted.');
   }

This seems to be the opposite of the old test, how so?

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Priority: Normal » Critical

Random test failures are critical, this is happening frequently. See also #2194357: CacheArray::__destruct() invoked after test tables have been removed for the second type of new random test fails that we currently have.

neclimdul’s picture

neclimdul
He/Him
commented

I'm never a fan of embedding test code into library functions. We can mitigate that concern though by changing "$chmod" to "$force" though. We don't need to expose the implementation, just that we're going to take extra steps to remove it even if it wouldn't normally be deleted.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs work » Needs review
StatusFileSize
new drupal8.file-delete-recursive.7.patch7.26 KB

@neclimdul: Good idea, done that.

@Berdir: Yeah, I've adjusted the test accordingly, because "deleting a file recursively" makes no whatsoever sense at all. :) The caller should already know whether it is dealing with a file or a directory.

Status: Needs review » Needs work

The last submitted patch, 7: drupal8.file-delete-recursive.7.patch, failed testing.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs work » Needs review

7: drupal8.file-delete-recursive.7.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 7: drupal8.file-delete-recursive.7.patch, failed testing.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

Hrm. The test failures are permanent and are staying the same:

unlink(sites/simpletest/191952/settings.php): Permission denied

unlink('sites/simpletest/191952/settings.php')
file_unmanaged_delete_recursive('sites/simpletest/191952', 1)
Drupal\simpletest\TestBase->restoreEnvironment()
Drupal\simpletest\TestBase->run()
simpletest_script_run_one_test('151', 'Drupal\content_translation\Tests\ContentTranslationSettingsTest')

This means that even the $force flag of file_unmanaged_delete_recursive() is not able to delete the files in the test site directory.

→ Typically, this just simply means CLI user != Apache user.

This would also explain why testbots are constantly running out of disk space, because run-tests.sh is simply not able to clean up all artifacts after each test run. — Every file that has been created by the separate Apache process cannot be deleted.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs work » Needs review
StatusFileSize
new file.delete-recursive.12.patch11.68 KB
new interdiff.txt4.42 KB

This is a bit insane, but I wanted to see whether it resolves the test failures... :-)

Prepare test child site destruction via system route.

Status: Needs review » Needs work

The last submitted patch, 12: file.delete-recursive.12.patch, failed testing.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Parent issue: » #2229865: [meta] Modernize File/StreamWrapper API
catch’s picture

catch
he/him
Primary language English
commented
Category: Task » Bug report
Priority: Critical » Major
Status: Needs work » Postponed (maintainer needs more info)

Haven't seen this for a long time, is there an actual bug here?

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Postponed (maintainer needs more info) » Needs work

The errors are not visible, because file_unmanaged_delete_recursive() + its called functions are using @error-suppression.

However, at this point, this is more of Testing system bug & File API bug than a random test failure.

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
commented
Issue tags: +Needs issue summary update
mgifford’s picture

mgifford
he/him
Primary language English
commented
Assigned: sun » Unassigned

Unassigning stale issue. Hopefully someone else will pursue this.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Toronto, Canada
commented

Can we close this now? I don't think #16 is valid now. Or is it?

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

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should 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.

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

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should 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.

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

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should 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.

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

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should 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.

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

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should 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.

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

Drupal 8.6.x will not receive any further development aside from security fixes. Bug reports should be targeted against the 8.8.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.9.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.7 was released on June 3, 2020 and is the final full bugfix release for the Drupal 8.8.x series. Drupal 8.8.x will not receive any further development aside from security fixes. Sites should prepare to update to Drupal 8.9.0 or Drupal 9.0.0 for ongoing support.

Bug reports should be targeted against the 8.9.x-dev branch from now on, and new development or disruptive changes should be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

quietone’s picture

quietone commented
Status: Needs work » Closed (outdated)
Issue tags: +Bug Smash Initiative

Thanks for opening.

Dig some digging and found this is no longer relevant, file_unmanaged_delete_recursive was deprecated in 8.7.0 in #2244513: Move the unmanaged file APIs to the file_system service (file.inc) and removed in #3062757: Remove deprecated legacy include files from Drupal 9 .