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

wiki doesn't redirect correctly when moodle installed in subdirectory

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 2.2.3, 2.2.4, 2.2.5, 2.2.10, 2.3, 2.3.1, 2.3.2, 2.3.6, 2.4.3
    • Fix Version/s: 2.3.7, 2.4.4
    • Component/s: Wiki (2.x)
    • Labels:

      Description

      Introduced in MDL-30478 (commit: 5dedd64)

      The redirect to (view.php?pageid=) doesn't include path (/mod/wiki/) so moodle_url() doesn't create a valid URL.

        Gliffy Diagrams

          Attachments

            Issue Links

              Activity

              Hide
              kemitix Paul Campbell added a comment -

              There were several other instances of add_to_log(), and a few redirect(), that didn't use moodle_url(). These have been fixed in the pull branch linked.

              Show
              kemitix Paul Campbell added a comment - There were several other instances of add_to_log(), and a few redirect(), that didn't use moodle_url(). These have been fixed in the pull branch linked.
              Hide
              salvetore Michael de Raadt added a comment -

              Thanks for reporting that and providing a fix.

              This might also help resolve MDL-36981. We should test that after fixing this issue.

              Show
              salvetore Michael de Raadt added a comment - Thanks for reporting that and providing a fix. This might also help resolve MDL-36981 . We should test that after fixing this issue.
              Hide
              rajeshtaneja Rajesh Taneja added a comment -

              Thanks for providing the patch Paul,

              add_to_log is just populating log table and has nothing to do with redirect.
              I tried replicating this issue on master and it seems to be working fine, (moodle installed in sub-dir).

              Can you please confirm if $CFG->wwwroot is set properly.
              Also, it will be helpful if you can provide some more details about your installation, as I was unable to replicate this issue with testing instructions provided in this ticket.

              FYI: links in add_to_log are relative and get reconstructed as per module. Please check other areas like book/view.php, url/view.php etc.

              Show
              rajeshtaneja Rajesh Taneja added a comment - Thanks for providing the patch Paul, add_to_log is just populating log table and has nothing to do with redirect. I tried replicating this issue on master and it seems to be working fine, (moodle installed in sub-dir). Can you please confirm if $CFG->wwwroot is set properly. Also, it will be helpful if you can provide some more details about your installation, as I was unable to replicate this issue with testing instructions provided in this ticket. FYI: links in add_to_log are relative and get reconstructed as per module. Please check other areas like book/view.php, url/view.php etc.
              Hide
              kemitix Paul Campbell added a comment -

              You're right. All the add_to_log changes I made are irrelevant.

              The issue is actually fixed by the change to mod/wiki/pagelib.php which only uses new moodle_url('view.php... rather than specifying the path /mod/wiki/view.php

              How should I update my fix? Rewrite the branch or create a new one?

              Show
              kemitix Paul Campbell added a comment - You're right. All the add_to_log changes I made are irrelevant. The issue is actually fixed by the change to mod/wiki/pagelib.php which only uses new moodle_url('view.php... rather than specifying the path /mod/wiki/view.php How should I update my fix? Rewrite the branch or create a new one?
              Hide
              rajeshtaneja Rajesh Taneja added a comment -

              Thanks Paul,

              You are right, it's not correct. Looking at the changes you have made in your current branch, it will be easy for you to create a new branch.

              We will appreciate if you can create patch for 2.4 and 2.3 branches as well.

              Show
              rajeshtaneja Rajesh Taneja added a comment - Thanks Paul, You are right, it's not correct. Looking at the changes you have made in your current branch, it will be easy for you to create a new branch. We will appreciate if you can create patch for 2.4 and 2.3 branches as well.
              Hide
              kemitix Paul Campbell added a comment -
              • Updating Description to reflect correct origin and nature of this bug.
              • Add new pull branches for master, 2.3 and 2.4
              Show
              kemitix Paul Campbell added a comment - Updating Description to reflect correct origin and nature of this bug. Add new pull branches for master, 2.3 and 2.4
              Hide
              rajeshtaneja Rajesh Taneja added a comment -

              Thanks for providing the quick patch Paul,

              pushing it for peer-review.

              Show
              rajeshtaneja Rajesh Taneja added a comment - Thanks for providing the quick patch Paul, pushing it for peer-review.
              Hide
              phalacee Jason Fowler added a comment -

              [Y] Syntax
              [-] Output
              [Y] Whitespace
              [-] Language
              [-] Databases
              [Y] Testing
              [-] Security
              [-] Documentation
              [Y] Git
              [Y] Sanity check

              Show
              phalacee Jason Fowler added a comment - [Y] Syntax [-] Output [Y] Whitespace [-] Language [-] Databases [Y] Testing [-] Security [-] Documentation [Y] Git [Y] Sanity check
              Hide
              damyon Damyon Wiese added a comment -

              This is not actually a bug. Sure it is nice to use the same standard as the rest of the code in wiki - but new moodle_url('view.php'...) works fine and is used in lots of places in Moodle. Pinging Raj to see if he ever managed to reproduce this bug.

              Show
              damyon Damyon Wiese added a comment - This is not actually a bug. Sure it is nice to use the same standard as the rest of the code in wiki - but new moodle_url('view.php'...) works fine and is used in lots of places in Moodle. Pinging Raj to see if he ever managed to reproduce this bug.
              Hide
              damyon Damyon Wiese added a comment -

              (And sorry - this will not help MDL-36981 at all - it will do nothing).

              Show
              damyon Damyon Wiese added a comment - (And sorry - this will not help MDL-36981 at all - it will do nothing).
              Hide
              poltawski Dan Poltawski added a comment -

              Integrated to master, 24 and 23. Thanks a lot Paul!

              Show
              poltawski Dan Poltawski added a comment - Integrated to master, 24 and 23. Thanks a lot Paul!
              Hide
              poltawski Dan Poltawski added a comment -

              LOL, great teamwork there by me and Damyon!

              Show
              poltawski Dan Poltawski added a comment - LOL, great teamwork there by me and Damyon!
              Hide
              poltawski Dan Poltawski added a comment -

              Discussed with damyon that this does produce different urls (with the / then url will be an absolute rather than relative url).

              But Damyon contends that this doesn't actually change in redirect() because redirect handles it.

              So failing this, awaiting clarification.

              Show
              poltawski Dan Poltawski added a comment - Discussed with damyon that this does produce different urls (with the / then url will be an absolute rather than relative url). But Damyon contends that this doesn't actually change in redirect() because redirect handles it. So failing this, awaiting clarification.
              Hide
              rajeshtaneja Rajesh Taneja added a comment -

              Thanks Dan and Damyon,

              I couldn't reproduce this as well, but seems it make sense to have this in.
              As this is relative path moodle_url->out() = view.php?pageid=1&group=0
              Passing this to redirect create http://rajesh.moodle.local/m/mod/wiki/edit.php/../view.php?pageid=1&group=0
              which uses me() function to get name of the current script (mod/wiki/edit.php).

              As per phpDoc for me(), it will fail if needed global variables are not set.

              me() returns $ME, which is set as $rurl['fullpath'] (initialise_fullme()) and fullpath is set by setup_get_remote_url() which has a lot of TODO's and no fall-back. Seems setup_get_remote_url() is failing in Paul's setup and hence he can see this issue.

              IMHO this should be integrated and create another issue to investigate why setup_get_remote_url() could fail.

              Show
              rajeshtaneja Rajesh Taneja added a comment - Thanks Dan and Damyon, I couldn't reproduce this as well, but seems it make sense to have this in. As this is relative path moodle_url->out() = view.php?pageid=1&group=0 Passing this to redirect create http://rajesh.moodle.local/m/mod/wiki/edit.php/../view.php?pageid=1&group=0 which uses me() function to get name of the current script (mod/wiki/edit.php). As per phpDoc for me(), it will fail if needed global variables are not set. me() returns $ME, which is set as $rurl ['fullpath'] (initialise_fullme()) and fullpath is set by setup_get_remote_url() which has a lot of TODO's and no fall-back. Seems setup_get_remote_url() is failing in Paul's setup and hence he can see this issue. IMHO this should be integrated and create another issue to investigate why setup_get_remote_url() could fail.
              Hide
              poltawski Dan Poltawski added a comment -

              We're not so hot on integrating un reproducible problems. However I don't think this change will do harm, so to get this through the process we need to check that it hasn't caused any regressions. So i'll send it back to testing.

              But please make it much clearer in future when you've not been able to reproduce a problem that is not the road for success for us

              Show
              poltawski Dan Poltawski added a comment - We're not so hot on integrating un reproducible problems. However I don't think this change will do harm, so to get this through the process we need to check that it hasn't caused any regressions. So i'll send it back to testing. But please make it much clearer in future when you've not been able to reproduce a problem that is not the road for success for us
              Hide
              poltawski Dan Poltawski added a comment -

              Passes, no regressions.

              Show
              poltawski Dan Poltawski added a comment - Passes, no regressions.
              Hide
              stronk7 Eloy Lafuente (stronk7) added a comment -

              Your awesome contributions are now part of Moodle, your fav LMS out there.

              Closing this as fixed.

              Many thanks for all the hard work, ciao

              Show
              stronk7 Eloy Lafuente (stronk7) added a comment - Your awesome contributions are now part of Moodle, your fav LMS out there. Closing this as fixed. Many thanks for all the hard work, ciao

                People

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

                  Dates

                  • Created:
                    Updated:
                    Resolved:
                    Fix Release Date:
                    13/May/13