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
            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: