Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix Atom & RSS id fields and incorrect use of slug #10

Merged
merged 4 commits into from
Feb 1, 2023
Merged

Fix Atom & RSS id fields and incorrect use of slug #10

merged 4 commits into from
Feb 1, 2023

Conversation

n-peugnet
Copy link

@n-peugnet n-peugnet commented Dec 29, 2022 •
edited
Loading

This patch fixes the misuse of atom IDs throughout the feed.

In Atom, the IDs of feeds and entries should not change, otherwise the aggregator will treat the renamed entity as another one.

The IDs of feeds and entries must indeed be some sort of URI, so using an URL is convenient. But using the slug URL is not a good idea as it can change if the discussion is renamed. So now it uses URLs without the slugs for each feed and entries.
This is done by explicitly set the ID as another field than the link.

This change also fixes the link property of atom/d/<ID> feeds which had the ID doubled.

Here is an exemple diff of the changes this produces:

diff -u --color before/d-4.xml after/d-4.xml
--- before/d-4.xml	2022-12-29 16:27:48.023063424 +0100
+++ after/d-4.xml	2022-12-29 16:24:51.225833370 +0100
@@ -5,14 +5,14 @@
     <title><![CDATA[Coucou bis]]></title>
     <subtitle><![CDATA[Last messages in this discussion]]></subtitle>
     <link href="http://localhost:8080/atom/d/4" rel="self" />
