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

          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