console command custom-piwik-js:update should work when directory is writable and file does not exist yet (#10576)

* followup #10449

* Fix test
(cherry picked from commit fac3d63)

* Prevent chmod(): No such file or directory

3 files changed, 75 insertions, 12 deletions

diff --git a/plugins/CustomPiwikJs/File.php b/plugins/CustomPiwikJs/File.php
index 5ef1a13c6f..03d874e61d 100644
--- a/plugins/CustomPiwikJs/File.php
+++ b/plugins/CustomPiwikJs/File.php
@@ -38,7 +38,9 @@ class File
 
     public function save($content)
     {
-        file_put_contents($this->file, $content);
+        if(false === file_put_contents($this->file, $content)) {
+            throw new AccessDeniedException(sprintf("Could not write to %s", $this->file));
+        }
     }
 
     public function getContent()
@@ -60,7 +62,10 @@ class File
      */
     public function hasWriteAccess()
     {
-        return is_writable($this->file);
+        if (file_exists($this->file) && !is_writable($this->file)) {
+            return false;
+        }
+        return is_writable(dirname($this->file)) || is_writable($this->file);
     }
 
     /**
diff --git a/plugins/CustomPiwikJs/tests/Integration/FileTest.php b/plugins/CustomPiwikJs/tests/Integration/FileTest.php
index 4d96bd6478..aa8cee9c5d 100644
--- a/plugins/CustomPiwikJs/tests/Integration/FileTest.php
+++ b/plugins/CustomPiwikJs/tests/Integration/FileTest.php
@@ -19,7 +19,8 @@ use Piwik\Tests\Framework\TestCase\IntegrationTestCase;
  */
 class FileTest extends IntegrationTestCase
 {
-    const NOT_EXISTING_FILE = 'notExisTinGFile.js';
+    const NOT_EXISTING_FILE_IN_WRITABLE_DIRECTORY = 'notExisTinGFile.js';
+    const NOT_EXISTING_FILE_IN_NON_WRITABLE_DIRECTORY = 'is-not-writable/notExisTinGFile.js';
 
     /**
      * @var string
@@ -30,14 +31,23 @@ class FileTest extends IntegrationTestCase
     {
         parent::setUp();
         $this->dir = PIWIK_DOCUMENT_ROOT . '/plugins/CustomPiwikJs/tests/resources/';
+
+        // make directory not writable
+        $nonWritableDir = dirname($this->dir . self::NOT_EXISTING_FILE_IN_NON_WRITABLE_DIRECTORY);
+        @chmod($nonWritableDir, 0444);
+        if(is_writable($nonWritableDir)) {
+            throw new \Exception("The directory $nonWritableDir should have been made non writable by this test, but it didn't work");
+        }
     }
 
     public function tearDown()
     {
-        if (file_exists($this->dir . self::NOT_EXISTING_FILE)) {
-            unlink($this->dir . self::NOT_EXISTING_FILE);
-        }
+        // restore permissions changed by makeNotWritableFile()
+        chmod($this->dir, 0777);
 
+        if (file_exists($this->dir . self::NOT_EXISTING_FILE_IN_WRITABLE_DIRECTORY)) {
+            unlink($this->dir . self::NOT_EXISTING_FILE_IN_WRITABLE_DIRECTORY);
+        }
         parent::tearDown();
     }
 
@@ -46,9 +56,40 @@ class FileTest extends IntegrationTestCase
         return new File($this->dir . $fileName);
     }
 
+    private function makeNotWritableFile()
+    {
+        $path = $this->dir . 'file-made-non-writable.js';
+        if(file_exists($path)) {
+            chmod($path, 0777);
+        }
+        $file = new File($path);
+        $file->save('will be saved OK, and then we make it non writable.');
+
+        if (!chmod($path, 0444)) {
+            throw new \Exception("chmod on the file didn't work");
+        }
+        if (!chmod(dirname($path), 0755)) {
+            throw new \Exception("chmod on the directory didn't work");
+        }
+        $this->assertTrue(is_writable(dirname($path)));
+        $this->assertFalse(is_writable($path));
+        $this->assertTrue(file_exists($path));
+        return $file;
+    }
+
     private function makeNotReadableFile()
     {
-        return $this->makeFile(self::NOT_EXISTING_FILE);
+        return $this->makeNotReadableFile_inWritableDirectory();
+    }
+
+    private function makeNotReadableFile_inNonWritableDirectory()
+    {
+        return $this->makeFile(self::NOT_EXISTING_FILE_IN_NON_WRITABLE_DIRECTORY);
+    }
+
+    private function makeNotReadableFile_inWritableDirectory()
+    {
+        return $this->makeFile(self::NOT_EXISTING_FILE_IN_WRITABLE_DIRECTORY);
     }
 
     public function test_getName()
@@ -66,7 +107,13 @@ class FileTest extends IntegrationTestCase
     public function test_hasWriteAccess()
     {
         $this->assertTrue($this->makeFile()->hasWriteAccess());
-        $this->assertFalse($this->makeNotReadableFile()->hasWriteAccess());
+        $this->assertTrue($this->makeNotReadableFile_inWritableDirectory()->hasWriteAccess());
+        $this->assertFalse($this->makeNotReadableFile_inNonWritableDirectory()->hasWriteAccess());
+    }
+
+    public function test_hasWriteAccess_whenFileExistAndIsNotWritable()
+    {
+        $this->assertFalse($this->makeNotWritableFile()->hasWriteAccess());
     }
 
     public function test_checkReadable_shouldNotThrowException_IfIsReadable()
@@ -96,7 +143,12 @@ class FileTest extends IntegrationTestCase
      */
     public function test_checkWritable_shouldThrowException_IfNotIsWritable()
     {
-        $this->makeNotReadableFile()->checkWritable();
+        $this->makeNotReadableFile_inNonWritableDirectory()->checkWritable();
+    }
+
+    public function test_checkWritable_shouldNotThrowException_IfDirectoryIsWritable()
+    {
+        $this->makeNotReadableFile_inWritableDirectory()->checkWritable();
     }
 
     public function test_getContent()
@@ -111,9 +163,9 @@ class FileTest extends IntegrationTestCase
 
     public function test_save()
     {
-        $notExistingFile = $this->makeNotReadableFile();
+        $notExistingFile = $this->makeNotReadableFile_inWritableDirectory();
         $this->assertFalse($notExistingFile->hasReadAccess());
-        $this->assertFalse($notExistingFile->hasWriteAccess());
+        $this->assertTrue($notExistingFile->hasWriteAccess());
 
         $notExistingFile->save('myTestContent');
 
diff --git a/plugins/CustomPiwikJs/tests/Integration/TrackerUpdaterTest.php b/plugins/CustomPiwikJs/tests/Integration/TrackerUpdaterTest.php
index 240638c5e9..58650adf7e 100644
--- a/plugins/CustomPiwikJs/tests/Integration/TrackerUpdaterTest.php
+++ b/plugins/CustomPiwikJs/tests/Integration/TrackerUpdaterTest.php
@@ -62,7 +62,13 @@ class TrackerUpdaterTest extends IntegrationTestCase
      * @expectedException \Piwik\Plugins\CustomPiwikJs\Exception\AccessDeniedException
      * @expectedExceptionMessage not writable
      */
-    public function test_checkWillSucceed_shouldThrowExceptionIfTargetIsNotWritable()
+    public function test_checkWillSucceed_shouldNotThrowExceptionIfTargetIsNotWritable()
+    {
+        $updater = $this->makeUpdater(null, $this->dir . 'not-writable/MyNotExisIngFilessss.js');
+        $updater->checkWillSucceed();
+    }
+
+    public function test_checkWillSucceed_shouldNotThrowExceptionIfTargetIsWritable()
     {
         $updater = $this->makeUpdater(null, $this->dir . 'MyNotExisIngFilessss.js');
         $updater->checkWillSucceed();