When updating the config file failed (or when any other file is not writable...), the Updater (for core or plugins) will now automatically throw an error and cancel the update

core/Config.php

@@ -12,6 +12,7 @@
 use Exception;
 use Piwik\Application\Kernel\GlobalSettingsProvider;
 use Piwik\Container\StaticContainer;
+use Piwik\Exception\MissingFilePermissionException;
 use Piwik\ProfessionalServices\Advertising;
 
 /**
@@ -375,15 +376,18 @@ public function dumpConfig()
      */
     protected function writeConfig($clear = true)
     {
-        if ($this->doNotWriteConfigInTests) {
-            return;
-        }
-
         $output = $this->dumpConfig();
         if ($output !== null
             && $output !== false
         ) {
-            $success = @file_put_contents($this->getLocalPath(), $output);
+
+            if ($this->doNotWriteConfigInTests) {
+                // simulate whether it would be successful
+                $success = is_writable($this->getLocalPath());
+            } else {
+                $success = @file_put_contents($this->getLocalPath(), $output);
+            }
+
             if ($success === false) {
                 throw $this->getConfigNotWritableException();
             }
@@ -411,6 +415,6 @@ public function forceSave()
     public function getConfigNotWritableException()
     {
         $path = "config/" . basename($this->getLocalPath());
-        return new Exception(Piwik::translate('General_ConfigFileIsNotWritable', array("(" . $path . ")", "")));
+        return new MissingFilePermissionException(Piwik::translate('General_ConfigFileIsNotWritable', array("(" . $path . ")", "")));
     }
 }

core/Updater.php

@@ -10,6 +10,7 @@
 
 use Piwik\Columns\Updater as ColumnUpdater;
 use Piwik\Container\StaticContainer;
+use Piwik\Exception\MissingFilePermissionException;
 use Piwik\Updater\UpdateObserver;
 use Zend_Db_Exception;
 
@@ -257,8 +258,8 @@ public function update($componentName)
                 $this->markComponentSuccessfullyUpdated($componentName, $fileVersion);
             } catch (UpdaterErrorException $e) {
                 $this->executeListenerHook('onError', array($componentName, $fileVersion, $e));
-
                 throw $e;
+
             } catch (\Exception $e) {
                 $warningMessages[] = $e->getMessage();
 
@@ -552,7 +553,9 @@ private function executeSingleUpdateClass($className)
             // make sure to check for them here
             if ($e instanceof Zend_Db_Exception) {
                 throw new UpdaterErrorException($e->getMessage(), $e->getCode(), $e);
-            } else {
+            } else if ($e instanceof MissingFilePermissionException) {
+                throw new UpdaterErrorException($e->getMessage(), $e->getCode(), $e);
+            }{
                 throw $e;
             }
         }

plugins/CoreAdminHome/tests/Integration/SetConfigTest.php

@@ -36,6 +36,12 @@ public function setUp()
         parent::setUp();
     }
 
+    public function tearDown()
+    {
+        parent::tearDown();
+        $this->makeLocalConfigWritable();
+    }
+
     public function test_Command_SucceedsWhenOptionsUsed()
     {
         $code = $this->applicationTester->run(array(
@@ -80,6 +86,22 @@ public function getInvalidArgumentsForTest()
         );
     }
 
+    public function test_Command_FailsWithMissingFilePermissionException_whenConfigFileNotWritable()
+    {
+        $this->makeLocalConfigNotWritable();
+
+        $code = $this->applicationTester->run(array(
+            'command' => 'config:set',
+            'assignment' => array(
+                'MySection.other_array_value=[]',
+            ),
+            '-vvv' => false,
+        ));
+
+        $this->assertNotEquals(0, $code, $this->getCommandDisplayOutputErrorMessage());
+        $this->assertContains('[Piwik\Exception\MissingFilePermissionException]', $this->applicationTester->getDisplay());
+    }
+
     public function test_Command_SucceedsWhenArgumentsUsed()
     {
         $config = Config::getInstance();
@@ -191,4 +213,23 @@ private static function removeTestConfigFile()
             unlink($configPath);
         }
     }
+
+    protected function makeLocalConfigNotWritable()
+    {
+        $local = Config::getInstance()->getLocalPath();
+        touch($local);
+        chmod($local, 0444);
+        $this->assertFalse(is_writable($local));
+    }
+
+    protected function makeLocalConfigWritable()
+    {
+        $local = Config::getInstance()->getLocalPath();
+        @chmod(dirname($local), 0755);
+        @chmod($local, 0755);
+        $this->assertTrue(is_writable(dirname($local)));
+        if(file_exists($local)) {
+            $this->assertTrue(is_writable($local));
+        }
+    }
 }

tests/PHPUnit/Integration/UpdaterTest.php

@@ -18,6 +18,21 @@
  */
 class UpdaterTest extends IntegrationTestCase
 {
+    public function test_doUpdate_reportsAnError_whenMissingFilePermissionException()
+    {
+        $updater = new Updater($pathToCoreUpdates = null, PIWIK_INCLUDE_PATH . '/tests/resources/Updater/%s/');
+        $updater->markComponentSuccessfullyUpdated('testpluginUpdates', '0.4');
+        $componentsWithUpdateFile = $updater->getComponentsWithUpdateFile(array('testpluginUpdates' => '0.5'));
+
+        $this->assertEquals(1, count($componentsWithUpdateFile));
+
+        $result = $updater->updateComponents($componentsWithUpdateFile);
+
+        $this->assertTrue( count ($result['errors']) > 0, 'when an update fails because config file is not writable, we expect the updater to report a critical error');
+        $this->assertEquals( 'make sure this exception is thrown', $result['errors'][0]);
+    }
+
+
     public function testUpdaterChecksCoreVersionAndDetectsUpdateFile()
     {
         $updater = new Updater(PIWIK_INCLUDE_PATH . '/tests/resources/Updater/core/');

tests/resources/Updater/testpluginUpdates/0.5.php

@@ -0,0 +1,12 @@
+<?php
+namespace Piwik\Plugins\testpluginUpdates;
+
+use Piwik\Updates as PiwikUpdates;
+
+class Updates_0_5 extends PiwikUpdates
+{
+    function doUpdate(\Piwik\Updater $updater)
+    {
+        throw new \Piwik\Exception\MissingFilePermissionException('make sure this exception is thrown');
+    }
+}