Skip to content

fix(redirects): survive FreshRSS cores older than 1.29.2 - #4

Merged
andrewshell merged 3 commits into
mainfrom
fix/redirect-probe-older-freshrss
Aug 5, 2026
Merged

fix(redirects): survive FreshRSS cores older than 1.29.2#4
andrewshell merged 3 commits into
mainfrom
fix/redirect-probe-older-freshrss

Conversation

@andrewshell

@andrewshell andrewshell commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

The bug

Adding a feed 500s on every released FreshRSS:

PHP Fatal error:  Uncaught Error: Call to undefined method
FreshRSS_http_Util::getCurlResolveInfo() in RssCloud/Redirects.php:152

getCurlResolveInfo() was added to core on 2026-06-28 (FreshRSS/FreshRSS#8400) and is contained in
no tagged releasegit tag --contains on that commit is empty. The newest stable release is
1.29.1, tagged 2026-05-20, five weeks earlier, and that is what the freshrss/freshrss:latest
Docker image ships. So onFeedBeforeInsert fatals and takes the whole add-feed request with it.

The fix

Guard the call. Cores without the method probe without DNS pinning, which is the same exposure they
already carry for every feed fetch — their httpGet() did not pin DNS either. The alternative,
refusing to probe, would have silently disabled redirect resolution on every release, which is the
feature's whole point.

PHPStan calls the guard redundant whichever core it analyses, just from opposite directions
(alreadyNarrowedType against edge, impossibleType against 1.29.1). Both suppressions live in
phpstan.neon; reportUnmatchedIgnoredErrors: false was already set, so the inapplicable one
passes unmatched and a single config serves either core. Both are scoped to the one file and one
message, and marked for deletion along with the guard once 1.29.2 is the oldest supported release.

Why it shipped

ci.yml pinned the core checkout to ref: edge — the one core that has the method was the only
one ever analysed. It now runs a matrix over both ends of the supported range, edge and 1.29.0,
with fail-fast: false.

1.29.0 is the true floor: Minz_HookType::FeedsListBeforeActualize is the binding constraint. I
verified that rather than assuming it, since CI now depends on it.

Verification

Check Result
PHPStan @ edge / 1.29.2-dev [OK] No errors
PHPStan @ 1.29.1 (Docker latest) [OK] No errors
PHPStan @ 1.29.0 (declared floor) [OK] No errors
phpcs (core ruleset, extensions/ re-enabled) 10/10 files, clean
markdownlint-cli2 0.23.2 0 issues
php -l clean

Analysing 1.29.1 also confirmed the useful negative: getCurlResolveInfo was the only
incompatibility. No signature drift anywhere else — httpGet()'s positional args, checkUrl()'s
fixScheme: named arg and the hook callback shapes all still match.

Static analysis only. There is no test suite here, so nothing exercises an actual redirect
probe; this proves the call cannot fatal, not that the unpinned path behaves correctly at runtime.
permanentLocation() is the natural first candidate for coverage.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features

    • Added compatibility with FreshRSS 1.29.0 and newer.
    • Redirect checks now use DNS resolution pinning when supported, with fallback behavior for older FreshRSS versions.
  • Documentation

    • Added requirements and version-specific redirect behavior to the README.
  • Tests

    • Updated CI to test against FreshRSS edge and version 1.29.0.

andrewshell and others added 3 commits August 5, 2026 17:06
FreshRSS_http_Util::getCurlResolveInfo() was added to core after 1.29.1 and is
in no tagged release, so resolving a feed's permanent redirects raised a fatal
"call to undefined method" on every released FreshRSS, taking the whole
add-feed request down with a 500.

Guard the call. Cores without it probe without DNS pinning, which is the same
exposure they already carry for every feed fetch, since their httpGet() did not
pin DNS either -- the alternative, refusing to probe, would silently disable
redirect resolution on every release.

PHPStan calls the guard redundant whichever core it analyses, from opposite
directions, so phpstan.neon carries both suppressions; reportUnmatchedIgnored-
Errors lets the inapplicable one pass. Verified clean against edge, 1.29.1 and
1.29.0.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Pinning the core checkout to edge is how a call to an edge-only core method
reached users as a fatal error on every released FreshRSS: the one core that
had the method was the only one ever analysed.

Run a matrix over both ends of the supported range instead -- the development
tip and 1.29.0, the oldest release this extension supports. fail-fast is off so
a break on one core still reports the other.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The binding constraint is Minz_HookType::FeedsListBeforeActualize, which core
added in 1.29.0. Also records which behaviour degrades on cores predating
getCurlResolveInfo() rather than being required outright.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Aug 5, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

The change adds compatibility for FreshRSS versions without getCurlResolveInfo(). It documents FreshRSS version requirements and redirect behavior. CI now tests both edge and 1.29.0.

Changes

FreshRSS compatibility

Layer / File(s) Summary
Version-compatible redirect probing
RssCloud/Redirects.php, phpstan.neon
permanentLocation() uses DNS resolution pinning when getCurlResolveInfo() exists. It continues without pinning when the method is unavailable. PHPStan ignores both version-dependent type diagnostics.
Version matrix and requirements
.github/workflows/ci.yml, README.md
CI checks FreshRSS edge and 1.29.0. The README documents support from 1.29.0 and DNS-pinning behavior from 1.29.1.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related PRs

Poem

A rabbit hops through versions bright,
With pinned paths when methods are right.
If older FreshRSS hides the call,
The probe still runs without it all.
CI checks each path with care.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly identifies the redirect compatibility fix for FreshRSS cores older than 1.29.2.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/redirect-probe-older-freshrss

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
RssCloud/Redirects.php (1)

151-160: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy lift

Add runtime coverage for both FreshRSS API shapes.

The CI checks prove syntax and PHPStan compatibility. They do not execute method_exists(), validate the getCurlResolveInfo() return handling, or verify the CURLOPT_RESOLVE setup. Add runtime coverage for a core without the method and a core with it. Cover allowed arrays, null, false, and permanent redirect chains.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@RssCloud/Redirects.php` around lines 151 - 160, Add runtime tests for the
redirect resolution flow around getCurlResolveInfo(), covering cores both
without and with that method. Verify allowed array results configure
CURLOPT_RESOLVE, null and false results stop resolution, and permanent redirect
chains preserve the expected behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@RssCloud/Redirects.php`:
- Around line 151-160: Add runtime tests for the redirect resolution flow around
getCurlResolveInfo(), covering cores both without and with that method. Verify
allowed array results configure CURLOPT_RESOLVE, null and false results stop
resolution, and permanent redirect chains preserve the expected behavior.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 5866fd63-9551-44ec-bee5-6c335c306448

📥 Commits

Reviewing files that changed from the base of the PR and between e272ceb and 802b184.

📒 Files selected for processing (4)
  • .github/workflows/ci.yml
  • README.md
  • RssCloud/Redirects.php
  • phpstan.neon

@andrewshell
andrewshell merged commit d9c77c1 into main Aug 5, 2026
4 checks passed
@andrewshell
andrewshell deleted the fix/redirect-probe-older-freshrss branch August 5, 2026 22:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant