From d183b627c37d0ec4a4dcfec7b5d2a3e7ee0a62ac Mon Sep 17 00:00:00 2001 From: Tom Elliott Date: Sun, 30 Aug 2026 16:55:26 -0500 Subject: [PATCH] Retire the discovery-extension files an --oldcopy upgrade leaves behind configureHttpd()'s retirement sweep matched *.class.php only. GH-1528 retired 52 more files the same way -- every core page, hook, report and event moved from lib/{pages,hooks,reports,events}/..php to src//.php -- so on FOG_copy_back_old=yes all 52 survive the upgrade. They are not the harmless case the existing comment describes. A stale *.class.php is inert because autoload() answers a bare name out of src/ first. A stale *.report.php is FOUND: ReportManagement::loadCustomReports() merges core's src/Reports with the fileitems() walk that picks up plugin reports, and that walk reaches lib/reports/ too. Every core report is then discovered twice and the Reports menu renders each of them twice. Reproduced on a live 1.6 install rather than argued: restoring the 52 pre-GH-1528 files into a deployed webroot took the discovered report list from 17 entries to 30, 13 of them duplicates. Page and report rendering stayed green throughout, which is why this needed the discovery probe to see at all. The keep-if-still-shipped test is unchanged and is what keeps the wider match safe -- lib/router/ still ships its .class.php files and they are matched, tested and kept exactly as before. tests/oldcopy-retires-moved-classes.test.sh gains three checks: a retired file of each of the four kinds is removed, a discovery-extension file the release still ships is spared, and a bundled plugin's report one level deeper is left to the fog-plugins release. Both mutations fail it -- narrowing the find back to *.class.php, and dropping the keep test, which is the obvious wrong fix. Its extractor also had to change. It grabbed the loop out of functions.sh and stopped at the first `done < <(find ` line; the find is a multi-line -o list now, so that terminator would cut the snippet mid-command. It stops on -print0 instead. Co-Authored-By: Claude --- lib/common/functions.sh | 27 ++++++++++++- tests/oldcopy-retires-moved-classes.test.sh | 42 ++++++++++++++++++++- 2 files changed, 67 insertions(+), 2 deletions(-) diff --git a/lib/common/functions.sh b/lib/common/functions.sh index a7c412f4cb..55df896c46 100755 --- a/lib/common/functions.sh +++ b/lib/common/functions.sh @@ -10606,13 +10606,38 @@ configureHttpd() { # FOG_SCHEMA_INSTALL_TOKEN, sitting readable in the web root while a # different file is the one actually being used. Nothing reads it, # nothing reports it, and it survives every future upgrade. + # THE DISCOVERY EXTENSIONS ARE NOT OPTIONAL HERE, AND THEY ARE + # WORSE THAN THE .class.php CASE ABOVE. + # + # GH-1528 retired 52 more files the same way: every core page, + # hook, report and event moved from lib/{pages,hooks,reports, + # events}/..php to src//.php. A + # loop matching only *.class.php leaves all 52 behind. + # + # A stale .class.php is inert -- the comment above says why. A + # stale *.report.php is NOT. ReportManagement::loadCustomReports() + # merges core's src/Reports with the fileitems() walk that finds + # plugin reports, and the walk reaches lib/reports/ as well, so + # every core report is found TWICE and the Reports menu renders + # each of them twice. Measured on a 1.6 install upgraded this way: + # 17 reports became 30, 13 of them duplicates. + # + # So the sweep asks for the four discovery extensions alongside + # the class files. The keep-if-still-shipped test is unchanged and + # is what makes that safe: lib/router/ still ships its .class.php + # files and they are matched, tested and kept, exactly as before. dots "Removing retired class files from the old web folder" local relpath while IFS= read -r -d '' i; do relpath="${i#$webdirdest/}" [[ -e ${webdirsrc}/${relpath} ]] && continue rm -f "$i" >>$error_log 2>&1 - done < <(find "$webdirdest/lib" -maxdepth 2 -type f -name '*.class.php' -print0 2>>$error_log) + done < <(find "$webdirdest/lib" -maxdepth 2 -type f \( \ + -name '*.class.php' -o \ + -name '*.page.php' -o \ + -name '*.hook.php' -o \ + -name '*.report.php' -o \ + -name '*.event.php' \) -print0 2>>$error_log) errorStat $? fi fi diff --git a/tests/oldcopy-retires-moved-classes.test.sh b/tests/oldcopy-retires-moved-classes.test.sh index 012193a638..857d62aa80 100644 --- a/tests/oldcopy-retires-moved-classes.test.sh +++ b/tests/oldcopy-retires-moved-classes.test.sh @@ -79,10 +79,15 @@ fi # --------------------------------------------------------------------------- # The loop itself, lifted out of configureHttpd(). # --------------------------------------------------------------------------- +# Terminated on -print0 rather than on `done < <(find `: the find grew a +# multi-line \( -name ... -o -name ... \) list in GH-1531, and stopping at the +# first line would cut the snippet mid-command -- which fails as a SYNTAX +# ERROR in the eval below rather than as a wrong answer, but is exactly the +# kind of extraction that silently stops covering what it names. snippet=$(awk ' /^ local relpath$/ { grab = 1 } grab { print } - grab && /^ done < <\(find / { exit } + grab && /-print0 2>>\$error_log\)/ { exit } ' "$functions") if [[ -z $snippet ]]; then @@ -133,6 +138,29 @@ echo generated > "$webdirdest/commons/config.class.php" # A bundled plugin's own class file, one level deeper. echo plugin > "$webdirdest/lib/plugins/site/class/site.class.php" +# GH-1528 retired the four DISCOVERY extensions the same way: every core page, +# hook, report and event is src//.php now. These matter more +# than the class files above, which are inert once src/ answers first -- a +# stale *.report.php is FOUND by ReportManagement::loadCustomReports()'s +# plugin walk, so every core report appears in the menu twice. Reproduced on a +# 1.6 install: 17 reports became 30. +mkdir -p "$webdirdest/lib/pages" "$webdirdest/lib/hooks" \ + "$webdirdest/lib/reports" "$webdirdest/lib/events" \ + "$webdirsrc/lib/pages" "$webdirdest/lib/plugins/site/reports" +echo stale > "$webdirdest/lib/pages/hostmanagement.page.php" +echo stale > "$webdirdest/lib/hooks/bootitem.hook.php" +echo stale > "$webdirdest/lib/reports/audit_report.report.php" +echo stale > "$webdirdest/lib/events/hostlist.event.php" +# Still shipped under lib/, so the keep-if-present test must spare it. There is +# no such core file today, which is precisely why one is invented here: the +# obvious wrong fix deletes every discovery-extension file it finds. +: > "$webdirsrc/lib/pages/index.page.php" +echo kept > "$webdirdest/lib/pages/index.page.php" +# A bundled plugin's report, one level deeper than this loop's maxdepth. It +# belongs to the fog-plugins release, and deleting it would strip a report +# from an upgraded server. +echo plugin > "$webdirdest/lib/plugins/site/reports/site_report.report.php" + # Wrapped in a function, which is where it really runs (configureHttpd) and # what makes its `local` declaration legal. eval "fog_retire_class_files() { @@ -160,6 +188,18 @@ check "the generated commons/config.class.php survives" \ check "a bundled plugin's class file is not this loop's to delete" \ "$([[ -s $webdirdest/lib/plugins/site/class/site.class.php ]]; echo $?)" +check "a retired page/hook/report/event file is removed (GH-1528)" \ + "$([[ ! -e $webdirdest/lib/pages/hostmanagement.page.php \ + && ! -e $webdirdest/lib/hooks/bootitem.hook.php \ + && ! -e $webdirdest/lib/reports/audit_report.report.php \ + && ! -e $webdirdest/lib/events/hostlist.event.php ]]; echo $?)" + +check "a discovery-extension file the release still ships is left alone" \ + "$([[ -s $webdirdest/lib/pages/index.page.php ]]; echo $?)" + +check "a bundled plugin's report is not this loop's to delete" \ + "$([[ -s $webdirdest/lib/plugins/site/reports/site_report.report.php ]]; echo $?)" + # --------------------------------------------------------------------------- # The loop has to run on the RESTORED tree, before the new files are laid over # it. Run it after, and every retired file it was meant to delete has already