[3.0] Records what release-2.1 means, and that a removed check may be deliberate - #9538
Merged
Sesquipedalian merged 1 commit intoAug 19, 2026
Conversation
Both from the review on SimpleMachines#9535, and both cases where the wrong conclusion looks perfectly reasonable from inside the repository. Checking whether a name needs a compatibility shim by looking at the release-2.1 branch finds things that were added to it last month and have never shipped. A mod can only call what was in a release, so the tags are the thing to check, not the branch. And a check that has been taken out is not automatically a regression. FtpConnection::passive() had grown one that reads exactly like SSRF protection, and would be, if FtpConnection fetched from the open web. It does not: it is how the Package Manager reaches an FTP server the admin nominated, which is usually on the LAN, so the check broke the ordinary case. That belongs on FtpFetcher instead. Quotes the review in both places, the way the bug reporting section already quotes SimpleMachines#9520. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: albertlast <mathiaspapealbert@hotmail.com>
Sesquipedalian
approved these changes
Aug 19, 2026
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
3 participants
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.
Description
@Sesquipedalian left a comment on #9535 addressed "FYI for the AI", correcting two things
I had argued for. Both corrections are worth keeping, because in both cases the wrong
conclusion looks perfectly reasonable from inside the repository — so the next agent will
reach it too unless it is written down.
release-2.1is not the same thing as released 2.1.I checked whether
make_fetch_safe()needed preserving inSubs-Compat.phpby looking forit on the
release-2.1branch, found it, and concluded it was 2.1 public API. It is not —it was added to the branch on 2026-08-04 and has never shipped:
Subs-Compat.phpexists to keep working the names that mods actually call, and a mod canonly call what was in a release. So the note says to check the tags, not the branch, and
gives the two commands that do it.
A removed check is not automatically a regression.
FtpConnection::passive()had a check that the PASV address was globally routable, andthis PR removes it. I read that as a dropped SSRF guard, since that is exactly what it
looks like. But
FtpConnectionis how the Package Manager reaches an FTP server the adminnominated, which is commonly on the LAN or on localhost — so the check broke the ordinary
case, and it belongs on
FtpFetcher, which does fetch from the open web.The generalisable part, and the reason it is worth a note rather than just a fix: a class
that fetches from the open web and a class that talks to infrastructure the admin
configured want opposite defaults. Working out which one you are looking at comes before
arguing a guard back in.
Both entries quote the review verbatim, the way the bug-reporting section already quotes
#9520.
Testing
Documentation only — no code changes, nothing to run. LF endings, no trailing whitespace.
Note for whoever merges: #9511 also touches
AGENTS.md, in a different section (itdocuments when the unit suite can cover a change). The two should not collide, but they
have not been merged together.
Issues References (Fixes|Related|Closes)