feat: Make Feed Atom 1.0 valid - #83
Conversation
|
Warning Review limit reached
More reviews will be available in 52 minutes. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThis PR migrates the blog plugin's feed output from RSS 2.0 to Atom format. The HTTP Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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: 3
🤖 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 `@components/rssfeed/default.htm`:
- Around line 23-26: The current CDATA-wrapped output for the <summary
type="html"> and <content type="html" xml:base="{{ post.url }}"> blocks is
unsafe because literal "]]>" inside post.summary or post.content_html will
prematurely close the CDATA; change these to emit entity-escaped HTML instead of
CDATA: replace <![CDATA[{{ post.summary }}]]> with an escaped output of
post.summary (e.g. {{ post.summary | escape }} or the template engine's xml/html
escape filter) and replace the CDATA-wrapped post.content_html with an escaped
version (e.g. {{ post.content_html | escape }}), ensuring the feed remains
well-formed XML even if the content contains "]]>".
- Around line 15-17: Add a guard around the entry author and add a feed-level
author fallback to ensure the feed remains valid when post.user is missing: when
rendering each entry, only output the <author><name>{{ post.user.full_name
}}</name></author> block if post.user (or post.user.full_name) is present, and
add a top-level <author><name>Fallback Author Name</name></author> (or a
site-wide variable) inside the atom:feed metadata so the feed has an author even
if some entries lack one; locate the <name>{{ post.user.full_name }}</name>
usage in components/rssfeed/default.htm and update the template logic
accordingly.
- Line 3: In components/rssfeed/default.htm fix Atom author null handling by
guarding usage of post.user.full_name: wrap the <author><name> and <rights>
output in a conditional that checks post.user (or use a fallback like site owner
name) and omit the <author> element entirely when post.user is missing; for
post.summary and post.content_html avoid raw CDATA blocks that can be broken by
literal "]]>" — either XML-escape those fields instead of using CDATA or
implement a safe CDATA strategy (split/encode any "]]>" sequences) so
post.summary and post.content_html cannot break the feed.
🪄 Autofix (Beta)
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
Run ID: 9cd46c50-b792-4445-ac9c-8326d3e551f9
📒 Files selected for processing (2)
components/RssFeed.phpcomponents/rssfeed/default.htm
|
Any Updates in this? I saw the AI generated suggestions, but these don't seem too relevant, except maybe the CDATA termination, which could be an easy replace string fix, although you'd need admin access to mangle the data in any case, right? Do you want me to change anything, or what's your opinion on this PR? |
|
@nathanlesage can you provide more background on why this change is needed? RSS feeds seem to be more popular than atom. Perhaps this should be done as an option on the component or a separate component? Also we should probably add tests to make sure valid RSS & Atom are being generated, I tried using this on WinterCMS.com with the partial override and it generated invalid Atom because the links / post.url were outputting blank values. |
|
Hi Luke, sorry for the radio silence — it's currently quite busy here, and I'm still not out of the woods, but I wanted to give you a lifesign that I didn't forget this PR.
It should not be too difficult to make this configurable, and allow users to switch between RSS and Atom. That being said, the page that you shared clearly argues that RSS is mainly important if you need to maintain "maximum backward compatibility with very old systems". Atom has a lot of benefits that we can make use of in that it's a more thought-out protocol. I personally have been using quite a few RSS readers in my life, and all of those seem to have zero issues with Atom. The format has been around for quite some time, and the question is: Should we support software from before the millennium? I would argue that, if someone really needs to support such old software, people need to come up with their own solution. I doubt that the vast majority of Winter CMS users ever will need to come around to that. But of course, if you really believe that this is necessary, I can make this change and let people configure which of the formats they want to use.
Oh, absolutely. (EDIT: The blog plugin doesn't have a test suite right now, correct? If so, can I just follow this guide to implement some?)
Can you illuminate why the The question here really is: which fields of the post model can we expect to be set, and which ones are optional and not always present? I was already getting a bit confused that the author field can be empty since the UI in the backend doesn't seem to allow setting an empty author, so I want to make sure that there are default fallbacks for any optional fields. If you could say something about this, I could use that information to do another run over the PR so that it becomes mergeable. |
|
@nathanlesage yes, the guide on the WinterCMS.com documentation should work fine to help you setup tests. Additionally AI should be pretty good at generating tests. It would probably be easiest to just offer this as a separate AtomFeed component mirroring the functionality of the RSSFeed component but specifically made for Atom. |
2 participants
That was easier than expected -- this PR makes the Winter Blog generated feed Atom 1.0 valid. It does so by simply adjusting and modifying the feed template according to RFC 4287.
Changes
<![CDATA[]]>to ensure proper parsing by feed readersgeneratorto identify the plugin as the generator;rightsto indicate copyrightapplication/atom+xmlinstead oftext/xml)Additional Information
This PR is only the first of potentially more to improve feed experience. Additional fields that could be made configurable are:
icon(for a preview icon),logo(for a bigger logo type),subtitle(for a description/subtitle of the feed),categorys to denote an article's categories, and make various existing fields configurable.But since we can now at least ship valid Atom 1.0 feeds, I thought this is a minimal-effort improvement to the plugin itself, and users can still override the partial how they like (which is how I am currently inserting logos, icons, and subtitles).
Let me know if that looks for now good to you, or if I should do some changes to this particular improvement already.
Summary by CodeRabbit