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.

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

Fix Atom feed #764

Merged
merged 9 commits into from May 4, 2020
Merged

Fix Atom feed #764

merged 9 commits into from May 4, 2020

Conversation

KitaitiMakoto
Copy link
Contributor

This patches include breaking change. I percent-encoded URI segments in ap_urls. This affects only future users, blogs and posts. Existing ap_urls will not be changed.

This fixes Atom feeds so that it conform to Atom spec. This should fix Atom-related issues #673 and #735.

@codecov
Copy link

codecov bot commented May 2, 2020

Codecov Report

Merging #764 into master will decrease coverage by 0.03%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master     #764      +/-   ##
==========================================
- Coverage   39.01%   38.97%   -0.04%     
==========================================
  Files          73       73              
  Lines        9700     9712      +12     
  Branches     2224     2226       +2     
==========================================
+ Hits         3784     3785       +1     
- Misses       4863     4875      +12     
+ Partials     1053     1052       -1     

@elegaanz
Copy link
Member

elegaanz commented May 3, 2020

Could it be possible to encode URI only for the Atom feed, not in the whole app? The JSON-LD spec (on which ActivityPub is built) says that every URI actually is an IRI, meaning it should be able to contain any Unicode character (with very few exceptions), so I don't know how percent-encoded strings are treated…

@KitaitiMakoto
Copy link
Contributor Author

KitaitiMakoto commented May 3, 2020

I've done! Thank you for your advice.

And... actually, Atom also uses IRI. Its spec says:

4.2.6. The "atom:id" Element
(snip)
Its content MUST be an IRI, as defined by [RFC3987].

This is the reason why I reverted all the code around percent-encoding. I don't know why W3C feed validator claims IRIs are errors. But, how about deploying IRI version of Atom and convert them to URI if someone reports issue about specified feed reader implementation?

@elegaanz
Copy link
Member

elegaanz commented May 3, 2020

But, how about deploying IRI version of Atom and convert them to URI if someone reports issue about specified feed reader implementation?

Sorry, but I'm not sure if I understand. Could you please try to rephrase ? (my English is so bad, help) ☹️

@KitaitiMakoto
Copy link
Contributor Author

Thank you for reply. I'm also not good at English. My English should be hard to understand. Will rephrase it. What I mean was...

  • My current patch allows Atom feeds to use IRI in id, link and so on.
  • It is because Atom spec says their value is IRI.
  • Some Atom reader implementations might not parse it correctly.
  • I propose that we accept the possibility until someone reports the issue actually.
  • How do you think about it? If it's okay, can you merge these patches?
  • My point is these patches doesn't lose anything from now.

@elegaanz
Copy link
Member

elegaanz commented May 4, 2020

Okay, it is clearer, thank you. Indeed, I think we can merge.

@elegaanz elegaanz merged commit 847d6f7 into Plume-org:master May 4, 2020
@KitaitiMakoto
Copy link
Contributor Author

Thank you!

@KitaitiMakoto KitaitiMakoto deleted the fix-atom branch May 4, 2020 18:37
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.

None yet

2 participants