search

AmauryCarrade / flarum-ext-syndication

Brings RSS and Atom feeds to Flarum
Other
20 stars 13 forks source link

Simplicity and validation #10

Closed zerosonesfun closed 5 years ago

zerosonesfun commented 5 years ago

These changes make things simple, ensure the feeds validate, and keep the feed post content text only versus trying to include HTML. This last part is admittedly a personal preference. I do not think we need the feeds to include the full HTML for posts. Plain text descriptions/summaries should suffice and help to ensure validation and feed reader compatibility.

The main reason I started making these changes is because the ID portion of the atom feed wasn’t validating. It was only pulling in the post ID which happens to be “1” every time. This pull request makes the ID the full permalink which validates.

dexif commented 5 years ago

+1

AmauryCarrade commented 5 years ago

Hi,

Thank you for your PR! I'm not home right now so I won't be able to review and test it until next week, but it's very appreciated :)

zerosonesfun commented 5 years ago

I noticed another small issue when validating the atom feed. It did not like the site URL not ending with a “/“. Therefore, I added a slash in the code.

There is just one more warning at times but it is not a big deal. Sometimes, depending on the post (a post with no text, just an image, for example), the summary may end up being blank. The feed validator still marks the feed as valid, but gives a warning that there shouldn’t be blank summaries. In the future, some type of if/else coding could solve this... if summary is blank, add the text, “No summary found.” This way, there is never a blank summary.

Here are screenshots of my feeds validating. As I state above in my replies, the couple changes you suggest are not necessary; the feeds are valid (you will see a warning about the utf-8 encoding but that’s just something with my server. Others may not get that warning...

RSS valid:

0600-BE66-232-F-4106-9-F66-E108-C1-B75978

ATOM valid:

609132-F1-5-D56-4-F67-986-B-833-A0-FF00-D31
zerosonesfun commented 5 years ago

@AmauryCarrade,

Please review my latest changes. Here is what I have done:

AmauryCarrade commented 5 years ago

This means, if users want, they can remove or rename the original blade.php files, and remove the "nonHTML" part in these filenames and then the extension will use the non HTML / excerpt versions instead.

One should never change anything in the vendor directory, so I'm not convinced. This being said, I'll merge these changes and add a settings interface to configure:

Thanks for your contributions!