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:

      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()

        Gliffy Diagrams

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