Skip to content

fix(php-update): deduplicate old-version detection, complete multi-version config migration and cleanup - #226

Merged
PDowney merged 3 commits into
masterfrom
copilot/refactor-php-version-check
May 2, 2026
Merged

fix(php-update): deduplicate old-version detection, complete multi-version config migration and cleanup#226
PDowney merged 3 commits into
masterfrom
copilot/refactor-php-version-check

Conversation

Copilot AI commented May 2, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Software Version Updates

php-update.sh had several bugs when multiple old PHP versions are detected: only the first was used for config migration and cleanup, sed replacements could double-substitute, and OLD_PHP_VERS could silently contain duplicates.

Changed Versions

  • scripts/update/php-update.sh — multi-version detection, migration, and cleanup fixes

Version Diff

# 1. Deduplicate OLD_PHP_VERS population
+ declare -A _SEEN_OLD_PHP_VERS=()
  for ver in "${SUPPORTED_PHP_VERSIONS[@]}"; do
      if [[ "${ver}" != "${NEW_PHP_VER}" ]] && dpkg -l | grep -q "php${ver}-fpm"; then
+         if [[ -z "${_SEEN_OLD_PHP_VERS["${ver}"]+x}" ]]; then
              OLD_PHP_VERS+=("${ver}")
+             _SEEN_OLD_PHP_VERS["${ver}"]=1
+         fi
      fi
  done

# 2. Expose all detected versions via MIGRATION_SOURCE_PHP_VERS
  OLD_PHP_VER="${OLD_PHP_VERS[0]}"   # kept for backward compat
+ MIGRATION_SOURCE_PHP_VERS=("${OLD_PHP_VERS[@]}")

# 3. Config updates loop over all old versions; single sed -E avoids double-replacement
- sed -i "s|php${OLD_PHP_VER}-fpm|php${NEW_PHP_VER}-fpm|g" "$file"
- sed -i "s|php${OLD_PHP_VER}|php${NEW_PHP_VER}|g" "$file"
+ for _OLD_VER in "${MIGRATION_SOURCE_PHP_VERS[@]}"; do
+     sed -E -i "s|php${_OLD_VER}(-fpm)?|php${NEW_PHP_VER}\1|g" "$file"
+ done

# 4. Cleanup loops over all old versions
- systemctl stop "php${OLD_PHP_VER}-fpm" ...
- apt purge -y php${OLD_PHP_VER}* ...
+ for OLD_VER in "${OLD_PHP_VERS[@]}"; do
+     systemctl stop "php${OLD_VER}-fpm" ...
+     apt purge -y php${OLD_VER}* ...
+ done

Verification Checklist

  • Version numbers are correctly formatted
  • Version numbers are stable releases (not beta/RC)
  • No syntax errors in variables file

Notes

Affected config targets: /etc/nginx/globals/php-fpm.conf, all nginx sites-available configs, phpsysinfo.ini, and api.php. The OLD_PHP_VER scalar is preserved unchanged for any downstream scripts that depend on it.

Original prompt 
Please apply the following diffs and create a pull request.
Once the PR is ready, give it a title based on the messages of the fixes being applied.

