Moodle
  1. Moodle
  2. MDL-30203

RSS feed: adds <enclosure> tag for mailto: links; invalid

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.2
    • Fix Version/s: 2.2
    • Component/s: RSS
    • Labels:
    • Testing Instructions:
      Hide

      1. Ensure that RSS is turned on at site level (there are two settings; the overall one and the yes/no dropdown for forums; you can find both if you search for 'rss' in admin menu).

      2. Ensure that RSS is turned on for a test forum in the forum settings.

      3. Start a new discussion in the forum; use the following HTML code as message text:

      <p><a href="mailto:test@example.org">Test</a></p>
      

      4. On the forum main page, view source and search for 'rss' to obtain the link for the RSS feed (see MDL-30202)

      5. View source of the RSS feed.
      Confirm there is no <enclosure> tag in the item in question.

      6. Validate the RSS feed at http://validator.w3.org/feed/
      Confirm that the feed is valid (albeit there may be some warnings).

      7. Using Firefox, view the RSS feed.
      Verify that the message with the mailto link displays correctly.

      Note: Ideally this test would also cover an actual attachment, such as an mp3, from a forum post, but currently the forum code that is supposed to provide the list of attachments is TODOed out, so there is no point trying it.

      Show
      1. Ensure that RSS is turned on at site level (there are two settings; the overall one and the yes/no dropdown for forums; you can find both if you search for 'rss' in admin menu). 2. Ensure that RSS is turned on for a test forum in the forum settings. 3. Start a new discussion in the forum; use the following HTML code as message text: <p><a href= "mailto:test@example.org" >Test</a></p> 4. On the forum main page, view source and search for 'rss' to obtain the link for the RSS feed (see MDL-30202 ) 5. View source of the RSS feed. Confirm there is no <enclosure> tag in the item in question. 6. Validate the RSS feed at http://validator.w3.org/feed/ Confirm that the feed is valid (albeit there may be some warnings). 7. Using Firefox, view the RSS feed. Verify that the message with the mailto link displays correctly. Note: Ideally this test would also cover an actual attachment, such as an mp3, from a forum post, but currently the forum code that is supposed to provide the list of attachments is TODOed out, so there is no point trying it.
    • Affected Branches:
      MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_22_STABLE
    • Pull Master Branch:
      MDL-30203-master
    • Rank:
      26128

      Description

      1. The RSS feed system adds <enclosure> tags, which are supposed to be URLs to file attachments (such as podcasts), for mailto: links. (I haven't tested, but possibly it adds them for other URLs such as http URLs which are not files. This would also be inappropriate.)

      2. When it adds the <enclosure> tag for a mailto link, the tag does not have the 'length' attribute. This makes the feed invalid as reported by the W3C validator. The length attribute is required.

      3. These messages do not display at all when looking at the feed in Firefox. (This might actually be a Firefox bug, or at least it's excessively fragile, but many people use Firefox to check RSS feeds and this can cause confusion when testing, so it would be better if Moodle does not unnecessarily create feeds that break Firefox.)

      This is being caused by code that tries to automatically add enclosure tags based only on the content of the message, without any knowledge of whether or not there are actual attachments and what size they are. The code has comments acknowledging that it does not work correctly because it doesn't know the length. Also, the code contains a feature that correctly supports attachments with length values if provided by modules.

      I propose that we remove the 'randomly include all links' feature, leaving only the support for attachments with length values supplied by module.

      (Note that if anyone is relying on this for a 'podcast' (really?) and if by some unlikely chance it actually worked before despite being invalid, including irrelevant links, etc., then this change I'm proposing will actually remove that support - at least with regard to forum. You can see why by looking at the TODO in mod/forum/rsslib.php line 305 where attachment support is commented out.)

        Activity

        Hide
        Dan Poltawski added a comment -

        I see this problem in the moodle.org RSS feed all the time in Google Reader and indeed I agree with the assertion.

        Maybe it affects the contrib podcasting module?

        Show
        Dan Poltawski added a comment - I see this problem in the moodle.org RSS feed all the time in Google Reader and indeed I agree with the assertion. Maybe it affects the contrib podcasting module?
        Hide
        Dan Poltawski added a comment -

        Interestingly the only case where those file enclosures would work would surely be when the user is logged in anyway..

        Show
        Dan Poltawski added a comment - Interestingly the only case where those file enclosures would work would surely be when the user is logged in anyway..
        Hide
        Sam Marshall added a comment -

        Dan - good point, I didn't think of that - you're saying that for modules like forum you have to be logged on in order to access attachments, so they wouldn't work from an external 'podcast' client which operates without user intervention, so this 'feature' is largely pointless anyway... users are better off just having to click on the links.

        I assume that the contrib podcasting module(s) have their own fileserving script so as not to require login. Hopefully, if they use the core logic to generate feeds, they also use the attachment thingy [ie the bit of code I am proposing to leave in!] so as to create a valid feed with length included. If they don't do that and therefore don't output length, they need fixing anyway (invalid feeds), imo, so probably better to still make this change.

        Show
        Sam Marshall added a comment - Dan - good point, I didn't think of that - you're saying that for modules like forum you have to be logged on in order to access attachments, so they wouldn't work from an external 'podcast' client which operates without user intervention, so this 'feature' is largely pointless anyway... users are better off just having to click on the links. I assume that the contrib podcasting module(s) have their own fileserving script so as not to require login. Hopefully, if they use the core logic to generate feeds, they also use the attachment thingy [ie the bit of code I am proposing to leave in!] so as to create a valid feed with length included. If they don't do that and therefore don't output length, they need fixing anyway (invalid feeds), imo, so probably better to still make this change.
        Hide
        Eloy Lafuente (stronk7) added a comment -

        The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

        TIA and ciao

        PS: Note this is the last message of this type that you will receive along the whole November month, because we are running continuous integration this weeks while QA tests for release of Moodle 2.2 (Dec 1st) are being performed.

        Show
        Eloy Lafuente (stronk7) added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao PS: Note this is the last message of this type that you will receive along the whole November month, because we are running continuous integration this weeks while QA tests for release of Moodle 2.2 (Dec 1st) are being performed.
        Hide
        Sam Marshall added a comment -

        Rebased.

        Show
        Sam Marshall added a comment - Rebased.
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Just confirmed that the the ipodcast contrib module seems to have its own rsslib with methods like 'getfeed_add_enclosures' handling that. Anyway it also uses core rsslib.php (includes it).

        I hope this won't break anything... integrating (as far as the enclosures being generated were incorrect - missing length).

        Ciao

        Show
        Eloy Lafuente (stronk7) added a comment - Just confirmed that the the ipodcast contrib module seems to have its own rsslib with methods like 'getfeed_add_enclosures' handling that. Anyway it also uses core rsslib.php (includes it). I hope this won't break anything... integrating (as far as the enclosures being generated were incorrect - missing length). Ciao
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Integrated, thanks!

        Show
        Eloy Lafuente (stronk7) added a comment - Integrated, thanks!
        Hide
        Adrian Greeve added a comment -

        checked the source code for the xml, and then ran in firefox. The link turns up properly. Passed.

        Show
        Adrian Greeve added a comment - checked the source code for the xml, and then ran in firefox. The link turns up properly. Passed.
        Hide
        Eloy Lafuente (stronk7) added a comment -

        And this has landed upstream, just on time for the upcoming new releases week. Thanks for it!

        Ciao

        Show
        Eloy Lafuente (stronk7) added a comment - And this has landed upstream, just on time for the upcoming new releases week. Thanks for it! Ciao

          People

          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: