simplify copy/permission process + fix safe-upgrade check

Signed-off-by: Andy Miller <rhuk@mac.com>

system/src/Grav/Common/Upgrade/SafeUpgradeService.php

@@ -23,24 +23,18 @@ use RecursiveIteratorIterator;
 use FilesystemIterator;
 use function array_key_exists;
 use function basename;
-use function chgrp;
 use function chmod;
-use function chown;
 use function copy;
 use function count;
 use function dirname;
 use function file_get_contents;
 use function file_put_contents;
-use function filegroup;
-use function fileowner;
 use function glob;
 use function in_array;
 use function is_dir;
 use function is_file;
 use function json_decode;
 use function json_encode;
-use function lchgrp;
-use function lchown;
 use function preg_match;
 use function preg_replace;
 use function property_exists;
@@ -54,8 +48,6 @@ use function trim;
 use function uniqid;
 use function unlink;
 use function ltrim;
-use function posix_getgrgid;
-use function posix_getpwuid;
 use const GRAV_ROOT;
 use const GLOB_ONLYDIR;
 use const JSON_PRETTY_PRINT;
@@ -74,7 +66,7 @@ class SafeUpgradeService
      *
      * @var string
      */
-    public const IMPLEMENTATION_VERSION = '20251106'; // 2025-11-06 - Added preflight to Install.php
+    public const IMPLEMENTATION_VERSION = '20251109'; // 2025-11-09 - Simplified to match traditional upgrade
 
     /** @var string */
     private $rootPath;
@@ -100,8 +92,6 @@ class SafeUpgradeService
     ];
     /** @var callable|null */
     private $progressCallback = null;
