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:

      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

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            salvetore 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
            salvetore 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
            rhodes187 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
            rhodes187 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 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 Wenxin Lu added a comment - Dear Greg, It works! Thank you very much. Hope you can add this fix into next version. Cheers
            Hide
            cfulton 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
            cfulton 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
            poltawski Dan Poltawski added a comment -

            Thanks Charles,

            +1 here, submitting for integration.

            Show
            poltawski Dan Poltawski added a comment - Thanks Charles, +1 here, submitting for integration.
            Hide
            poltawski 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
            poltawski 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
            cfulton Charles Fulton added a comment -

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

            Show
            cfulton Charles Fulton added a comment - Thanks Dan, added testing instructions. This should be a clean cherry-pick.
            Hide
            stronk7 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
            stronk7 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
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Integrated (22, 23 & master), thanks!

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Integrated (22, 23 & master), thanks!
            Hide
            abgreeve 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
            abgreeve 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
            poltawski 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
            poltawski 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:
                  Fix Release Date:
                  12/Nov/12