update_delete_file_if_stale() must return a value or update_manager_file_get() will always fetch remote files

please review now.

1. +++ b/core/modules/update/src/Tests/UpdateDeleteFileIfStaleTest.php
   @@ -45,9 +45,9 @@ function testUpdateDeleteFileIfStale() {
    
   -    $this->assertFalse(is_file($file_path));
   +		$this->assertTrue($deleted, 'Successfull deleted the stale file');
   

   Always use two spaces instead of tabs.

2. +++ b/core/modules/update/update.module
   @@ -741,14 +741,19 @@ function update_clear_update_disk_cache() {
     * @param $path
     *   A string containing a file path or (streamwrapper) URI.
   + * @return
   + *	 TRUE if delete file successfully, FALSE otherwise.
   

   There must be an empty line above @return and @return must be followed by the type (bool)

Please review updated patch.

Please review update patch with minor fixes.

update_manager_file_get-fetch-stale-files-2303323-6.patch still applies cleanly at f07bcff .

update_manager_file_get-fetch-stale-files-2303323-6.patch still applies cleanly at f07bcff

This should have a test-only patch uploaded to show that the updated test fails and catches the bug.

https://www.drupal.org/contributor-tasks/write-tests

Added the test-only patch.

err.. that one was supposed to fail anyway

I reviewed this issue and for me work as expected. Also it follows the coding standars. RTBC?

Looking at this as part of Sprint Weekend 2016.

The patch as it stands does test the return value of update_delete_file_if_stale(). However, it no longer tests that the file is actually deleted!

I've modified the test so it checks:

1. the file exists.
2. trying to delete a non-stale file returns FALSE.
3. the file is deleted when stale.
4. trying to delete a stale file returns TRUE.

All six assertions in the test pass on my dev build, so submitting for review.

The patch applies to 8.2.x and looks good to me. The added tests properly checks if the file was deleted and the status of the returned boolean.

Also, since #17 doesn't include a fail patch, I've added one. Also retesting #17 on 8.2.x

So, the fail patch in #20 shows that even though the stale file is deleted a success (true) is not returned. The patch in #17 is the fix and looks good to me.

But this is tagged with Needs change record, so there is still work to do.

RE uploading the patch in #17, the latest working one, so this can be set to NR.

Does this need a change record?

An attempt at a reroll and move to 8.9.x

Just seeing this issue for the first time.

Mostly looks great. Love the expanded test coverage. The change itself is small and good. One very minor quibble:

+++ b/core/modules/update/tests/src/Kernel/UpdateDeleteFileIfStaleTest.php
@@ -36,10 +51,12 @@ public function testUpdateDeleteFileIfStale() {
+    // Now attempt to delete the file now; as it should be considered stale,

We don't need "now" twice in this comment.

Otherwise, I wonder if we want a better title for this issue for git history reasons. The title talks about update_manager_file_get(), but only change here is in update_delete_file_if_stale().

Finally, I agree we probably need a CR for this.

Thanks!
-Derek

A little git archeology. This bug came in when update_delete_file_if_stale() was first added at #605318: Add some garbage collection to the update manager. Not sure how we missed it. ;) That issue was re-opened at #605318-67: Add some garbage collection to the update manager for exactly this problem. I'm going to go ahead and re-close that one in favor of this, since the patch here is in better shape than the latest over there. However, crediting Alan D. so their contribution at #605318 is acknowledged.

Hi Dww,

As per suggestion I have updated the patch.

Before

+++ b/core/modules/update/tests/src/Kernel/UpdateDeleteFileIfStaleTest.php
@@ -36,10 +51,12 @@ public function testUpdateDeleteFileIfStale() {
+    // Now attempt to delete the file now; as it should be considered stale,

Now

+++ b/core/modules/update/tests/src/Kernel/UpdateDeleteFileIfStaleTest.php
@@ -36,10 +51,12 @@ public function testUpdateDeleteFileIfStale() {
+    // Now attempt to delete the file; as it should be considered stale,

Thanks

Changing status

Attempt at resolving #30 with a more complete title.

Sorry I didn't notice before, but one other minor optional change:

+++ b/core/modules/update/tests/src/Kernel/UpdateDeleteFileIfStaleTest.php
@@ -27,6 +27,21 @@ public function testUpdateDeleteFileIfStale() {
+    $this->assertTrue(is_file($file_path), 'Non-stale file is not deleted.');
+    // Test return status $deleted is consistent.
+    $this->assertFalse($deleted, 'Stale file deletion reports failure correctly.');

@@ -36,10 +51,12 @@ public function testUpdateDeleteFileIfStale() {
+    $this->assertFalse(is_file($file_path), 'Stale file is deleted.');
+    // As before, test return status $deleted is consistent.
+    $this->assertTrue($deleted, 'Stale file deletion reports success correctly.');

I don't think these comments are needed given the assertions themselves.

If we keep them, we don't need "As before, ".

However, NW because it still needs the CR.

Thanks!
-Derek

+++ b/core/modules/update/tests/src/Kernel/UpdateDeleteFileIfStaleTest.php
@@ -27,6 +27,21 @@ public function testUpdateDeleteFileIfStale() {
+    $this->assertTrue(is_file($file_path), 'Non-stale file is not deleted.');

@@ -36,10 +51,12 @@ public function testUpdateDeleteFileIfStale() {
+    $this->assertTrue($deleted, 'Stale file deletion reports success correctly.');

In addition to #36, assertFileNotExists() and assertFileNotExists() are more appropriate here.

Filed a new issue for #37, as there are more to fix.

Updated test changes, Patch created, Please review.

1. +++ b/core/modules/update/tests/src/Kernel/UpdateDeleteFileIfStaleTest.php
   @@ -27,6 +27,21 @@ public function testUpdateDeleteFileIfStale() {
   +    // Test return status $deleted is consistent.
   

   Should remove this comment.

2. +++ b/core/modules/update/tests/src/Kernel/UpdateDeleteFileIfStaleTest.php
   @@ -27,6 +27,21 @@ public function testUpdateDeleteFileIfStale() {
   +    $this->assertFalse($deleted, 'Stale file deletion reports failure correctly.');
   

   phpunit assertion messages should explain when the assertion fails. (Messages used to be positive in the SimpleTest world, but are negative now).

3. +++ b/core/modules/update/tests/src/Kernel/UpdateDeleteFileIfStaleTest.php
   @@ -36,10 +51,12 @@ public function testUpdateDeleteFileIfStale() {
   +    // As before, test return status $deleted is consistent.
   

   Remove this.

4. +++ b/core/modules/update/tests/src/Kernel/UpdateDeleteFileIfStaleTest.php
   @@ -36,10 +51,12 @@ public function testUpdateDeleteFileIfStale() {
   +    $this->assertTrue($deleted, 'Stale file deletion reports success correctly.');
   

   Fix this (see above).

5. +++ b/core/modules/update/update.module
   @@ -832,6 +832,10 @@ function update_clear_update_disk_cache() {
   + *
   

   https://www.drupal.org/pift-ci-job/1664921

   /var/lib/drupalci/workspace/jenkins-drupal_patches-43446/source/core/modules/update/update.module ✗ 1 more
   line 839 Additional blank lines found at end of doc comment

Attached fixes all of this. But still NW for the change record. ;)

Added the draft CR:

https://www.drupal.org/node/3131432

Needs version / branch info filled in when published, depending on how far back this gets backported.

RTBC?

Thanks
-Derek

Slightly improved the @return docs (IMHO). ;) Canceled the testbot run on #41. Let's use this, instead.

Self-review. Attached fixes all this:

1. +++ b/core/modules/update/tests/src/Kernel/UpdateDeleteFileIfStaleTest.php
   @@ -27,6 +27,20 @@ public function testUpdateDeleteFileIfStale() {
   +    // During testing the file change and the stale checking occurs in the same
   

   Wants , after "During testing".

2. +++ b/core/modules/update/tests/src/Kernel/UpdateDeleteFileIfStaleTest.php
   @@ -27,6 +27,20 @@ public function testUpdateDeleteFileIfStale() {
   +    $this->assertFalse($deleted, 'Non-stale file deletion should report failure.');
   

   A) Should come right after we get the value for $deleted.

   B) We don't need the assertion message here at all. It's obvious from the comments, and we're trying to get rid of those.

3. +++ b/core/modules/update/tests/src/Kernel/UpdateDeleteFileIfStaleTest.php
   @@ -36,10 +50,11 @@ public function testUpdateDeleteFileIfStale() {
   +    // Now attempt to delete the file; as it should be considered stale,
   +    // this attempt should succeed.
   

   80 char wrapping is off.

4. +++ b/core/modules/update/tests/src/Kernel/UpdateDeleteFileIfStaleTest.php
   @@ -36,10 +50,11 @@ public function testUpdateDeleteFileIfStale() {
   +    $this->assertTrue($deleted, 'Stale file deletion should report success.');
   

   Same as above:
   - Move directly under update_delete_file_if_stale() call.
   - Remove message.

Sorry, forgot test-only + full. This should be red/green candy.

Thanks @dww.

1. +++ b/core/modules/update/tests/src/Kernel/UpdateDeleteFileIfStaleTest.php
   @@ -27,6 +27,20 @@ public function testUpdateDeleteFileIfStale() {
   +    // During testing, the file change and the stale checking occurs in the same
   +    // request, so the beginning of request will be before the file changes and
   +    // REQUEST_TIME - $filectime is negative or zero. Set the maximum age to a
   +    // number greater than that.
   ...
        // During testing the file change and the stale checking occurs in the same
        // request, so the beginning of request will be before the file changes and
   

   2 ~ 3 lines are duplicate, should the comments be reworded/adjusted?

2. +++ b/core/modules/update/update.module
   @@ -710,10 +713,13 @@ function update_delete_file_if_stale($path) {
        if (REQUEST_TIME - $filectime > $max_age || (preg_match('/.*-dev\.(tar\.gz|zip)/i', $path) && REQUEST_TIME - $filectime > 300)) {
   

   Not sure the following are in scope:

   1. Replace REQUEST_TIME with \Drupal::time()->getRequestTime()?
   2. $filectime -> $file_ctime ?

Otherwise, it's good to me.

@jungle: Thanks for the review! Re: #47:

1. Yeah, probably a good move. Fixed here. How's this?

2.1: IMHO, out of scope. That should be handled (everywhere) when core properly deprecates REQUEST_TIME.

2.2: Nope, we're referring to the value returned by filectime(). But to clarify, changing from $filectime to filectime(). ;)

Re: #46: I missed in drupalci.yml, this test-only should properly fail...

RTBC? 🤞

    $filectime = filectime($path);
    $max_age = \Drupal::config('system.file')->get('temporary_maximum_age');

    if (REQUEST_TIME - $filectime > $max_age || (preg_match('/.*-dev\.(tar\.gz|zip)/i', $path) && REQUEST_TIME - $filectime > 300)) {
    

To make it clear. I meant changing $filectime = filectime($path); to $file_ctime = filectime($path);, agree on that both 47.2.1 and 47.2.2 are out of scope.

The change looks good. #46 passed, #48 should pass too. So setting to RTBC. Thanks @dww!

Re: #49: Oh, sorry, I totally missed the context you were quoting in #47.2. Whoops. But yeah, same answer. ;) Still out of scope.

Thanks for the review and RTBC!

Cheers,
-Derek