[{"message":"The loop filters out the NEW_PHP_VER but doesn't verify that the detected old versions are actually different from each other. If SUPPORTED_PHP_VERSIONS contains duplicate entries, OLD_PHP_VERS could have duplicates, leading to redundant processing. Consider adding a check to ensure uniqueness or using associative arrays.","fixFiles":[{"filePath":"scripts/update/php-update.sh","diff":"diff --git a/scripts/update/php-update.sh b/scripts/update/php-update.sh\n--- a/scripts/update/php-update.sh\n+++ b/scripts/update/php-update.sh\n@@ -36,9 +36,13 @@\n \n # Auto-detect currently installed PHP-FPM version\n OLD_PHP_VERS=()\n+declare -A _SEEN_OLD_PHP_VERS=()\n for ver in \"${SUPPORTED_PHP_VERSIONS[@]}\"; do\n     if [[ \"${ver}\" != \"${NEW_PHP_VER}\" ]] && dpkg -l | grep -q \"php${ver}-fpm\"; then\n-        OLD_PHP_VERS+=(\"${ver}\")\n+        if [[ -z \"${_SEEN_OLD_PHP_VERS[\"${ver}\"]+x}\" ]]; then\n+            OLD_PHP_VERS+=(\"${ver}\")\n+            _SEEN_OLD_PHP_VERS[\"${ver}\"]=1\n+        fi\n     fi\n done\n \n"}]},{"message":"The script detects multiple old PHP versions but only uses the first one (OLD_PHP_VERS[0]) for migration. This could lead to incomplete configuration migration if multiple old versions exist. The comment acknowledges this limitation but doesn't address it. Consider iterating through all old versions or documenting why only the first version is sufficient.","fixFiles":[{"filePath":"scripts/update/php-update.sh","diff":"diff --git a/scripts/update/php-update.sh b/scripts/update/php-update.sh\n--- a/scripts/update/php-update.sh\n+++ b/scripts/update/php-update.sh\n@@ -53,19 +53,21 @@\n     fi\n fi\n \n-# Preserve existing variable name for downstream logic that expects a single source version.\n+# Keep backward-compatible single-version variable for legacy downstream logic.\n+# Migration logic must use MIGRATION_SOURCE_PHP_VERS to ensure all detected old versions are handled.\n OLD_PHP_VER=\"${OLD_PHP_VERS[0]}\"\n+MIGRATION_SOURCE_PHP_VERS=(\"${OLD_PHP_VERS[@]}\")\n \n echo \"\"\n echo \"=============================================================\"\n echo \"\"\n-echo \"PHP Upgrade: Migrating from PHP ${OLD_PHP_VER} to PHP ${NEW_PHP_VER}\"\n+echo \"PHP Upgrade: Migrating to PHP ${NEW_PHP_VER} from detected old version(s): ${MIGRATION_SOURCE_PHP_VERS[*]}\"\n echo \"\"\n echo \"=============================================================\"\n echo \"\"\n \n-echo \"Detected PHP installation(s): ${OLD_PHP_VERS[*]}\"\n-echo \"Proceeding with upgrade to PHP ${NEW_PHP_VER} (primary source version: ${OLD_PHP_VER})...\"\n+echo \"Detected PHP installation(s): ${MIGRATION_SOURCE_PHP_VERS[*]}\"\n+echo \"Proceeding with upgrade to PHP ${NEW_PHP_VER}...\"\n \n # Stop old PHP service\n echo \"Stopping PHP ${OLD_PHP_VER} service...\"\n"}]},{"message":"The sed replacement on line 131 will also replace the occurrences already modified by line 130, potentially causing unintended substitutions. For example, 'php8.1-fpm' becomes 'php8.3-fpm' on line 130, then line 131 might match 'php8.1' within other contexts. The second sed command should use a more specific pattern to avoid double-replacement issues, or the replacements should be consolidated into a single command.","fixFiles":[{"filePath":"scripts/update/php-update.sh","diff":"diff --git a/scripts/update/php-update.sh b/scripts/update/php-update.sh\n--- a/scripts/update/php-update.sh\n+++ b/scripts/update/php-update.sh\n@@ -127,15 +127,13 @@\n \n # Update php-fpm.conf\n if [[ -f \"/etc/nginx/globals/php-fpm.conf\" ]]; then\n-    sed -i \"s|php${OLD_PHP_VER}-fpm|php${NEW_PHP_VER}-fpm|g\" \"/etc/nginx/globals/php-fpm.conf\"\n-    sed -i \"s|php${OLD_PHP_VER}|php${NEW_PHP_VER}|g\" \"/etc/nginx/globals/php-fpm.conf\"\n+    sed -E -i \"s|php${OLD_PHP_VER}(-fpm)?|php${NEW_PHP_VER}\\1|g\" \"/etc/nginx/globals/php-fpm.conf\"\n fi\n \n # Update all nginx site configurations\n for config_file in /etc/nginx/sites-available/*; do\n     if [[ -f \"$config_file\" ]]; then\n-        sed -i \"s|php${OLD_PHP_VER}-fpm|php${NEW_PHP_VER}-fpm|g\" \"$config_file\"\n-        sed -i \"s|php${OLD_PHP_VER}|php${NEW_PHP_VER}|g\" \"$config_file\"\n+        sed -E -i \"s|php${OLD_PHP_VER}(-fpm)?|php${NEW_PHP_VER}\\1|g\" \"$config_file\"\n     fi\n done\n \n"}]},{"message":"Same issue as Comment 4: the second sed command on line 138 may re-match and incorrectly replace strings already modified by line 137. This can lead to malformed configuration. Consolidate the sed patterns or ensure they don't overlap to prevent double replacements.","fixFiles":[{"filePath":"scripts/update/php-update.sh","diff":"diff --git a/scripts/update/php-update.sh b/scripts/update/php-update.sh\n--- a/scripts/update/php-update.sh\n+++ b/scripts/update/php-update.sh\n@@ -134,8 +134,10 @@\n # Update all nginx site configurations\n...

@PDowney
fix(php-update): deduplicate old-version detection, add MIGRATION_SOU…
dc189e2 
…RCE_PHP_VERS, fix double-sed replacements, loop cleanup over all old versions

Agent-Logs-Url: https://github.com/EngineScript/EngineScript/sessions/80d3a537-5c8d-4cfa-9c4b-49e7ddc7a580

Co-authored-by: PDowney <11467177+PDowney@users.noreply.github.com>
Copilot AI changed the title [WIP] Fix duplicate version detection in PHP update script fix(php-update): deduplicate old-version detection, complete multi-version config migration and cleanup May 2, 2026
Copilot finished work on behalf of PDowney May 2, 2026 03:15
Copilot AI requested a review from PDowney May 2, 2026 03:15
@PDowney
Remove PHP update entry from CHANGELOG
7568443 
Removed the entry for the PHP update on multi-version detection and cleanup fixes from the changelog.
@github-actions

github-actions Bot commented May 2, 2026

Copy link
Copy Markdown

Dependency Review

✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.

Scanned Files

None

@sonarqubecloud

sonarqubecloud Bot commented May 2, 2026

Copy link
Copy Markdown

Quality Gate Passed Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

@PDowney
PDowney marked this pull request as ready for review May 2, 2026 03:17
Copilot AI review requested due to automatic review settings May 2, 2026 03:17
@PDowney
PDowney merged commit 3000323 into master May 2, 2026
11 of 12 checks passed
@github-actions
github-actions Bot deleted the copilot/refactor-php-version-check branch May 2, 2026 03:18
Copilot AI reviewed May 2, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR improves php-update.sh’s handling of environments where multiple older PHP-FPM versions are installed, ensuring configuration migration and cleanup are applied across all detected old versions.

Changes:

  • Deduplicates detected old PHP versions during auto-detection.
  • Introduces MIGRATION_SOURCE_PHP_VERS and updates config migration to iterate over all detected old versions.
  • Updates removal/cleanup logic to loop over all detected old versions instead of only the first.
Comments suppressed due to low confidence (1)

scripts/update/php-update.sh:79

  • With multiple old PHP versions installed, the script still stops only the first detected version (OLD_PHP_VERS[0]). This can leave the actually active php-fpm service running during the upgrade, and the log message implies all old PHP has been stopped. Consider stopping/disabling all versions in OLD_PHP_VERS (or whichever versions are active) before installing/reloading Nginx to make behavior consistent with the new multi-version migration/cleanup logic.
# Stop old PHP service
echo "Stopping PHP ${OLD_PHP_VER} service..."
systemctl stop "php${OLD_PHP_VER}-fpm" 2>/dev/null || true

Comment thread scripts/update/php-update.sh
Comment on lines +60 to +63
# Keep backward-compatible single-version variable for legacy downstream logic.
# Migration logic must use MIGRATION_SOURCE_PHP_VERS to ensure all detected old versions are handled.
OLD_PHP_VER="${OLD_PHP_VERS[0]}"
MIGRATION_SOURCE_PHP_VERS=("${OLD_PHP_VERS[@]}")
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot
Copilot review effort, defaults to Lite
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments
+1 more reviewer
@PDowney PDowney PDowney approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@PDowney PDowney

Copilot code review Copilot

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@PDowney