Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-28952

Link to user profile in comments block fails

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.0.3, 2.1.4, 2.2.1
    • Fix Version/s: 2.1.6, 2.2.3
    • Component/s: Comments
    • Labels:
    • Testing Instructions:
      Hide
      1. Goto a course>turn editing> add a comment block
      2. Add few comments as admin, make sure the correct course urls are displayed to you along with the comment
      3. Refresh the page, make sure links are still correct
      4. login as a student and make a comment and refresh
      5. make sure all commenter names are linked to correct profile urls
      Show
      Goto a course>turn editing> add a comment block Add few comments as admin, make sure the correct course urls are displayed to you along with the comment Refresh the page, make sure links are still correct login as a student and make a comment and refresh make sure all commenter names are linked to correct profile urls
    • Affected Branches:
      MOODLE_20_STABLE, MOODLE_21_STABLE, MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE
    • Pull Master Branch:
      MDL-28952-master

      Description

      Prerequisite:

      In the comments block, there are links to each commenting user's profile of this type:
      http://MYMOODLEDOMAIN/user/view.php?id=USERID&course=COURSEID

      But: "&" should be "&".

      Now, if a user clicks this link, the course parameter isn't recognized and the user is forwarded to
      http://MYMOODLEDOMAIN/user/profile.php?id=USERID
      which he may or may not be able to view.

      Replacing in /comment/lib.php, line 598
      $c->profileurl = $url->out();
      with
      $c->profileurl = $url->out(false);
      solves the problem, the user is shown view.php as desired, but I don't know which side-effects this has.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            salvetore Michael de Raadt added a comment -

            Thanks for reporting this. I was able to replicate the problem.

            It looks like this ampersand is being escaped unnecessarily.

            The link is correct when it is loaded by Ajax after the comment is added, but it is incorrect when someone else views it or the page is reloaded.

            Show
            salvetore Michael de Raadt added a comment - Thanks for reporting this. I was able to replicate the problem. It looks like this ampersand is being escaped unnecessarily. The link is correct when it is loaded by Ajax after the comment is added, but it is incorrect when someone else views it or the page is reloaded.
            Hide
            abias Alexander Bias added a comment -

            I just verified that the problem is still present in 2.2.x
            I would be grateful if the small patch could be included into moodle core

            Show
            abias Alexander Bias added a comment - I just verified that the problem is still present in 2.2.x I would be grateful if the small patch could be included into moodle core
            Hide
            ankit_frenz Ankit Agarwal added a comment -

            $url->out should really be used only when we have to print the output
            up for review
            Thanks

            Show
            ankit_frenz Ankit Agarwal added a comment - $url->out should really be used only when we have to print the output up for review Thanks
            Hide
            rwijaya Rossiani Wijaya added a comment -

            Looks good Ankit.

            +1 for integration.

            Show
            rwijaya Rossiani Wijaya added a comment - Looks good Ankit. +1 for integration.
            Hide
            ankit_frenz Ankit Agarwal added a comment -

            Thanks Rosie for the review.
            Sending for integration
            Thanks

            Show
            ankit_frenz Ankit Agarwal added a comment - Thanks Rosie for the review. Sending for integration Thanks
            Hide
            samhemelryk Sam Hemelryk added a comment -

            Hi guys,

            There is a bit of an issue here as you are changing the result that is returned from the get_comments method (comments API).
            Normally we would only allow such a change in master as it backporting it to stable may cause peoples own code to break.
            If there's a good reason to backport then it can be considered. Either way I think the original solution Alexander suggested may be the safer approach for STABLE branches.
            (For instance Moodle's plugins database uses this API and would be affected)

            I've added Dongsheng as a watcher on this issue. Dongsheng would you mind having a look at this and seeing what you think about changing the result of the get_comments API.
            Is it a good idea or do you think we should avoid it.

            Cheers
            Sam

            Show
            samhemelryk Sam Hemelryk added a comment - Hi guys, There is a bit of an issue here as you are changing the result that is returned from the get_comments method (comments API). Normally we would only allow such a change in master as it backporting it to stable may cause peoples own code to break. If there's a good reason to backport then it can be considered. Either way I think the original solution Alexander suggested may be the safer approach for STABLE branches. (For instance Moodle's plugins database uses this API and would be affected) I've added Dongsheng as a watcher on this issue. Dongsheng would you mind having a look at this and seeing what you think about changing the result of the get_comments API. Is it a good idea or do you think we should avoid it. Cheers Sam
            Hide
            dongsheng Dongsheng Cai added a comment -

            Hello all

            I think using `$c->profileurl = $url->out(false);` makes sense. I cannot see problems of fixing & in url, after all it's the expected result, is it?

            Show
            dongsheng Dongsheng Cai added a comment - Hello all I think using `$c->profileurl = $url->out(false);` makes sense. I cannot see problems of fixing & in url, after all it's the expected result, is it?
            Hide
            samhemelryk Sam Hemelryk added a comment -

            Hi Dongsheng,

            $url->out(false) I think is certainly the fix for the stable branches.
            But for the master branch what do you think about changing profileurl from a string, to a moodle_url object?

            (sorry I could have been clearer in my earlier question)

            Cheers
            Sam

            Show
            samhemelryk Sam Hemelryk added a comment - Hi Dongsheng, $url->out(false) I think is certainly the fix for the stable branches. But for the master branch what do you think about changing profileurl from a string, to a moodle_url object? (sorry I could have been clearer in my earlier question) Cheers Sam
            Hide
            dongsheng Dongsheng Cai added a comment -

            Hi Sam

            I suggest to use $url->out(false).

            The return value from get_comments will be used by json_encode, thus moodle_url::__toString will be called, currently this method is using moodle_url::out(true)

            Show
            dongsheng Dongsheng Cai added a comment - Hi Sam I suggest to use $url->out(false). The return value from get_comments will be used by json_encode, thus moodle_url::__toString will be called, currently this method is using moodle_url::out(true)
            Hide
            samhemelryk Sam Hemelryk added a comment -

            Awesome thanks DS, that clears it up nicely!

            All up to you now Ankit.

            Show
            samhemelryk Sam Hemelryk added a comment - Awesome thanks DS, that clears it up nicely! All up to you now Ankit.
            Hide
            ankit_frenz Ankit Agarwal added a comment -

            Thanks guys for the feedback,
            Changes made, submitting again for integration.
            Thanks

            Show
            ankit_frenz Ankit Agarwal added a comment - Thanks guys for the feedback, Changes made, submitting again for integration. Thanks
            Hide
            samhemelryk Sam Hemelryk added a comment -

            Thanks Ankit, this has been integrated now

            Show
            samhemelryk Sam Hemelryk added a comment - Thanks Ankit, this has been integrated now
            Hide
            abgreeve Adrian Greeve added a comment -

            Tested on branches 2.1, 2.2 and master. The link works properly now.
            Thanks.

            Show
            abgreeve Adrian Greeve added a comment - Tested on branches 2.1, 2.2 and master. The link works properly now. Thanks.
            Hide
            samhemelryk Sam Hemelryk added a comment -

            Congratulations are in order, you've made it, or at least your code has!
            It's now part of Moodle and both the git and cvs repositories have been updated.

            This issue is being marked as fixed and closed.

            Thank you.

            Show
            samhemelryk Sam Hemelryk added a comment - Congratulations are in order, you've made it, or at least your code has! It's now part of Moodle and both the git and cvs repositories have been updated. This issue is being marked as fixed and closed. Thank you.
            Hide
            netspot_dbezborodov Damien Bezborodov added a comment -

            I have a patch at https://github.com/dbezborodovrp/moodle/commit/e6b79031314e6da13cc645fd5ac08bc64f7e184f , but am not quite sure how to attach patch to this ticket in JIRA.

            Show
            netspot_dbezborodov Damien Bezborodov added a comment - I have a patch at https://github.com/dbezborodovrp/moodle/commit/e6b79031314e6da13cc645fd5ac08bc64f7e184f , but am not quite sure how to attach patch to this ticket in JIRA.
            Hide
            netspot_dbezborodov Damien Bezborodov added a comment -

            Sorry, wrong ticket... see MDL-44634

            Show
            netspot_dbezborodov Damien Bezborodov added a comment - Sorry, wrong ticket... see MDL-44634

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  14/May/12