Moodle
  1. Moodle
  2. MDL-34548

Remove get_context_instance() from core

    Details

    • Type: Task Task
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Won't Fix
    • Affects Version/s: 2.4
    • Fix Version/s: STABLE backlog
    • Component/s: Libraries
    • Labels:

      Description

      The function was deprecated in 2.1 and removed from core with a deprecated warning in 2.4
      Refer MDL-33061 for further details.

      It should be safe to remove the function from core in 2.6

      Thanks

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            Ankit Agarwal added a comment -

            We might have to move this to a later version depending on when it is decided to integrate MDL-34472

            Thanks

            Show
            Ankit Agarwal added a comment - We might have to move this to a later version depending on when it is decided to integrate MDL-34472 Thanks
            Hide
            Martin Dougiamas added a comment -

            I don't think there is a good case to remove this from core totally. As long as we have DEVELOPER warnings to help them upgrade to the new ones there is no reason to remove this function completely as long as it works.

            Show
            Martin Dougiamas added a comment - I don't think there is a good case to remove this from core totally. As long as we have DEVELOPER warnings to help them upgrade to the new ones there is no reason to remove this function completely as long as it works.
            Hide
            Ankit Agarwal added a comment -

            We decided never to remove this from core. It will always stay with the debugging notice.
            closing this issue as won't fix.

            Show
            Ankit Agarwal added a comment - We decided never to remove this from core. It will always stay with the debugging notice. closing this issue as won't fix.
            Hide
            Eloy Lafuente (stronk7) added a comment - - edited

            While I agree with Dan's main points @ MDL-34472:

            • there are many uses of it by non-core
            • codechecker should be able to look for them.
            • we cannot delete it "suddenly".
            • ....

            I simply think that "deprecating forever" is not a good principle. I don't mind if it's Moodle 3.0, or Moodle 2.112. But deprecation must have an end date/release.

            Surely it's our fault that we missed to document those deprecations properly in 2.2, but well, you know how the initial 2.x cycles were.

            I really take this as a second opportunity do the things properly, with "official" deprecation of this function (and others) with documentation, checkers and a target version for removal (no matter how distant it is).

            But "deprecating forever", no. Sorry my -99 for that.

            Ciao

            Edited: 2.2

            Show
            Eloy Lafuente (stronk7) added a comment - - edited While I agree with Dan's main points @ MDL-34472 : there are many uses of it by non-core codechecker should be able to look for them. we cannot delete it "suddenly". .... I simply think that "deprecating forever" is not a good principle. I don't mind if it's Moodle 3.0, or Moodle 2.112. But deprecation must have an end date/release. Surely it's our fault that we missed to document those deprecations properly in 2.2, but well, you know how the initial 2.x cycles were. I really take this as a second opportunity do the things properly, with "official" deprecation of this function (and others) with documentation, checkers and a target version for removal (no matter how distant it is). But "deprecating forever", no. Sorry my -99 for that. Ciao Edited: 2.2
            Hide
            Dan Poltawski added a comment -

            Well, if you want to frame it another way, I don't think these accessors should've been deprecated and should continue to be fully supported.

            What would change my mind is if someone could answer this question with something meaningful:

            > The benefit of changing every piece of code to use context_module::instance() over get_context_instance(CONTEXT_MODULE,) is?

            The way it looks to me is:

            1. Developer creates newer (slightly nicer) API
            2. Developer deprecates old way (with no clear benefit, or discussion)
            3. Whole community has to do a lot of unnecessary work to update to new style

            (In the middle of writing this, Damyon and I had a discussion about consistency for new developers, being tied down by legacy, which sure mellowed me whilst posting this )

            Show
            Dan Poltawski added a comment - Well, if you want to frame it another way, I don't think these accessors should've been deprecated and should continue to be fully supported. What would change my mind is if someone could answer this question with something meaningful: > The benefit of changing every piece of code to use context_module::instance() over get_context_instance(CONTEXT_MODULE,) is? The way it looks to me is: Developer creates newer (slightly nicer) API Developer deprecates old way (with no clear benefit, or discussion) Whole community has to do a lot of unnecessary work to update to new style (In the middle of writing this, Damyon and I had a discussion about consistency for new developers, being tied down by legacy, which sure mellowed me whilst posting this )
            Hide
            Damyon Wiese added a comment -

            IMO - we should be safe to remove deprecations when all supported versions of Moodle include the new function (This is longer than the currently documented period). This means plugin developers can use one function for all supported versions of Moodle.

            Show
            Damyon Wiese added a comment - IMO - we should be safe to remove deprecations when all supported versions of Moodle include the new function (This is longer than the currently documented period). This means plugin developers can use one function for all supported versions of Moodle.
            Hide
            Sam Hemelryk added a comment -

            Damyon raises a good point.

            Myself I think either we commit to deprecating it or we re-invoke it as a usable function. Indefinite limbo sends the message that you can continue using deprecated things in your code and that's not the message I think we want to send at all.
            You could talk about the pros and cons of leaving it there indefinitely but in doing so you introduce another point of confusion for people.
            Lets make a commitment one way or the other.

            Myself, usually there are core deprecations with every major release that are going to require a plugin to be updated to avoid debugging notices and stay up to times.
            I'm well aware that not everyone does this of course, there's no doubt a lot of unloved plugins in the plugins database.
            But this deprecation is easy at the moment and a smart developer could easily upgrade their code for this one aspect using sed.
            Lets bite the bullet and require plugin developers to make the change.

            So from me.. +1 to deprecating it and -1 to deprecating it forever.

            Show
            Sam Hemelryk added a comment - Damyon raises a good point. Myself I think either we commit to deprecating it or we re-invoke it as a usable function. Indefinite limbo sends the message that you can continue using deprecated things in your code and that's not the message I think we want to send at all. You could talk about the pros and cons of leaving it there indefinitely but in doing so you introduce another point of confusion for people. Lets make a commitment one way or the other. Myself, usually there are core deprecations with every major release that are going to require a plugin to be updated to avoid debugging notices and stay up to times. I'm well aware that not everyone does this of course, there's no doubt a lot of unloved plugins in the plugins database. But this deprecation is easy at the moment and a smart developer could easily upgrade their code for this one aspect using sed. Lets bite the bullet and require plugin developers to make the change. So from me.. +1 to deprecating it and -1 to deprecating it forever.

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved: