fix: resolve translation keys, and fall back to http-post when https-post is rejected - #8
Conversation
Minz_Translate::resolveKey() splits a key on every dot and descends one array
level per segment, so a literal 'callback.help' key can never be reached: the
lookup enters 'callback', finds a string, and gives up. Keys that are also
parents need their own value under '_', which is how core's i18n files are
written -- and why core spells one of its own keys token_help with an
underscore.
26 of the 35 keys here were unreachable and rendered as their raw key. That
included every .help string and callback.invalid / callback.private, all of
which predate the status table by a long way; the table only made it obvious,
because there you read the value rather than a label you can infer from
context.
Verified by replicating resolveKey()'s algorithm against the file: all 35 keys
the configuration screen asks for now resolve, including the parent-and-child
pairs status ("Subscriptions"), status.kind ("Type") and status.kind.opml.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The protocol parameter of pleaseNotify names the notification method -- http-post for REST, as against xml-rpc or soap -- not the scheme of the callback, which is carried by port. Servers disagree: some accept https-post as a TLS-flavoured spelling, others take only the value the specification lists. An HTTPS instance therefore cannot assume either. Registering with https://www.valedalama.net/feed/ was answered with "Only the http-post (REST) protocol is supported by this endpoint." That feed advertises port="443" protocol="http-post", which is the convention this extension already applies when reading a <cloud> element -- we consumed it that way and produced something else. The preferred value is now tried first and plain http-post kept as a fallback, and the value a server accepts is recorded so later renewals go straight to it rather than failing the same way every time. https-post stays the preference: it is the path confirmed working against rpc.rsscloud.io, and flipping the default would need evidence about that server this change does not have. Only a server that actually answered can be objecting to the protocol, so a negative status -- FreshRSS-internal, meaning the request never arrived -- does not trigger the fallback, and a first attempt that will be retried logs at debug rather than warning, so a subscription that recovers does not look broken. remember() forgets the recorded value when the endpoint moves, since a different server need not accept what the old one did. Existing state files simply have no recorded value and rediscover it on the next renewal, so nothing needs migrating. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 34 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe registry now stores protocol state. The subscriber negotiates protocol candidates, retries failed notifications, persists outcomes, and uses nested RSS Cloud translations. Tests cover state and candidate selection. ChangesRSS Cloud protocol negotiation
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant RssCloud_Subscriber
participant RssCloud_Registry
participant rssCloud_server
RssCloud_Subscriber->>RssCloud_Registry: Load remembered protocol
RssCloud_Subscriber->>RssCloud_Subscriber: Build protocolCandidates()
RssCloud_Subscriber->>rssCloud_server: pleaseNotify with candidate protocol
rssCloud_server-->>RssCloud_Subscriber: Accept or reject request
RssCloud_Subscriber->>RssCloud_Registry: Persist accepted protocol or final error
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@RssCloud/Registry.php`:
- Around line 85-86: Update the protocol normalization in the Registry
state-loading logic to preserve only the recognized values “http-post” and
“https-post”; map missing, non-string, or any other stored value to an empty
string so unknown protocols cannot become the first pleaseNotify candidate.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1bd673a0-710b-437c-9c88-1ff6a53b97f4
📒 Files selected for processing (6)
RssCloud/Registry.phpRssCloud/Subscriber.phpi18n/en/ext.phptests/RssCloud/RegistryTest.phptests/RssCloud/SubscriberTest.phptests/bootstrap.php
normalise() treats the state file as entirely untrusted, and every other field is bounded accordingly -- `kind` was tightened to its two known values for this same reason. `protocol` was the one left accepting any string. Whatever is stored there is tried first, so a hand-edited or corrupted file could put an arbitrary value at the head of the candidate list and advertise it to a cloud server ahead of one known to work. In-process writers were already safe, since protocolCandidates() only ever returns the two constants; this bounds the path that does not come from us. Raised by CodeRabbit on #8. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 participant
Two independent defects that surfaced in the same screenshot. Split into a commit each, since the
i18n one predates the status table entirely.
1. Translation keys never resolved (
e21b6cc)Minz_Translate::resolveKey()splits a key on every dot and descends one array level persegment, so a literal
'callback.help'key is unreachable — the lookup enters'callback', finds astring, and gives up. Keys that are also parents need their value under
'_'. This is why corespells one of its own keys
token_helpwith an underscore.26 of the 35 keys here were unreachable and rendered as their raw key: every
.helpstring,plus
callback.invalidandcallback.private. All of that predates the status table by a long way;the table only made it visible, because there you read the value rather than a label you can infer
from context (
ext.rsscloud.status.errorwhere "Failed" belonged).Rewritten in core's nested form. Verified by replicating
resolveKey()'s algorithm against thefile: all 35 keys the configuration screen asks for now resolve, including the parent-and-child
pairs
status("Subscriptions"),status.kind("Type") andstatus.kind.opml.2.
pleaseNotifyprotocol fallback (a4293d1)The
protocolparameter names the notification method —http-postfor REST, as againstxml-rpcorsoap— not the scheme of the callback, which is carried byport. We were sendinghttps-postpurely because the instance is TLS. Two real endpoints reject that outright:Both feeds advertise
port="443"/port="80"withprotocol="http-post"— the same convention thisextension already applies when reading a
<cloud>element. We consumed it one way and producedanother.
The preferred value is now tried first with plain
http-postas a fallback, and whichever theserver accepts is recorded so later renewals go straight to it.
https-postremains the preferencebecause it is the path confirmed working against rpc.rsscloud.io; flipping the default would need
evidence about that server this change does not have.
Only a server that actually answered can be objecting to the protocol, so a negative status —
FreshRSS-internal, meaning the request never arrived — does not trigger the fallback. A first
attempt that will be retried logs at
debugrather thanwarning, so a subscription that recoverson the fallback does not look broken in the log.
One thing a reviewer should know
This does not fix refarmer.blog on its own. Its endpoint is the WordPress rssCloud plugin
(v0.5.0, unmaintained since 2022), which cannot speak HTTPS to a subscriber at all:
notification-request.phpbuilds the callback asdomain:port/pathand unconditionally prependshttp://, with no code path producinghttps://. It then challengeshttp://host:443/…—plaintext against a TLS port — and gets a 400. Any HTTPS FreshRSS is unreachable from that plugin
until it is fixed upstream. This PR removes one of the two blockers.
Verification
php -lover php+phtml clean · PHPUnit 21 tests / 56 assertions · PHPStan level 10[OK] No errors· phpcs 13/13 clean. All re-run after reverting an earlier speculative change, not carried over.
RssCloud_Subscriber::protocolCandidates()is a pure static, so the candidate logic is covereddirectly without HTTP — including the invariant that
http-poststays reachable for everycombination of remembered and callback protocol.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation