Details

    • Database:
      Any
    • Testing Instructions:
      Hide

      1. Find a Google News RSS feed (any will do).
      2. Test that feed outside of Moodle.
      3. Add a new Remote RSS Feed block.
      4. Add the Google News feed to Moodle.
      5. Add the feed to the block.
      6. Verify that when you click on the item links you are taken directly to the story.

      Show
      1. Find a Google News RSS feed (any will do). 2. Test that feed outside of Moodle. 3. Add a new Remote RSS Feed block. 4. Add the Google News feed to Moodle. 5. Add the feed to the block. 6. Verify that when you click on the item links you are taken directly to the story.
    • Workaround:
      Hide

      No way

      Show
      No way
    • Affected Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE, MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE
    • Pull from Repository:
    • Rank:
      39008

      Description

      1. Login as Admin > RSS client > Add/edit feeds
      2. Test the new feed, works well
      3. Add a new 'Remote RSS feeds' block.
      4. Block display fine.
      BUT click on any linked list, go to error page, 'Incorrect element id! '

      Checked the url:
      http://mysite/moodle/mod/glossary/showentry.php?courseid=110&eid=51
      Obviously it should be:
      http://mysite/moodle/mod/glossary/showentry.php?courseid=110&eid=51 (manually change it and works)

      HOW COME the link made by Moodle2.2.1 replaced the '&' with '&'?

      When I created a 'Remote RSS feeds' block for a Database activity module, got SAME problem:

      where:
      http://mysite/moodle//mod/data/view.php?d=2&rid=12
      link to the main database view, rather than the 'view single', It should be:
      http://mysite/moodle/mod/data/view.php?d=2&rid=12
      HOW COME the link made by Moodle2.2.1 replaced the '&' with '&'?

      (When I created a 'Remote RSS feeds' block for a forum module, No problem, list link to the correct post)

      Can some one help me to fix it please. URGENT

      Thanks

        Issue Links

          Activity

          Hide
          Michael de Raadt added a comment -

          Thanks for reporting this. It looks like the URL parameters are being unecessarily escaped.

          I've put that on the backlog.

          In the meantime feel free to help us work on this issue. If you are able to provide a patch, please add a patch label so we will spot it.

          Show
          Michael de Raadt added a comment - Thanks for reporting this. It looks like the URL parameters are being unecessarily escaped. I've put that on the backlog. In the meantime feel free to help us work on this issue. If you are able to provide a patch, please add a patch label so we will spot it.
          Hide
          Greg Rodenhiser added a comment -

          I didn't have time to make a patch but here's the fix for rss:

          Edit <MOODLE_ROOT>/blocks/rss_client/block_rss_client.php and change the following line:

          $link = new moodle_url($link);

          To:

          $link = htmlspecialchars_decode($link);

          Show
          Greg Rodenhiser added a comment - I didn't have time to make a patch but here's the fix for rss: Edit <MOODLE_ROOT>/blocks/rss_client/block_rss_client.php and change the following line: $link = new moodle_url($link); To: $link = htmlspecialchars_decode($link);
          Hide
          Wenxin Lu added a comment -

          Dear Greg,
          It works!
          Thank you very much.
          Hope you can add this fix into next version.

          Cheers

          Show
          Wenxin Lu added a comment - Dear Greg, It works! Thank you very much. Hope you can add this fix into next version. Cheers
          Hide
          Charles Fulton added a comment -

          Reading over the source code I think the key assumption is stated a few lines up: "URLs in our RSS cache will be escaped (correctly as theyre store in XML)...html_writer::link() will re-escape them. To prevent double escaping unescape here." That's not true anymore. SimplePie stores feeds as serialized data. Thus the rss feeds are subjected to a second round of cleaning which I think is unnecessary. The call to html_writer::link() in block_rss_client.php is the only one in the codebase which has an embedded call to clean_param (git grep -n html_writer::link | grep clean). Removing that function call lets the ampersands through as is, which I think is the desired behavior.

          Show
          Charles Fulton added a comment - Reading over the source code I think the key assumption is stated a few lines up: "URLs in our RSS cache will be escaped (correctly as theyre store in XML)...html_writer::link() will re-escape them. To prevent double escaping unescape here." That's not true anymore. SimplePie stores feeds as serialized data. Thus the rss feeds are subjected to a second round of cleaning which I think is unnecessary. The call to html_writer::link() in block_rss_client.php is the only one in the codebase which has an embedded call to clean_param (git grep -n html_writer::link | grep clean). Removing that function call lets the ampersands through as is, which I think is the desired behavior.
          Hide
          Dan Poltawski added a comment -

          Thanks Charles,

          +1 here, submitting for integration.

          Show
          Dan Poltawski added a comment - Thanks Charles, +1 here, submitting for integration.
          Hide
          Dan Poltawski added a comment -

          Note that we'd quite like to stop simplepie from cleaning this stuff, but I am not aware that its possible, so I think this solution is a pragmatic solution to the problem.

          (Charles, could you add testing instructions, etc).

          Show
          Dan Poltawski added a comment - Note that we'd quite like to stop simplepie from cleaning this stuff, but I am not aware that its possible, so I think this solution is a pragmatic solution to the problem. (Charles, could you add testing instructions, etc).
          Hide
          Charles Fulton added a comment -

          Thanks Dan, added testing instructions. This should be a clean cherry-pick.

          Show
          Charles Fulton added a comment - Thanks Dan, added testing instructions. This should be a clean cherry-pick.
          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

          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
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Integrated (22, 23 & master), thanks!

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

          Tested on 2.2, 2.3 and master integration branches.
          Links go to where they should.
          No problems encountered.
          Test passed.

          Show
          Adrian Greeve added a comment - Tested on 2.2, 2.3 and master integration branches. Links go to where they should. No problems encountered. Test passed.
          Hide
          Dan Poltawski added a comment -

          Congratulations, you've done it!

          Thanks, this change is now in the latest weekly release!

          Join the crowds of people tomorrow from 8am and download this Moodle release from your local apple store!

          Show
          Dan Poltawski added a comment - Congratulations, you've done it! Thanks, this change is now in the latest weekly release! Join the crowds of people tomorrow from 8am and download this Moodle release from your local apple store!

            People

            • Votes:
              8 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: