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

Blocks controls on the site page are redirecting to /my when default home is My Moodle

    Details

    • Database:
      Any
    • Testing Instructions:
      Hide
      1. Log in as Admin go to Home ▶ Site administration ▶ Appearance ▶ Navigation
      2. Set "Default Home page for users" to 'My Moodle'
      3. Click the 'Site home' link
      4. Check you are presented with http://example/?redirect=0
      5. Click on any editing icon in any block
      6. You are being redirected to corresponding action (not http://example/my)
      Show
      Log in as Admin go to Home ▶ Site administration ▶ Appearance ▶ Navigation Set "Default Home page for users" to 'My Moodle' Click the 'Site home' link Check you are presented with http://example/?redirect=0 Click on any editing icon in any block You are being redirected to corresponding action (not http://example/my )
    • Affected Branches:
      MOODLE_20_STABLE, MOODLE_21_STABLE, MOODLE_22_STABLE, MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-31316-master-4

      Description

      As per subject. The reason is that redirect parameter is not being passed over and /index.php redirects to My Moodle.

        Gliffy Diagrams

          Attachments

            Issue Links

              Activity

              Hide
              kabalin Ruslan Kabalin added a comment -

              Can be cleanly cherry-picked to all affected branches

              Show
              kabalin Ruslan Kabalin added a comment - Can be cleanly cherry-picked to all affected branches
              Hide
              salvetore Michael de Raadt added a comment -

              Thanks for providing a solution to this problem.

              Show
              salvetore Michael de Raadt added a comment - Thanks for providing a solution to this problem.
              Hide
              kabalin Ruslan Kabalin added a comment -

              Made some changes, it appeared that previous fix was not complete. Also changed the title as the bug affects add new block dropdown form as well.

              Show
              kabalin Ruslan Kabalin added a comment - Made some changes, it appeared that previous fix was not complete. Also changed the title as the bug affects add new block dropdown form as well.
              Hide
              rajeshtaneja Rajesh Taneja added a comment -

              Hello Ruslan,

              Patch works fine, but seems we are not attacking the problem in right way. Problem is in logic for redirection (index.php line - 61). It will be nice to check bui_editid and if provided the don't redirect else follow the process.
              So, replacing

              else if ($CFG->defaulthomepage == HOMEPAGE_MY && optional_param('redirect', 1, PARAM_BOOL) === 1) {

              with

              else if ($CFG->defaulthomepage == HOMEPAGE_MY && optional_param('redirect', 1, PARAM_BOOL) === 1 && optional_param('bui_editid', -1, PARAM_INT) === -1) {

              will solve the issue.

              Show
              rajeshtaneja Rajesh Taneja added a comment - Hello Ruslan, Patch works fine, but seems we are not attacking the problem in right way. Problem is in logic for redirection (index.php line - 61). It will be nice to check bui_editid and if provided the don't redirect else follow the process. So, replacing else if ($CFG->defaulthomepage == HOMEPAGE_MY && optional_param('redirect', 1, PARAM_BOOL) === 1) { with else if ($CFG->defaulthomepage == HOMEPAGE_MY && optional_param('redirect', 1, PARAM_BOOL) === 1 && optional_param('bui_editid', -1, PARAM_INT) === -1) { will solve the issue.
              Hide
              kabalin Ruslan Kabalin added a comment - - edited

              Hi Rajesh, thanks for reply. I was thinking about this approach first, problem is that the bug affects all possible control actions including bui_addblock. One option will obviously be listing them all in the line you suggest, but it is not ideal. From the point of avoiding future problems when someone adds a new control element, the solution I suggest is somewhat better

              Another potential problem is that some actions have two steps and not only redirect from the line you mentioned: when you click on the action, it is accessing the page with the parameters, processing and then redirects from (pagelib.php:1211), I fix this via $PAGE->url definition in /index.php rather than doing another check at the point of redirection.

              Show
              kabalin Ruslan Kabalin added a comment - - edited Hi Rajesh, thanks for reply. I was thinking about this approach first, problem is that the bug affects all possible control actions including bui_addblock. One option will obviously be listing them all in the line you suggest, but it is not ideal. From the point of avoiding future problems when someone adds a new control element, the solution I suggest is somewhat better Another potential problem is that some actions have two steps and not only redirect from the line you mentioned: when you click on the action, it is accessing the page with the parameters, processing and then redirects from (pagelib.php:1211), I fix this via $PAGE->url definition in /index.php rather than doing another check at the point of redirection.
              Hide
              rajeshtaneja Rajesh Taneja added a comment -

              Thanks Ruslan,

              Sorry, I didn't check rest of the options. Although it might be nice to check for posted optional param, and if yes then don't redirect, else redirect.

              If you still wish to go with your solution, then probably you don't need to add a separate check as redirect happens when redirect (optional_param ) is 1. As you are adding "redirect = 0" in block url, you might not require changes in index.php

              Show
              rajeshtaneja Rajesh Taneja added a comment - Thanks Ruslan, Sorry, I didn't check rest of the options. Although it might be nice to check for posted optional param, and if yes then don't redirect, else redirect. If you still wish to go with your solution, then probably you don't need to add a separate check as redirect happens when redirect (optional_param ) is 1. As you are adding "redirect = 0" in block url, you might not require changes in index.php
              Hide
              kabalin Ruslan Kabalin added a comment -

              Did not quite get what you mean by "posted optional param"

              If you still wish to go with your solution, then probably you don't need to add a separate check as redirect happens when redirect (optional_param ) is 1. As you are adding "redirect = 0" in block url, you might not require changes in index.php

              I have just edited my comment above adding an explanation just before you posted this, sorry.

              Show
              kabalin Ruslan Kabalin added a comment - Did not quite get what you mean by "posted optional param" If you still wish to go with your solution, then probably you don't need to add a separate check as redirect happens when redirect (optional_param ) is 1. As you are adding "redirect = 0" in block url, you might not require changes in index.php I have just edited my comment above adding an explanation just before you posted this, sorry.
              Hide
              rajeshtaneja Rajesh Taneja added a comment -

              With optional params, I meant white list of parameters which should not redirect user to home page.
              Anyways, explanation make sense and patch looks spot-on

              Thanks Ruslan for working on it, Pushing it for integration review.

              Show
              rajeshtaneja Rajesh Taneja added a comment - With optional params, I meant white list of parameters which should not redirect user to home page. Anyways, explanation make sense and patch looks spot-on Thanks Ruslan for working on it, Pushing it for integration review.
              Hide
              kabalin Ruslan Kabalin added a comment -

              My pleasure Thanks for revision!

              Show
              kabalin Ruslan Kabalin added a comment - My pleasure Thanks for revision!
              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
              rajeshtaneja Rajesh Taneja added a comment -

              Hello Ruslan,

              I think your fix resolves MDL-30355, can you please confirm this
              Thanks.

              Show
              rajeshtaneja Rajesh Taneja added a comment - Hello Ruslan, I think your fix resolves MDL-30355 , can you please confirm this Thanks.
              Hide
              kabalin Ruslan Kabalin added a comment - - edited

              > I think your fix resolves MDL-30355, can you please confirm this

              Not able to confirm, because I can't replicate it. It works for me correctly without my patch :/

              Show
              kabalin Ruslan Kabalin added a comment - - edited > I think your fix resolves MDL-30355 , can you please confirm this Not able to confirm, because I can't replicate it. It works for me correctly without my patch :/
              Hide
              kabalin Ruslan Kabalin added a comment -

              Rebased on latest master

              Show
              kabalin Ruslan Kabalin added a comment - Rebased on latest master
              Hide
              stronk7 Eloy Lafuente (stronk7) added a comment -

              Hi Ruslan,

              looking at your changes they look perfect, so you add the redirect=0 where necessary @:

              A- The /index.php $PAGE url.
              B- All the block controls.
              C- The add block drop-down.

              Some comments:

              1) By setting A... doesn't that cause B and C to be 100% unnecessary? As far as B and C get the the $PAGE url... and it already contains "redirect=0" ... do we need the param being checked and added again?

              2) If finally there is some good reason to keep B) and C) implemented... then I guess that the detection should add one more condition, checking that the url is /index.php. Else I can imagine problems with other pages also using "redirect=0" for any purpose (note I don't know if there are more pages using "redirect=0" for anything, just imagining it).

              Thoughts? Ciao

              Show
              stronk7 Eloy Lafuente (stronk7) added a comment - Hi Ruslan, looking at your changes they look perfect, so you add the redirect=0 where necessary @: A- The /index.php $PAGE url. B- All the block controls. C- The add block drop-down. Some comments: 1) By setting A... doesn't that cause B and C to be 100% unnecessary? As far as B and C get the the $PAGE url... and it already contains "redirect=0" ... do we need the param being checked and added again? 2) If finally there is some good reason to keep B) and C) implemented... then I guess that the detection should add one more condition, checking that the url is /index.php. Else I can imagine problems with other pages also using "redirect=0" for any purpose (note I don't know if there are more pages using "redirect=0" for anything, just imagining it). Thoughts? Ciao
              Hide
              kabalin Ruslan Kabalin added a comment -

              Hello Eloy,

              1) By setting A... doesn't that cause B and C to be 100% unnecessary? As far as B and C get the the $PAGE url... and it already contains "redirect=0" ... do we need the param being checked and added again?

              Regarding B and C, you are right, they can be removed. Shame on me, I have not spotted it and offered less efficient solution. 'A' was the last change I did and it was against the second redirection when action is performed, but I did not realise that first two changes could be removed. Thanks for pointing that out. I will push updated version in a moment.

              Thanks again, Ruslan

              Show
              kabalin Ruslan Kabalin added a comment - Hello Eloy, 1) By setting A... doesn't that cause B and C to be 100% unnecessary? As far as B and C get the the $PAGE url... and it already contains "redirect=0" ... do we need the param being checked and added again? Regarding B and C, you are right, they can be removed. Shame on me, I have not spotted it and offered less efficient solution. 'A' was the last change I did and it was against the second redirection when action is performed, but I did not realise that first two changes could be removed. Thanks for pointing that out. I will push updated version in a moment. Thanks again, Ruslan
              Hide
              kabalin Ruslan Kabalin added a comment -

              Branch has been updated, I removed B and C as per Eloy's suggestion. Tested as well.

              Show
              kabalin Ruslan Kabalin added a comment - Branch has been updated, I removed B and C as per Eloy's suggestion. Tested as well.
              Hide
              stronk7 Eloy Lafuente (stronk7) added a comment -

              Integrated (21, 22 and master) thanks!

              Show
              stronk7 Eloy Lafuente (stronk7) added a comment - Integrated (21, 22 and master) thanks!
              Hide
              kabalin Ruslan Kabalin added a comment -

              My pleasure

              Show
              kabalin Ruslan Kabalin added a comment - My pleasure
              Hide
              ifaaonline Pooja Jain added a comment - - edited

              .

              Show
              ifaaonline Pooja Jain added a comment - - edited .
              Hide
              rajeshtaneja Rajesh Taneja added a comment -

              grrrr...
              Should have spotted that. Thanks Eloy.

              Show
              rajeshtaneja Rajesh Taneja added a comment - grrrr... Should have spotted that. Thanks Eloy.
              Hide
              ankit_frenz Ankit Agarwal added a comment -

              Hi,
              This is working as expected.
              Thanks for fixing this.
              Passing
              Thanks

              Show
              ankit_frenz Ankit Agarwal added a comment - Hi, This is working as expected. Thanks for fixing this. Passing Thanks
              Hide
              kabalin Ruslan Kabalin added a comment -

              Show
              kabalin Ruslan Kabalin added a comment -
              Hide
              stronk7 Eloy Lafuente (stronk7) added a comment -

              A bit later this week, but finally your changes have been accepted and are now available in all the upstream git/cvs servers.

              Many thanks & ciao

              Show
              stronk7 Eloy Lafuente (stronk7) added a comment - A bit later this week, but finally your changes have been accepted and are now available in all the upstream git/cvs servers. Many thanks & ciao

                People

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

                  Dates

                  • Created:
                    Updated:
                    Resolved:
                    Fix Release Date:
                    12/Mar/12