Details

    • Type: Sub-task
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.3.2
    • Fix Version/s: 2.4
    • Component/s: Course
    • Labels:

      Description

      In MDL-22732 the code to add module/resource in Settings menu was commented but not removed.
      We need to remove completely unused code and callback_xxx_request_key() from course formats

        Gliffy Diagrams

          Attachments

            Activity

            Hide
            salvetore Michael de Raadt added a comment -

            We can't simply remove the function add_course_editing_links(). This may cause regressions. We have to do this following the proper deprecation process. The function needs to be marked as deprecated now then removed in 2.6. You will need to make another issue, linked to this one, as a sub-task of MDL-34434.

            Show
            salvetore Michael de Raadt added a comment - We can't simply remove the function add_course_editing_links(). This may cause regressions. We have to do this following the proper deprecation process. The function needs to be marked as deprecated now then removed in 2.6. You will need to make another issue, linked to this one, as a sub-task of MDL-34434 .
            Hide
            marina Marina Glancy added a comment -

            This is the protected function in the class, it can not be used anywhere else.
            Sam, you were doing MDL-22732, were there any serious reasons not to remove this function completely at that time?

            Show
            marina Marina Glancy added a comment - This is the protected function in the class, it can not be used anywhere else. Sam, you were doing MDL-22732 , were there any serious reasons not to remove this function completely at that time?
            Hide
            salvetore Michael de Raadt added a comment -

            As there was not specific documentation about deprecation, I quickly wrote some.

            http://docs.moodle.org/dev/Deprecation

            Show
            salvetore Michael de Raadt added a comment - As there was not specific documentation about deprecation, I quickly wrote some. http://docs.moodle.org/dev/Deprecation
            Hide
            samhemelryk Sam Hemelryk added a comment -

            Hi Marina,

            Martin requested the code be left there so that should anyone else want to make use of it they could.
            No reason other than that for the code to be there.

            To be truthful I see no reason not to remove it from master now, MDL-22732 was a long time ago and I don't believe there have been any requests to have it put back.
            Of course our normal procedure from deprecating + removing a function should be followed here just in case, as Michael points out it still has the potential to cause regressions, it being a protected method would still allow someone to override the class and make use of it.
            My +1 to add a debugging notice informing any developers this function will be removed in 2.5, integrating that in this issue, and opening an issue targeting 2.5 to remove that function entirely.

            Cheers
            Sam

            Show
            samhemelryk Sam Hemelryk added a comment - Hi Marina, Martin requested the code be left there so that should anyone else want to make use of it they could. No reason other than that for the code to be there. To be truthful I see no reason not to remove it from master now, MDL-22732 was a long time ago and I don't believe there have been any requests to have it put back. Of course our normal procedure from deprecating + removing a function should be followed here just in case, as Michael points out it still has the potential to cause regressions, it being a protected method would still allow someone to override the class and make use of it. My +1 to add a debugging notice informing any developers this function will be removed in 2.5, integrating that in this issue, and opening an issue targeting 2.5 to remove that function entirely. Cheers Sam
            Hide
            salvetore Michael de Raadt added a comment -

            Thanks for the extra info there, Sam.

            Show
            salvetore Michael de Raadt added a comment - Thanks for the extra info there, Sam.
            Hide
            marina Marina Glancy added a comment - - edited

            I still vote for removing it completely now. I uncommented to check this function and realised that it does not work anyway:

            1. https://github.com/moodle/moodle/blob/master/lib/navigationlib.php#L3753
            for topics and weeks formats Dan removed the callback functions in 2.3, which now throws a fatal error here (file exists and function does not)

            2. https://github.com/moodle/moodle/blob/master/lib/navigationlib.php#L3788
            $resources is undefined (even for scorm/social format)

            Show
            marina Marina Glancy added a comment - - edited I still vote for removing it completely now. I uncommented to check this function and realised that it does not work anyway: 1. https://github.com/moodle/moodle/blob/master/lib/navigationlib.php#L3753 for topics and weeks formats Dan removed the callback functions in 2.3, which now throws a fatal error here (file exists and function does not) 2. https://github.com/moodle/moodle/blob/master/lib/navigationlib.php#L3788 $resources is undefined (even for scorm/social format)
            Hide
            poltawski 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
            poltawski 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
            poltawski Dan Poltawski added a comment -

            Hi Marina,

            I agree with you that it is best to remove this function given that it is completely broken. If we keep it there we are not benefiting third party developers at all, which afterall is the point of the deprecation process.

            However, I do think we should document the change, noting that it was unused and completely broken. Could you add a brief note to lib/upgrade.txt about this?

            Show
            poltawski Dan Poltawski added a comment - Hi Marina, I agree with you that it is best to remove this function given that it is completely broken. If we keep it there we are not benefiting third party developers at all, which afterall is the point of the deprecation process. However, I do think we should document the change, noting that it was unused and completely broken. Could you add a brief note to lib/upgrade.txt about this?
            Hide
            marina Marina Glancy added a comment -

            Dan I added another commit

            Show
            marina Marina Glancy added a comment - Dan I added another commit
            Hide
            poltawski Dan Poltawski added a comment -

            Integrated, thanks.

            Show
            poltawski Dan Poltawski added a comment - Integrated, thanks.
            Hide
            poltawski Dan Poltawski added a comment -

            Congratulations, you've done it!

            Nf n erjneq sbe fhpprfshy vagrtengvba vagb guvf jrrxf eryrnfr, V pna abj qvfpybfr gb lbh gur rkvfgnapr bs shapgvba fge_ebg13(), gb tb va lbhe gbbyxvg nybat jvgu uggc://cuc.arg/znahny/ra/shapgvba.tmtrgff.cuc

            Cyrnfr qb abg nyybj guvf vasbezngvba gb cnff shegure.

            Show
            poltawski Dan Poltawski added a comment - Congratulations, you've done it! Nf n erjneq sbe fhpprfshy vagrtengvba vagb guvf jrrxf eryrnfr, V pna abj qvfpybfr gb lbh gur rkvfgnapr bs shapgvba fge_ebg13(), gb tb va lbhe gbbyxvg nybat jvgu uggc://cuc.arg/znahny/ra/shapgvba.tmtrgff.cuc Cyrnfr qb abg nyybj guvf vasbezngvba gb cnff shegure.

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  3/Dec/12