Add a blog posts RSS channel #425

yceruto · 2017-01-09T22:47:47Z

This try to implement a simple RSS channel accepted in #155 since Aug 2015.

Knowledge that brings:

Custom XML extension to specify the format of the template (index.xml.twig).
How to render a simple XML file.

Any suggestions would be welcome! ;)

javiereguiluz

@yceruto thanks for implementing this feature. I like most of it ... but I've suggested some minor changes.

javiereguiluz · 2017-01-10T09:02:51Z

src/AppBundle/Controller/BlogController.php

+        // This is a ResponseHeaderBag that has some nice methods for getting and
+        // setting response headers. The header names are normalized so that using
+        // Content-Type is equivalent to content-type or even content_type.
+        $response->headers->set('Content-Type', 'application/xhtml+xml');


I'm not sure about this. It's great to show how this works in Symfony ... but it may look "horrible" to newcomers because they might think they always need to do that (but it's optional, because $this->render() takes care of that).

👍 Simplified by using _format.

javiereguiluz · 2017-01-10T09:05:01Z

app/Resources/views/blog/rss.xml.twig

@@ -0,0 +1,23 @@
+{% spaceless %}


Just asking: why this tag?

Just for remove spaces between tags, but this detail can be ignored for simplicity
👍 Removed!

javiereguiluz · 2017-01-10T09:08:35Z

app/Resources/views/blog/rss.xml.twig

+{% spaceless %}
+<?xml version="1.0" encoding="UTF-8" ?>
+<rss version="2.0">
+    <channel>


What if we add these two tags to the <channel> tag?

<pubDate>{{ 'now'|date('r') }}</pubDate> <lastBuildDate>{{ (posts|last).publishedAt|default('now')|date('r') }}</lastBuildDate>

pubDate: The original publication date for the channel or item. (optional)

lastBuildDate: The most recent time the content of the channel was modified. (optional)

They are totally optional, so if you don't agree that's OK!

👍 Added!

javiereguiluz · 2017-01-10T09:26:14Z

app/Resources/views/blog/rss.xml.twig

+                <description>{{ post.summary }}</description>
+                <link>{{ url('blog_post', {'slug': post.slug}) }}</link>
+                <guid>{{ url('blog_post', {'slug': post.slug}) }}</guid>
+                <pubDate>{{ post.publishedAt|date(format=RSS, timezone='GMT') }}</pubDate>


Here I also have doubts about:

{% set RSS = constant('RSS', date()) %} <pubDate>{{ post.publishedAt|date(format=RSS, timezone='GMT') }}</pubDate>

Why not using this?

<pubDate>{{ post.publishedAt|date('r', timezone='GMT') }}</pubDate>

Showing the Twig constants and all it's great ... but we should be careful. If newcomers see this, they'll think that Symfony is this complicated and you must do it like this.

You are right, RFC822 (r) and RSS constants have the same format. Fixed!

javiereguiluz · 2017-01-10T09:27:42Z

app/Resources/views/base.html.twig

@@ -45,6 +45,12 @@

                                {% block header_navigation_links %}


Maybe we should also add the following to the <head> part of the page:

<link rel="alternate" type="application/rss+xml" href="{{ path('blog_rss') }}">

👍 Added!

voronkovich · 2017-01-10T10:46:52Z

src/AppBundle/Controller/BlogController.php

+     * @Route("/rss.xml", name="blog_rss")
+     * @Method("GET")
+     */
+    public function rssAction()


@yceruto, I think it's not needed to create a particular action. You could use the _format parameter in the indexAction. See http://symfony.com/doc/current/quick_tour/the_controller.html#using-formats

👍 Updated!

yceruto · 2017-01-10T13:36:18Z

@javiereguiluz, @voronkovich Thanks for the review. I have addressed your comments.

javiereguiluz · 2017-01-10T14:08:14Z

src/AppBundle/Controller/BlogController.php

-     * @Route("/", defaults={"page": "1"}, name="blog_index")
-     * @Route("/page/{page}", requirements={"page": "[1-9]\d*"}, name="blog_index_paginated")
+     * @Route("/", defaults={"page": "1", "_format"="html"}, name="blog_index")
+     * @Route("/rss.{_format}", name="blog_rss", defaults={"page": "1", "_format"="xml"}, requirements={"_format": "xml"})


If we decide to have a different @Route for the RSS, maybe we can hardcode some things to simplify:

@Route("/rss.xml", name="blog_rss", defaults={"page": "1", "_format"="xml"})

Totally agree! Fixed.

yceruto · 2017-01-10T14:39:36Z

app/Resources/views/base.html.twig

@@ -9,6 +9,7 @@
        <meta charset="UTF-8" />
        <meta name="viewport" content="width=device-width, initial-scale=1"/>
        <title>{% block title %}Symfony Demo application{% endblock %}</title>
+        <link rel="alternate" type="application/rss+xml" href="{{ path('blog_rss') }}">


The Content-Type that we return here is text/xml (by default), is it a contradiction? should change this type to text/xml then?

To be honest I don't know. It makes sense to only use text/xml ... but then, will browsers recognize that as a RSS link?

According to http://www.rssboard.org/rss-autodiscovery#element-link-type

The type attribute MUST contain the feed's MIME type, which is "application/rss+xml" for RSS 1.0 or RSS 2.0 feeds.

javiereguiluz

👍

@yceruto thanks for implementing this feature and for all the changes added during the review!

bocharsky-bw · 2017-01-10T18:24:07Z

src/AppBundle/Controller/BlogController.php

-     * @Route("/", defaults={"page": "1"}, name="blog_index")
-     * @Route("/page/{page}", requirements={"page": "[1-9]\d*"}, name="blog_index_paginated")
+     * @Route("/", defaults={"page": "1", "_format"="html"}, name="blog_index")
+     * @Route("/rss.xml", name="blog_rss", defaults={"page": "1", "_format"="xml"})


Could we keep the order of arguments, i.e. move name="blog_rss" to the end like in others? 😇

xabbuh · 2017-01-10T22:03:14Z

src/AppBundle/Controller/BlogController.php

     */
-    public function indexAction($page)
+    public function indexAction($page, $_format)


I would still name this variable $format.

We lost then the auto Content-Type header response specially in blog_rss route, remember we need the special variable _format into defaults option to make it automatically.

Sure, you're right.

javiereguiluz · 2017-01-12T08:56:17Z

@yceruto thanks to the help of @iltar and @fbourigault in the Symfony Slack channel, I've managed to add a commit to your PR. The change I propose is to remove the RSS link from the header, which makes it look crowded:

And instead display it in the sidebar of blog/index and blog/show:

javiereguiluz

👍

javiereguiluz · 2017-01-12T16:35:46Z

@yceruto thanks for implementing this feature. I truly love it. Thanks!

This PR was merged into the master branch. Discussion ---------- Add a blog posts RSS channel This try to implement a simple RSS channel accepted in #155 since Aug 2015. Knowledge that brings: * Custom XML extension to specify the format of the template (`index.xml.twig`). * How to render a simple XML file. Any suggestions would be welcome! ;) Commits ------- d7420d5 Moved the RSS link to the sidebar cc8fe6b Minor tweaks 8a02a15 add rss title to link element 4e2c7db simplify the blog_rss route definition 254632c Minor tweaks in the translation strings 8a51859 add rss channel

javiereguiluz requested changes Jan 10, 2017

View reviewed changes

javiereguiluz added the Feature label Jan 10, 2017

voronkovich reviewed Jan 10, 2017

View reviewed changes

yceruto and others added 2 commits January 10, 2017 08:53

add rss channel

8a51859

Minor tweaks in the translation strings

254632c

javiereguiluz reviewed Jan 10, 2017

View reviewed changes

simplify the blog_rss route definition

4e2c7db

yceruto commented Jan 10, 2017

View reviewed changes

add rss title to link element

8a02a15

javiereguiluz approved these changes Jan 10, 2017

View reviewed changes

bocharsky-bw reviewed Jan 10, 2017

View reviewed changes

Minor tweaks

cc8fe6b

xabbuh reviewed Jan 10, 2017

View reviewed changes

Moved the RSS link to the sidebar

d7420d5

javiereguiluz approved these changes Jan 12, 2017

View reviewed changes

javiereguiluz added the FINISHED label Jan 12, 2017

javiereguiluz merged commit d7420d5 into symfony:master Jan 12, 2017

yceruto deleted the rss_channel branch January 12, 2017 17:15

javiereguiluz mentioned this pull request Jan 21, 2017

Add a RSS channel #155

Closed

Uh oh!

Add a blog posts RSS channel #425

Add a blog posts RSS channel #425

Uh oh!

Conversation

yceruto commented Jan 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

javiereguiluz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yceruto Jan 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yceruto commented Jan 10, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yceruto Jan 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

javiereguiluz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yceruto Jan 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

javiereguiluz commented Jan 12, 2017

Uh oh!

javiereguiluz left a comment

Choose a reason for hiding this comment

Uh oh!

javiereguiluz commented Jan 12, 2017

Uh oh!

Uh oh!

yceruto commented Jan 9, 2017 •

edited

Loading

yceruto Jan 10, 2017 •

edited

Loading

yceruto Jan 10, 2017 •

edited

Loading

yceruto Jan 10, 2017 •

edited

Loading