Retire the discovery-extension files an --oldcopy upgrade leaves behind - #1531
Merged
Merged
Conversation
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}/<lowercase>.<type>.php to src/<Bucket>/<Class>.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 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #1528, found while deploying it to a live server.
configureHttpd()'s retirement sweep matched*.class.phponly. #1528 retired52 more files the same way — every core page, hook, report and event moved from
lib/{pages,hooks,reports,events}/<lowercase>.<type>.phptosrc/<Bucket>/<Class>.php— so onFOG_copy_back_old=yesall 52 survive theupgrade.
Why this is not the harmless case the existing comment describes
A stale
*.class.phpis inert:autoload()answers a bare name out ofsrc/before it consults the scanned class map, so those files are clutter on the
include path and nothing more. That is what the comment in place today says,
and it is true.
A stale
*.report.phpis found.ReportManagement::loadCustomReports()merges core's
src/Reportswith thefileitems()walk that picks up pluginreports, and that walk reaches
lib/reports/too. Every core report isdiscovered twice, and the Reports menu renders each of them twice.
Reproduced, not argued
Restoring the 52 pre-#1528 files into a deployed 1.6 webroot and re-running the
discovery probe:
Worth noting what did not catch it: a full page-and-report HTTP sweep of the
same server — 30 page nodes and all 17 reports, asserting status, body size and
absence of a PHP error — stayed at
52 passed, 0 failedwith the stale files inplace. The duplicate is in the menu, and every duplicate link works. Only the
probe that reports what discovery actually found could see it.
The fix
The
findasks for the four discovery extensions alongside the class files.The keep-if-still-shipped test is unchanged, and is what makes the wider match
safe:
lib/router/still ships its.class.phpfiles and they are matched,tested and kept exactly as before.
-maxdepth 2still holds the bundled-pluginboundary at
lib/plugins/<name>/<dir>/.Gates
tests/oldcopy-retires-moved-classes.test.shgains three checks — a retiredfile 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, run rather than assumed:
findback to*.class.phpFAIL a retired page/hook/report/event file is removedFAIL … still ships is left alone, twiceIts extractor also had to change. It lifted the loop out of
functions.shandstopped at the first
done < <(findline; thefindis a multi-line-olistnow, so that terminator would cut the snippet mid-command. It stops on
-print0instead.
Suite: 239 passed, 0 failed.
🤖 Generated with Claude Code
https://claude.ai/code/session_01VTEiBnDpNrgzu1tSqt1318