Allow setup:upgrade and setup:db-data:upgrade to run with a read-only app/etc (#26292)

lib/internal/Magento/Framework/Setup/FilePermissions.php

@@ -282,12 +282,14 @@ public function getMissingWritablePathsForInstallation($associative = false)
     /**
      * Checks writable paths for database upgrade, returns array of directory paths that requires write permission
      *
+     * Database upgrade only writes to var/. Deployment configuration (app/etc) is not touched, so it may stay
+     * read-only, which is the expected state for immutable/read-only deployments.
+     *
      * @return array List of directories that requires write permission for database upgrade
      */
     public function getMissingWritableDirectoriesForDbUpgrade()
     {
         $writableDirectories = [
-            DirectoryList::CONFIG,
             DirectoryList::VAR_DIR
         ];
 
@@ -309,6 +311,7 @@ public function getMissingWritableDirectoriesForDbUpgrade()
      *
      * @deprecated 100.1.0 Use getMissingWritablePathsForInstallation()
      * to get all missing writable paths required for install.
+     * @see getMissingWritablePathsForInstallation()
      * @return array
      */
     public function getMissingWritableDirectoriesForInstallation()

lib/internal/Magento/Framework/Setup/Test/Unit/FilePermissionsTest.php

@@ -248,14 +248,47 @@ public function testGetMissingWritableDirectoriesForDbUpgrade(): void
     {
         $directoryMethods = ['isExist', 'isDirectory', 'isReadable', 'isWritable'];
         foreach ($directoryMethods as $method) {
-            $this->directoryWriteMock->expects($this->exactly(2))
+            $this->directoryWriteMock->expects($this->once())
                 ->method($method)
                 ->willReturn(true);
         }
 
         $this->assertEmpty($this->filePermissions->getMissingWritableDirectoriesForDbUpgrade());
     }
 
+    /**
+     * Database upgrade must not require a writable app/etc (read-only deployments keep config.php and env.php
+     * read-only).
+     *
+     * @return void
+     */
+    public function testGetMissingWritableDirectoriesForDbUpgradeDoesNotRequireWritableConfigDirectory(): void
+    {
+        $readOnlyConfigDirectory = $this->createMock(Write::class);
+        $readOnlyConfigDirectory->method('isExist')->willReturn(true);
+        $readOnlyConfigDirectory->method('isDirectory')->willReturn(true);
+        $readOnlyConfigDirectory->method('isReadable')->willReturn(true);
+        $readOnlyConfigDirectory->method('isWritable')->willReturn(false);
+
+        $this->directoryWriteMock->method('isExist')->willReturn(true);
+        $this->directoryWriteMock->method('isDirectory')->willReturn(true);
+        $this->directoryWriteMock->method('isReadable')->willReturn(true);
+        $this->directoryWriteMock->method('isWritable')->willReturn(true);
+
+        $filesystemMock = $this->createMock(Filesystem::class);
+        $filesystemMock->method('getDirectoryWrite')
+            ->willReturnCallback(
+                fn (string $code) => $code === DirectoryList::CONFIG
+                    ? $readOnlyConfigDirectory
+                    : $this->directoryWriteMock
+            );
+        $this->directoryListMock->expects($this->never())->method('getPath');
+
+        $filePermissions = new FilePermissions($filesystemMock, $this->directoryListMock, $this->stateMock);
+
+        $this->assertEmpty($filePermissions->getMissingWritableDirectoriesForDbUpgrade());
+    }
+
     /**
      * @param array $mockMethods
      * @param array $expected

setup/src/Magento/Setup/Model/Installer.php

@@ -503,7 +503,7 @@ private function createModulesConfig($request, $dryRun = false)
                 $result[$module] = 1;
             }
         }
-        if (!$dryRun) {
+        if (!$dryRun && $result !== $currentModules) {
             $this->deploymentConfigWriter->saveConfig([ConfigFilePool::APP_CONFIG => ['modules' => $result]], true);
         }
         return $result;

setup/src/Magento/Setup/Test/Unit/Model/InstallerTest.php

@@ -68,7 +68,7 @@
     use Magento\Setup\Validator\DbValidator;
     use PHPUnit\Framework\MockObject\MockObject;
     use PHPUnit\Framework\TestCase;
-use PHPUnit\Framework\Attributes\DataProvider;
+    use PHPUnit\Framework\Attributes\DataProvider;
     use ReflectionException;
 
     /**
@@ -355,7 +355,7 @@ private function createObject($connectionFactory = false, $objectManagerProvider
          * @param array $logMetaMessages
          * @SuppressWarnings(PHPMD.ExcessiveMethodLength)
          */
-    #[DataProvider('installDataProvider')]
+        #[DataProvider('installDataProvider')]
         public function testInstall(array $request, array $logMessages, array $logMetaMessages)
         {
             $this->moduleList->method('getOne')
@@ -680,7 +680,7 @@ public static function installDataProvider()
          * @throws LocalizedException
          * @SuppressWarnings(PHPMD.ExcessiveMethodLength)
          */
-    #[DataProvider('installWithOrderIncrementPrefixDataProvider')]
+        #[DataProvider('installWithOrderIncrementPrefixDataProvider')]
         public function testInstallWithOrderIncrementPrefix(array $request, array $logMessages, array $logMetaMessages)
         {
             $this->moduleList->method('getOne')
@@ -950,7 +950,7 @@ public static function installWithOrderIncrementPrefixDataProvider(): array
          * @throws \Magento\Framework\Exception\RuntimeException
          * @SuppressWarnings(PHPMD.ExcessiveMethodLength)
          */
-    #[DataProvider('installWithInvalidRemoteStorageConfigurationDataProvider')]
+        #[DataProvider('installWithInvalidRemoteStorageConfigurationDataProvider')]
         public function testInstallWithInvalidRemoteStorageConfiguration(bool $isDeploymentConfigWritable)
         {
             $request = self::$request;
@@ -1369,7 +1369,7 @@ public function testInstallWithUnresolvableRemoteStorageValidator()
          * @throws \Magento\Framework\Exception\RuntimeException
          * @SuppressWarnings(PHPMD.ExcessiveMethodLength)
          */
-    #[DataProvider('installWithInvalidRemoteStorageConfigurationWithEarlyExceptionDataProvider')]
+        #[DataProvider('installWithInvalidRemoteStorageConfigurationWithEarlyExceptionDataProvider')]
         public function testInstallWithInvalidRemoteStorageConfigurationWithEarlyException(\Exception $exception)
         {
             $request = self::$request;
@@ -1694,6 +1694,37 @@ public function testUpdateModulesSequenceKeepGenerated()
             $installer->updateModulesSequence(true);
         }
 
+        public function testUpdateModulesSequenceSkipsConfigWriteWhenModulesUnchanged(): void
+        {
+            $this->cleanupFiles->expects($this->never())->method('clearCodeGeneratedClasses');
+            $installer = $this->prepareForUpdateModulesTestsWithCurrentModules(
+                ['Foo_One' => 1, 'Bar_Two' => 0, 'New_Module' => 1]
+            );
+            $this->configWriter->expects($this->never())->method('saveConfig');
+
+            $installer->updateModulesSequence(true);
+        }
+
+        public function testUpdateModulesSequenceWritesConfigWhenModuleOrderChanged(): void
+        {
+            $this->cleanupFiles->expects($this->never())->method('clearCodeGeneratedClasses');
+            $installer = $this->prepareForUpdateModulesTestsWithCurrentModules(
+                ['Bar_Two' => 0, 'Foo_One' => 1, 'New_Module' => 1]
+            );
+            $this->configWriter->expects($this->once())
+                ->method('saveConfig')
+                ->with(
+                    [
+                        ConfigFilePool::APP_CONFIG => [
+                            'modules' => ['Foo_One' => 1, 'Bar_Two' => 0, 'New_Module' => 1]
+                        ]
+                    ],
+                    true
+                );
+
+            $installer->updateModulesSequence(true);
+        }
+
         /**
          * @return void
          * @SuppressWarnings(PHPMD.CyclomaticComplexity)
@@ -1853,6 +1884,34 @@ private function prepareForUpdateModulesTests()
             return $newObject;
         }
 
+        /**
+         * Prepare mocks for update modules tests with the given `modules` section already present in config.php
+         *
+         * @param array $currentModules
+         * @return Installer
+         */
+        private function prepareForUpdateModulesTestsWithCurrentModules(array $currentModules): Installer
+        {
+            $cacheManager = $this->createMock(Manager::class);
+            $cacheManager->expects($this->once())->method('getAvailableTypes')->willReturn(['foo', 'bar']);
+            $cacheManager->expects($this->once())->method('clean');
+            $this->objectManager->expects($this->any())
+                ->method('get')
+                ->willReturnMap([[Manager::class, $cacheManager]]);
+            $this->moduleLoader->expects($this->once())
+                ->method('load')
+                ->willReturn(['Foo_One' => [], 'Bar_Two' => [], 'New_Module' => []]);
+            $this->config->expects($this->atLeastOnce())
+                ->method('get')
+                ->with(ConfigOptionsListConstants::KEY_MODULES)
+                ->willReturn(true);
+            $this->configReader->expects($this->once())
+                ->method('load')
+                ->willReturn(['modules' => $currentModules]);
+
+            return $this->createObject(false, false);
+        }
+
         /**
          * Sets a new ModuleResource object to the installer
          *