-    /** @var int */
-    private $metadataWarningCount = 0;
 
     /**
      * @param array $options
@@ -428,7 +418,9 @@ class SafeUpgradeService
             }
 
             $destination = $targetBase . DIRECTORY_SEPARATOR . $entry;
-            $metadata = $this->captureEntryMeta($destination);
+
+            // Use the same simple approach as traditional upgrade:
+            // Delete old, copy new, let filesystem handle ownership
             $this->removeEntry($destination);
 
             if (is_link($source)) {
@@ -436,29 +428,25 @@ class SafeUpgradeService
                 if (!@symlink(readlink($source), $destination)) {
                     throw new RuntimeException(sprintf('Failed to replicate symlink "%s".', $source));
                 }
-                $this->applyEntryMeta($destination, $metadata);
-                continue;
             } elseif (is_dir($source)) {
                 Folder::create(dirname($destination));
-                Folder::rcopy($source, $destination, true);
+                // DON'T preserve permissions - let filesystem inherit from parent
-                $this->applyEntryMeta($destination, $metadata);
+                // This matches traditional upgrade behavior
-                continue;
+                Folder::rcopy($source, $destination, false);
+
+                // Set bin/ permissions like traditional upgrade does
+                if ($entry === 'bin') {
+                    $binFiles = glob($destination . DIRECTORY_SEPARATOR . '*') ?: [];
+                    foreach ($binFiles as $binFile) {
+                        @chmod($binFile, 0755);
+                    }
+                }
             } else {
                 Folder::create(dirname($destination));
                 if (!@copy($source, $destination)) {
                     throw new RuntimeException(sprintf('Failed to copy file "%s" to "%s".', $source, $destination));
                 }
-                $perm = @fileperms($source);
-                if ($perm !== false) {
-                    @chmod($destination, $perm & 0777);
             }
-                $mtime = @filemtime($source);
-                if ($mtime !== false) {
-                    @touch($destination, $mtime);
-                }
-            }
-
-            $this->applyEntryMeta($destination, $metadata);
         }
     }
 
@@ -471,172 +459,6 @@ class SafeUpgradeService
         }
     }
 
-    /**
-     * Capture ownership and permission data for an existing filesystem entry.
-     *
-     * @param string $path
-     * @return array<string, mixed>
-     */
-    private function captureEntryMeta(string $path): array
-    {
-        if (!file_exists($path) && !is_link($path)) {
-            return [];
-        }
-
-        $meta = [
-            'link' => is_link($path),
-        ];
-
-        $perms = @fileperms($path);
-        if ($perms !== false) {
-            $meta['perms'] = $perms & 0777;
-        }
-
-        $owner = @fileowner($path);
-        if ($owner !== false) {
-            $meta['owner'] = $owner;
-        }
-
-        $group = @filegroup($path);
-        if ($group !== false) {
-            $meta['group'] = $group;
-        }
-
-        return $meta;
-    }
-
-    /**
-     * Reapply ownership and permission data to a copied entry when possible.
-     *
-     * @param string $path
-     * @param array<string, mixed> $meta
-     * @return void
-     */
-    private function applyEntryMeta(string $path, array $meta): void
-    {
-        if (!$meta) {
-            return;
-        }
-
-        if (isset($meta['perms'])) {
-            $result = @chmod($path, (int) $meta['perms']);
-            if ($result === false) {
-                $this->logMetadataWarning('chmod', $path, $meta['perms']);
-            }
-        }
-
-        $isLink = !empty($meta['link']);
-
-        if (isset($meta['owner'])) {
-            $owner = $this->resolveOwner($meta['owner']);
-            $result = false;
-            if ($isLink && function_exists('lchown')) {
-                $result = @lchown($path, $owner);
-            } elseif (!$isLink && function_exists('chown')) {
-                $result = @chown($path, $owner);
-            }
-            if ($result === false) {
-                $this->logMetadataWarning('chown', $path, $owner);
-            }
-        }
-
-        if (isset($meta['group'])) {
-            $group = $this->resolveGroup($meta['group']);
-            $result = false;
-            if ($isLink && function_exists('lchgrp')) {
-                $result = @lchgrp($path, $group);
-            } elseif (!$isLink && function_exists('chgrp')) {
-                $result = @chgrp($path, $group);
-            }
-            if ($result === false) {
-                $this->logMetadataWarning('chgrp', $path, $group);
-            }
-        }
-    }
-
-    /**
-     * Log a warning when metadata operations fail.
-     *
-     * @param string $operation Operation that failed (chmod, chown, chgrp)
-     * @param string $path Path to the file/directory
-     * @param mixed $value Value that was attempted (permissions, owner, group)
-     * @return void
-     */
-    private function logMetadataWarning(string $operation, string $path, $value): void
-    {
-        $this->metadataWarningCount++;
-
-        // Try to get Grav logger if available
-        try {
-            $grav = Grav::instance();
-            if (isset($grav['log'])) {
-                $grav['log']->warning(sprintf(
-                    'Safe-upgrade: Failed to apply %s(%s, %s). File permissions/ownership may not be preserved correctly. ' .
-                    'This is usually not critical but may require manual permission fixes after upgrade.',
-                    $operation,
-                    $path,
-                    is_scalar($value) ? $value : gettype($value)
-                ));
-            }
-        } catch (\Throwable $e) {
-            // Silently continue if logging fails - don't break the upgrade
-        }
-    }
-
-    /**
-     * Get count of metadata warnings during upgrade.
-     *
-     * @return int
-     */
-    public function getMetadataWarningCount(): int
-    {
-        return $this->metadataWarningCount;
-    }
-
-    /**
-     * Resolve stored owner identifier to a format accepted by chown/lchown.
-     *
-     * @param int|string $owner
-     * @return int|string
-     */
-    private function resolveOwner($owner)
-    {
-        if (is_string($owner)) {
-            return $owner;
-        }
-
-        if (function_exists('posix_getpwuid')) {
-            $info = @posix_getpwuid((int) $owner);
-            if (is_array($info) && isset($info['name'])) {
-                return $info['name'];
-            }
-        }
-
-        return (int) $owner;
-    }
-
-    /**
-     * Resolve stored group identifier to a format accepted by chgrp/lchgrp.
-     *
-     * @param int|string $group
-     * @return int|string
-     */
-    private function resolveGroup($group)
-    {
-        if (is_string($group)) {
-            return $group;
-        }
-
-        if (function_exists('posix_getgrgid')) {
-            $info = @posix_getgrgid((int) $group);
-            if (is_array($info) && isset($info['name'])) {
-                return $info['name'];
-            }
-        }
-
-        return (int) $group;
-    }
-
     public function setProgressCallback(?callable $callback): self
     {
         $this->progressCallback = $callback;

system/src/Grav/Installer/Install.php

@@ -459,19 +459,7 @@ ERR;
      */
     private function shouldUseSafeUpgrade(): bool
     {
-        // CRITICAL: Check if class exists WITHOUT triggering autoloader
+        // Check static override first (for programmatic control)
-        // If not loaded yet, manually load the NEW one from this package
-        if (!class_exists('Grav\\Common\\Upgrade\\SafeUpgradeService', false)) {
-            // Class not loaded yet - try to load from NEW package
-            $serviceFile = $this->location . '/system/src/Grav/Common/Upgrade/SafeUpgradeService.php';
-            if (!file_exists($serviceFile)) {
-                return false; // SafeUpgradeService not available in this package
-            }
-            // Load the NEW SafeUpgradeService from this package
-            require_once $serviceFile;
-        }
-
-        // Check static override first
         if (null !== self::$forceSafeUpgrade) {
             return self::$forceSafeUpgrade;
         }
@@ -482,17 +470,60 @@ ERR;
             return $envValue === '1';
         }
 
-        // Check Grav config
+        // CRITICAL CHECK: Ensure current installation supports SafeUpgradeService
+        // This must be checked BEFORE reading config, because the NEW package's system.yaml
+        // has safe_upgrade:true as default, which would be returned even for old installations
+        $currentServiceFile = GRAV_ROOT . '/system/src/Grav/Common/Upgrade/SafeUpgradeService.php';
+        if (!file_exists($currentServiceFile)) {
+            // Current installation doesn't have SafeUpgradeService (upgrading from < 1.7.50)
+            // Use traditional upgrade method regardless of config
+            return false;
+        }
+
+        // PRIMARY CHECK: Check Grav config setting
+        // Only use safe-upgrade if explicitly enabled in user configuration
         try {
             $grav = Grav::instance();
             if ($grav && isset($grav['config'])) {
-                return (bool) $grav['config']->get('system.updates.safe_upgrade', true);
+                // IMPORTANT: Read from USER config only, not merged config
+                // This prevents the NEW package's default (true) from being used
+                $userConfigFile = USER_DIR . 'config/system.yaml';
+                if (file_exists($userConfigFile)) {
+                    $userConfig = \Grav\Common\Yaml::parseFile($userConfigFile);
+                    if (is_array($userConfig) && isset($userConfig['updates']['safe_upgrade'])) {
+                        // User has explicitly set this in their config
+                        return (bool) $userConfig['updates']['safe_upgrade'];
                     }
-        } catch (\Throwable $e) {
-            // Grav container may not be initialised yet, default to safe upgrade.
                 }
 
-        return true;
+                // Fallback: try reading from merged config
+                // This is safe now because we already verified current installation has SafeUpgradeService
+                $configValue = $grav['config']->get('system.updates.safe_upgrade');
+                if ($configValue !== null) {
+                    return (bool) $configValue;
+                }
+            }
+        } catch (\Throwable $e) {
+            // Grav container may not be initialised yet
+            // Fall through to default behavior
+        }
+
+        // FINAL STEP: Load SafeUpgradeService from NEW package if needed
+        // Only reached if current installation HAS SafeUpgradeService but config is not set
+        if (!class_exists('Grav\\Common\\Upgrade\\SafeUpgradeService', false)) {
+            // Class not loaded yet - try to load from NEW package
+            $serviceFile = $this->location . '/system/src/Grav/Common/Upgrade/SafeUpgradeService.php';
+            if (!file_exists($serviceFile)) {
+                return false; // SafeUpgradeService not available in this package
+            }
+            // Load the NEW SafeUpgradeService from this package
+            require_once $serviceFile;
+        }
+
+        // If we get here: current installation HAS SafeUpgradeService, but config is not explicitly set
+        // Default to FALSE (traditional upgrade) for safety
+        // Users must explicitly enable system.updates.safe_upgrade to use it
+        return false;
     }
 
     /**