-    <link href="http://localhost:8080/d/4-4-coucou-bis/" />
-    <id><![CDATA[http://localhost:8080/d/4-4-coucou-bis/]]></id>
+    <link href="http://localhost:8080/d/4-coucou-bis/" />
+    <id>http://localhost:8080/atom/d/4</id>
     <updated>2022-12-29T15:22:48+00:00</updated>
 
         <entry>
         <title><![CDATA[Coucou bis]]></title>
         <link rel="alternate" type="text/html" href="http://localhost:8080/d/4-coucou-bis/1"/>
-        <id>http://localhost:8080/d/4-coucou-bis/1</id>
+        <id>http://localhost:8080/d/4/1</id>
         <updated>2022-12-29T15:21:59+00:00</updated>
         <content
                     type="html"
diff -u --color before/discussions.xml after/discussions.xml
--- before/discussions.xml	2022-12-29 16:29:18.783690085 +0100
+++ after/discussions.xml	2022-12-29 16:26:34.314552262 +0100
@@ -6,13 +6,13 @@
     <subtitle><![CDATA[The newest discussions in the forum]]></subtitle>
     <link href="http://localhost:8080/atom/discussions" rel="self" />
     <link href="http://localhost:8080/" />
-    <id><![CDATA[http://localhost:8080/]]></id>
+    <id>http://localhost:8080/atom/discussions</id>
     <updated>2022-12-29T15:22:40+00:00</updated>
 
         <entry>
         <title><![CDATA[Coucou bis]]></title>
         <link rel="alternate" type="text/html" href="http://localhost:8080/d/4-coucou-bis/1"/>
-        <id>http://localhost:8080/d/4-coucou-bis/1</id>
+        <id>http://localhost:8080/d/4/1</id>
         <updated>2022-12-29T15:21:59+00:00</updated>
         <content
                     type="html"
@@ -24,7 +24,7 @@
         <entry>
         <title><![CDATA[Tout nouveau forum]]></title>
         <link rel="alternate" type="text/html" href="http://localhost:8080/d/3-tout-nouveau-forum/1"/>
-        <id>http://localhost:8080/d/3-tout-nouveau-forum/1</id>
+        <id>http://localhost:8080/d/3/1</id>
         <updated>2022-12-15T14:54:13+00:00</updated>
         <content
                     type="html"
@@ -36,7 +36,7 @@
         <entry>
         <title><![CDATA[Coucou les amis]]></title>
         <link rel="alternate" type="text/html" href="http://localhost:8080/d/1-coucou-les-amis/1"/>
-        <id>http://localhost:8080/d/1-coucou-les-amis/1</id>
+        <id>http://localhost:8080/d/1/1</id>
         <updated>2022-12-15T13:12:51+00:00</updated>
         <content
                     type="html"
diff -u --color before/global.xml after/global.xml
--- before/global.xml	2022-12-29 16:29:39.831835032 +0100
+++ after/global.xml	2022-12-29 16:26:59.150724742 +0100
@@ -6,13 +6,13 @@
     <subtitle><![CDATA[Last posts in the forum]]></subtitle>
     <link href="http://localhost:8080/atom" rel="self" />
     <link href="http://localhost:8080/" />
-    <id><![CDATA[http://localhost:8080/]]></id>
+    <id>http://localhost:8080/atom</id>
     <updated>2022-12-29T15:22:36+00:00</updated>
 
         <entry>
         <title><![CDATA[Coucou bis]]></title>
         <link rel="alternate" type="text/html" href="http://localhost:8080/d/4-coucou-bis/1"/>
-        <id>http://localhost:8080/d/4-coucou-bis/1</id>
+        <id>http://localhost:8080/d/4/1</id>
         <updated>2022-12-29T15:21:59+00:00</updated>
         <content
                     type="html"
@@ -24,7 +24,7 @@
         <entry>
         <title><![CDATA[Tout nouveau forum]]></title>
         <link rel="alternate" type="text/html" href="http://localhost:8080/d/3-tout-nouveau-forum/21"/>
-        <id>http://localhost:8080/d/3-tout-nouveau-forum/21</id>
+        <id>http://localhost:8080/d/3/21</id>
         <updated>2022-12-23T12:52:27+00:00</updated>
         <content
                     type="html"
@@ -36,7 +36,7 @@
         <entry>
         <title><![CDATA[Coucou les amis]]></title>
         <link rel="alternate" type="text/html" href="http://localhost:8080/d/1-coucou-les-amis/6"/>
-        <id>http://localhost:8080/d/1-coucou-les-amis/6</id>
+        <id>http://localhost:8080/d/1/6</id>
         <updated>2022-12-15T13:25:52+00:00</updated>
         <content
                     type="html"
diff -u --color before/t-general-discussions.xml after/t-general-discussions.xml
--- before/t-general-discussions.xml	2022-12-29 16:28:36.343397397 +0100
+++ after/t-general-discussions.xml	2022-12-29 16:25:13.481989007 +0100
@@ -6,13 +6,13 @@
     <subtitle><![CDATA[The newest discussions in the general tag]]></subtitle>
     <link href="http://localhost:8080/atom/t/general/discussions" rel="self" />
     <link href="http://localhost:8080/" />
-    <id><![CDATA[http://localhost:8080/]]></id>
+    <id>http://localhost:8080/atom/t/general/discussions</id>
     <updated>2022-12-29T15:22:46+00:00</updated>
 
         <entry>
         <title><![CDATA[Coucou les amis]]></title>
         <link rel="alternate" type="text/html" href="http://localhost:8080/d/1-coucou-les-amis/1"/>
-        <id>http://localhost:8080/d/1-coucou-les-amis/1</id>
+        <id>http://localhost:8080/d/1/1</id>
         <updated>2022-12-15T13:12:51+00:00</updated>
         <content
                     type="html"
diff -u --color before/t-general.xml after/t-general.xml
--- before/t-general.xml	2022-12-29 16:28:57.399542682 +0100
+++ after/t-general.xml	2022-12-29 16:26:14.258412784 +0100
@@ -6,13 +6,13 @@
     <subtitle><![CDATA[Last posts in the general tag]]></subtitle>
     <link href="http://localhost:8080/atom/t/general" rel="self" />
     <link href="http://localhost:8080/" />
-    <id><![CDATA[http://localhost:8080/]]></id>
+    <id>http://localhost:8080/atom/t/general</id>
     <updated>2022-12-29T15:22:44+00:00</updated>
 
         <entry>
         <title><![CDATA[Coucou les amis]]></title>
         <link rel="alternate" type="text/html" href="http://localhost:8080/d/1-coucou-les-amis/6"/>
-        <id>http://localhost:8080/d/1-coucou-les-amis/6</id>
+        <id>http://localhost:8080/d/1/6</id>
         <updated>2022-12-15T13:25:52+00:00</updated>
         <content
                     type="html"

As stated above, modifying the ID will cause the aggregator to double the entries, so this will be the case after this change is merged. But only once 🙂.

@n-peugnet
Copy link
Author

I just pushed 361c204 that make "self" link behave like a proper canonical URI (see RFC 4287 (page22) and RSS guidelines for Apple News (Tags and attributes)).

Also, FWIW this branch is live on https://forum.club1.fr.

@n-peugnet
Copy link
Author

n-peugnet commented Jan 29, 2023 •
edited
Loading

@imorland: what do you think about this? I don't know RSS as well as Atom but it seems that the guid element12 could also benefit from the id entry I added in this PR. Do you think I should also change it in this pull request?

EDIT: I just pushed 65d14cf that does just that.

Footnotes

  1. https://cyber.harvard.edu/rss/rss.html#ltguidgtSubelementOfLtitemgt ↩

  2. https://validator.w3.org/feed/docs/warning/MissingGuid.html ↩

@n-peugnet
Add getPermalink() to the feed's controller
a8388ea 
This way both getFeedId and getSelfLink use the same function by default,
but changing one does not affect the other. This is more in line with the
real purpose of each function.
@n-peugnet
Fix: use entry's 'id' field as guid for RSS feed
65d14cf 
And rename the 'permalink' entry's field into 'link', because it is not
a permalink, as it contains the discussion's slug.
Also be explicit about the fact that RSS' entries' guid is a permalink
by setting 'isPermaLink' to ture, even if it is the default.
@n-peugnet n-peugnet changed the title Fix atom id fields and incorrect use of slug Fix Atom & RSS id fields and incorrect use of slug Jan 29, 2023
@imorland
Copy link
Owner

Sorry for my delayed response, and thank you for putting this PR together @n-peugnet

I'll take a proper look over this later today, but first impressions look good 👍

@imorland imorland merged commit 8d3b3d8 into imorland:master Feb 1, 2023
@zgq354 zgq354 mentioned this pull request May 3, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@imorland imorland imorland approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@n-peugnet @imorland