Details

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

      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

        Activity

        Hide
        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
        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 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 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
        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
        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
        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
        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
        Michael de Raadt added a comment -

        Thanks for the extra info there, Sam.

        Show
        Michael de Raadt added a comment - Thanks for the extra info there, Sam.
        Hide
        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 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
        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
        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
        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 Glancy added a comment -

        Dan I added another commit

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

        Integrated, thanks.

        Show
        Dan Poltawski added a comment - Integrated, thanks.
        Hide
        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
        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: