Moodle
  1. Moodle
  2. MDL-31316

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

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 2.0.7, 2.1, 2.2, 2.3
    • Fix Version/s: 2.1.5, 2.2.2
    • Component/s: Blocks, My home
    • Labels:
    • 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
    • Rank:
      37785

      Description

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

        Issue Links

          Activity

          Hide
          Ruslan Kabalin added a comment -

          Can be cleanly cherry-picked to all affected branches

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

          Thanks for providing a solution to this problem.

          Show
          Michael de Raadt added a comment - Thanks for providing a solution to this problem.
          Hide
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          Ruslan Kabalin added a comment -

          My pleasure Thanks for revision!

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

          Hello Ruslan,

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

          Show
          Rajesh Taneja added a comment - Hello Ruslan, I think your fix resolves MDL-30355 , can you please confirm this Thanks.
          Hide
          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
          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
          Ruslan Kabalin added a comment -

          Rebased on latest master

          Show
          Ruslan Kabalin added a comment - Rebased on latest master
          Hide
          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
          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
          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
          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
          Ruslan Kabalin added a comment -

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

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

          Integrated (21, 22 and master) thanks!

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

          My pleasure

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

          .

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

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

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

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

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

          Show
          Ruslan Kabalin added a comment -
          Hide
          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
          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: