Moodle
  1. Moodle
  2. MDL-28952

Link to user profile in comments block fails

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor 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: Commenting
    • 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
    • Rank:
      18501

      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.

        Issue Links

          Activity

          Hide
          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
          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
          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
          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 Agarwal added a comment -

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

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

          Looks good Ankit.

          +1 for integration.

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

          Thanks Rosie for the review.
          Sending for integration
          Thanks

          Show
          Ankit Agarwal added a comment - Thanks Rosie for the review. Sending for integration Thanks
          Hide
          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
          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 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 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
          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
          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 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 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
          Sam Hemelryk added a comment -

          Awesome thanks DS, that clears it up nicely!

          All up to you now Ankit.

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

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

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

          Thanks Ankit, this has been integrated now

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

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

          Show
          Adrian Greeve added a comment - Tested on branches 2.1, 2.2 and master. The link works properly now. Thanks.
          Hide
          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
          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
          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
          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
          Damien Bezborodov added a comment -

          Sorry, wrong ticket... see MDL-44634

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