Moodle
  1. Moodle
  2. MDL-28583

redirect() shows ugly error if called after page output has been made

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.2
    • Fix Version/s: 2.4
    • Component/s: Course, SCORM
    • Labels:
    • Rank:
      18264

      Description

      error seen:

      You should really redirect before you start page output

      line 560 of /lib/outputrenderers.php: call to debugging()
      line 2497 of /lib/weblib.php: call to core_renderer->redirect_message()
      line 714 of /mod/scorm/locallib.php: call to redirect()
      line 14 of /course/format/scorm/format.php: call to scorm_course_format_display()
      line 240 of /course/view.php: call to require()

        Issue Links

          Activity

          Hide
          Dan Marsden added a comment -

          not sure what we can do with this....

          redirect is the right way to redirect users.. but inside a course format the output is already printed to screen before getting to the course format page.

          Show
          Dan Marsden added a comment - not sure what we can do with this.... redirect is the right way to redirect users.. but inside a course format the output is already printed to screen before getting to the course format page.
          Hide
          Dan Marsden added a comment - - edited

          aparup - I saw this a while back but couldn't see a way to fix it unless we added a function to course formats own lib.php eg course/format/scorm/lib.php - something like "checkredirect()" then in course/view.php before any $PAGE stuff is done it could check to see if the courseformat wanted to do something before it printed anything to screen.

          in any case - I have no plans to spend time on this - happy for someone else to take it!

          Show
          Dan Marsden added a comment - - edited aparup - I saw this a while back but couldn't see a way to fix it unless we added a function to course formats own lib.php eg course/format/scorm/lib.php - something like "checkredirect()" then in course/view.php before any $PAGE stuff is done it could check to see if the courseformat wanted to do something before it printed anything to screen. in any case - I have no plans to spend time on this - happy for someone else to take it!
          Hide
          Aparup Banerjee added a comment -

          no worries Dan,it is minorish (& i couldn't find a duplicate). at some pint (and point) it'll get triaged and into the back log for anyone else to grab.

          Show
          Aparup Banerjee added a comment - no worries Dan,it is minorish (& i couldn't find a duplicate). at some pint (and point) it'll get triaged and into the back log for anyone else to grab.
          Hide
          Michael de Raadt added a comment -

          This is happening all over the place. It's really annoying. I'm not sure we should be giving this as an error. Sure it's sub-optimal, but it's not really an error.

          Show
          Michael de Raadt added a comment - This is happening all over the place. It's really annoying. I'm not sure we should be giving this as an error. Sure it's sub-optimal, but it's not really an error.
          Hide
          Dan Marsden added a comment -

          this annoyed me today so I decided to push a patch to remove it - pushing up for peer review.

          Show
          Dan Marsden added a comment - this annoyed me today so I decided to push a patch to remove it - pushing up for peer review.
          Hide
          Aparup Banerjee added a comment -

          Thanks Dan , fix looks fine.

          hm,I can't help but think there's a better way.

          Setting Sam, the born renderer, as integrator. have a feeling he'll have some useful input here.

          Show
          Aparup Banerjee added a comment - Thanks Dan , fix looks fine. hm,I can't help but think there's a better way. Setting Sam, the born renderer, as integrator. have a feeling he'll have some useful input here.
          Hide
          Dan Marsden added a comment -

          yeah - happy for someone to reject it, it just annoyed me today and it was an easy thing to remove so I thought I'd try to sneak it in....

          Show
          Dan Marsden added a comment - yeah - happy for someone to reject it, it just annoyed me today and it was an easy thing to remove so I thought I'd try to sneak it in....
          Hide
          Dan Poltawski 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
          Dan Poltawski 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
          Petr Škoda added a comment -

          I disagree with this change, please stop using redirect after you print out anything, those delayed redirects are extremely annoying and not accessible. my -1

          Show
          Petr Škoda added a comment - I disagree with this change, please stop using redirect after you print out anything, those delayed redirects are extremely annoying and not accessible. my -1
          Hide
          Dan Marsden added a comment -

          yeah - quite happy for this to be rejected - problem is there is no way for a course format to redirect before page output so there's no "core supported" way of making it work. I'd imagine there are other cases in core code where redirect is not possible before page output either and this is a pretty annoying error/warning to see in core code.

          Show
          Dan Marsden added a comment - yeah - quite happy for this to be rejected - problem is there is no way for a course format to redirect before page output so there's no "core supported" way of making it work. I'd imagine there are other cases in core code where redirect is not possible before page output either and this is a pretty annoying error/warning to see in core code.
          Hide
          Aparup Banerjee added a comment -

          perhaps we should have a core system (weblib or navigationlib?) of redirecting, this would allow redirecting to be possible even at the later output stages. i believe this might be possible with renderers used prevalently now.

          so we could have :

          • */db/redirects.php
          • all the places using returnid etc in urls could use this instead.
          • navigation lib might be extendable to incorporate this.
          • with renderers, we should be able to skip having to output any http to the browser and abort rendering (not sure if i'm right here - Petr?) in fact, upon any redirect "$PAGE->redirect() //old redirect() calls ?" , we can totally reset the renderers and redirect within apache/php.
          • the page to be redirect to... becomes the page that is actually first output. We can always add in a 'you have been redirected' message + reasons or dialog etc.
          Show
          Aparup Banerjee added a comment - perhaps we should have a core system (weblib or navigationlib?) of redirecting, this would allow redirecting to be possible even at the later output stages. i believe this might be possible with renderers used prevalently now. so we could have : */db/redirects.php all the places using returnid etc in urls could use this instead. navigation lib might be extendable to incorporate this. with renderers, we should be able to skip having to output any http to the browser and abort rendering (not sure if i'm right here - Petr?) in fact, upon any redirect "$PAGE->redirect() //old redirect() calls ?" , we can totally reset the renderers and redirect within apache/php. the page to be redirect to... becomes the page that is actually first output. We can always add in a 'you have been redirected' message + reasons or dialog etc.
          Hide
          Petr Škoda added a comment -

          Course format is one of the oldest parts of moodle, it should imo control the whole course page since the very start of the course/view.php, the fact that it does not control output indicates that the API in not good enough for current usage - why not fix it instead?

          What other places should be allowed to redirect via PHP from the middle of an already started page?

          Show
          Petr Škoda added a comment - Course format is one of the oldest parts of moodle, it should imo control the whole course page since the very start of the course/view.php, the fact that it does not control output indicates that the API in not good enough for current usage - why not fix it instead? What other places should be allowed to redirect via PHP from the middle of an already started page?
          Hide
          Sam Hemelryk added a comment -

          Hi guys,

          I just popped this up now, had a look at the change and have had a read of what's been said here.
          Personally, I agree with Petr. I don't think should land sorry Dan.

          That debugging notice is there to discourage people from making redirects after having started output.
          Having it there hopefully will aid people in writing code that separates out action from display, and will help prevent people from adding redirects within the content of their pages.
          I understand that there are still the odd places in core where it is being done, and that in some of those places its unavoidable (as is the case with course formats obviously).
          However I think the intention of this debugging notice is still in the right direction and I think it would be a shame to remove it now.

          Petr is right in suggesting that instead we should focus on area causing the issue, although I know that such a change would be huge and would take months of planning and development.
          But it is still correct, really course format should be given control of the page within course/view.php. The whole API is due to change, especially when looking at everything that people are doing (or trying to do) within course formats.

          Apu, in regards to introducing a new mechanism for redirecting within output I don't think it is such a great idea.
          I think the best idea is to not redirect during output.
          Preparing the page, initialising output, and generating html are costly to a page. In avoiding redirects within output we are achieving better separation of actions and output; that in turn brings several advantages including performance through avoiding unneeded processing.

          Anyway enough from me.

          I am going to reopen this so that the discussion may continue.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi guys, I just popped this up now, had a look at the change and have had a read of what's been said here. Personally, I agree with Petr. I don't think should land sorry Dan. That debugging notice is there to discourage people from making redirects after having started output. Having it there hopefully will aid people in writing code that separates out action from display, and will help prevent people from adding redirects within the content of their pages. I understand that there are still the odd places in core where it is being done, and that in some of those places its unavoidable (as is the case with course formats obviously). However I think the intention of this debugging notice is still in the right direction and I think it would be a shame to remove it now. Petr is right in suggesting that instead we should focus on area causing the issue, although I know that such a change would be huge and would take months of planning and development. But it is still correct, really course format should be given control of the page within course/view.php. The whole API is due to change, especially when looking at everything that people are doing (or trying to do) within course formats. Apu, in regards to introducing a new mechanism for redirecting within output I don't think it is such a great idea. I think the best idea is to not redirect during output. Preparing the page, initialising output, and generating html are costly to a page. In avoiding redirects within output we are achieving better separation of actions and output; that in turn brings several advantages including performance through avoiding unneeded processing. Anyway enough from me. I am going to reopen this so that the discussion may continue. Cheers Sam
          Hide
          Dan Marsden added a comment -

          Fine by me - the fact that pushing this for integration has forced discussion on the issue is a good thing IMO - would be really nice to see course formats improved.

          My vote would be to close this as "won't fix" and any discussion around rewriting course formats should happen separately.

          Show
          Dan Marsden added a comment - Fine by me - the fact that pushing this for integration has forced discussion on the issue is a good thing IMO - would be really nice to see course formats improved. My vote would be to close this as "won't fix" and any discussion around rewriting course formats should happen separately.
          Hide
          CiBoT added a comment -

          Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

          Show
          CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
          Hide
          Aparup Banerjee added a comment -

          yes Petr, agreed that course formats itself needs improvement. I can't think of any other need for the redirect mechanism. Also yes, it is very costly to do that Sam. Better we fix any reason for the redirect abuse.

          I've opened up MDL-34773 to address this and closing this as deferred.

          Thanks all for the trigger, discussion and direction here .

          Show
          Aparup Banerjee added a comment - yes Petr, agreed that course formats itself needs improvement. I can't think of any other need for the redirect mechanism. Also yes, it is very costly to do that Sam. Better we fix any reason for the redirect abuse. I've opened up MDL-34773 to address this and closing this as deferred. Thanks all for the trigger, discussion and direction here .
          Hide
          Aparup Banerjee added a comment -

          work is being done here (MDL-35218) for this.

          Show
          Aparup Banerjee added a comment - work is being done here ( MDL-35218 ) for this.
          Hide
          Marina Glancy added a comment -

          reopening since it has become possible to do in 2.4

          Show
          Marina Glancy added a comment - reopening since it has become possible to do in 2.4
          Hide
          Marina Glancy added a comment -

          to integrators: the branch also contains commits from other issues currently in integration

          Show
          Marina Glancy added a comment - to integrators: the branch also contains commits from other issues currently in integration
          Hide
          Sam Hemelryk added a comment -

          Thanks Marina, this has been integrated now

          Show
          Sam Hemelryk added a comment - Thanks Marina, this has been integrated now
          Hide
          Sam Hemelryk added a comment -

          Marking as passed as the QA issue MDLQA-4714 will see this tested.

          Show
          Sam Hemelryk added a comment - Marking as passed as the QA issue MDLQA-4714 will see this tested.
          Hide
          Dan Poltawski added a comment -

          Congratulations! Another bug solved.. only another 7330 to go, thanks for contributing to contributing to 0.8% of all bugs being fixed this week!

          ciao
          Dan

          Show
          Dan Poltawski added a comment - Congratulations! Another bug solved.. only another 7330 to go, thanks for contributing to contributing to 0.8% of all bugs being fixed this week! ciao Dan

            People

            • Votes:
              1 Vote for this issue
              Watchers:
              5 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: