Moodle
  1. Moodle
  2. MDL-40299

migrate textlib to core_ prefix and auto class loader

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.6
    • Fix Version/s: 2.6
    • Component/s: Libraries
    • Rank:
      51071

      Description

      reasons:

      • memory use
      • coding style improvements

      With full BC of course...

        Issue Links

          Activity

          Hide
          Petr Škoda added a comment -

          to integrators: feel free to reject, I am testing what is acceptable as our future core direction.

          note: if this gets accepted we may start migrating uses of texlib and collator lib, I did not want this to collide with other issues for now.

          Show
          Petr Škoda added a comment - to integrators: feel free to reject, I am testing what is acceptable as our future core direction. note: if this gets accepted we may start migrating uses of texlib and collator lib, I did not want this to collide with other issues for now.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          +1

          I like this, so has my +1 but really want/need to hold it till next week in order to have some discussion about how to document/explain the way we are going to perform all these moves. It implies modifying a lot of code/calls and we need to improve documentation, deprecation detection in some way.

          Thanks for the exemplary case, Petr!

          Show
          Eloy Lafuente (stronk7) added a comment - +1 I like this, so has my +1 but really want/need to hold it till next week in order to have some discussion about how to document/explain the way we are going to perform all these moves. It implies modifying a lot of code/calls and we need to improve documentation, deprecation detection in some way. Thanks for the exemplary case, Petr!
          Hide
          Sam Hemelryk added a comment -

          +1 from me.

          I think this is a very smart move and definitely its good idea to start to ascertain the boundaries for conversion.
          Certainly there need to be limitations, we don't want to end up with a multitude of deprecated files.
          This is a great example of a conversion that makes a lot of sense. The code is used sporadically throughout Moodle, it is essential but hard to predict on what pages we will or won't need it.
          Having it autoloaded is a perfect solution here.

          I have thought about the cache classes a bit in the past week, I suppose to get other people thinking about situations, and to perhaps to spur on discussion I'll share them here:

          1. Classes have been organised closely to but not exactly conforming to the requirements for autoloading. Is it worth smoothing this out and then relying on autoloading?
          2. I'd like to see cache_config_writer converted to make use of autoloading. It's currently within cache/locallib.php along with other code, it is not used often and I think would be a perfect candidate for conversion.
          Show
          Sam Hemelryk added a comment - +1 from me. I think this is a very smart move and definitely its good idea to start to ascertain the boundaries for conversion. Certainly there need to be limitations, we don't want to end up with a multitude of deprecated files. This is a great example of a conversion that makes a lot of sense. The code is used sporadically throughout Moodle, it is essential but hard to predict on what pages we will or won't need it. Having it autoloaded is a perfect solution here. I have thought about the cache classes a bit in the past week, I suppose to get other people thinking about situations, and to perhaps to spur on discussion I'll share them here: Classes have been organised closely to but not exactly conforming to the requirements for autoloading. Is it worth smoothing this out and then relying on autoloading? I'd like to see cache_config_writer converted to make use of autoloading. It's currently within cache/locallib.php along with other code, it is not used often and I think would be a perfect candidate for conversion.
          Hide
          Petr Škoda added a comment - - edited

          Thanks for the feedback! We could probably discuss this during the meeting on Thursday (is HQ going to be back already?).

          Show
          Petr Škoda added a comment - - edited Thanks for the feedback! We could probably discuss this during the meeting on Thursday (is HQ going to be back already?).
          Hide
          Eloy Lafuente (stronk7) 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
          Eloy Lafuente (stronk7) 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
          Tim Hunt added a comment -

          Generally this is a good thing, but ...

          Surely it is not necessary to break backwards compatibility immediately. We should (for at least one release) define classes

          /** @deprecated ... */
          class textlib extends core_text {
          }
          /** @deprecated ... */
          class collatorlib extends core_collator {
          }

          (Should we override all the methods to output debugging messages before calling the parent class?)

          Also, the debugging message:

          debugging('Do not include textlib.class.php directly, it is now using automatic class loading.');

          This makes classic mistake. When a developer is upgrading their code, it is useless to just tell them what not to do. They are already frustrated because a core change broke their previously working code. To avoid just making them more frustrated, you must tell me the right thing to do instead. Something like

          textlib:: has been replaced by core_text::, and collatorlib:: has been replaced by core_collator::*. Please convert your code to the new class names. Also, you no longer need to include textlib.php yourself, automatic class loading does that for you.

          Show
          Tim Hunt added a comment - Generally this is a good thing, but ... Surely it is not necessary to break backwards compatibility immediately. We should (for at least one release) define classes /** @deprecated ... */ class textlib extends core_text { } /** @deprecated ... */ class collatorlib extends core_collator { } (Should we override all the methods to output debugging messages before calling the parent class?) Also, the debugging message: debugging('Do not include textlib.class.php directly, it is now using automatic class loading.'); This makes classic mistake. When a developer is upgrading their code, it is useless to just tell them what not to do. They are already frustrated because a core change broke their previously working code. To avoid just making them more frustrated, you must tell me the right thing to do instead. Something like textlib:: has been replaced by core_text:: , and collatorlib:: has been replaced by core_collator::*. Please convert your code to the new class names. Also, you no longer need to include textlib.php yourself, automatic class loading does that for you.
          Hide
          Petr Škoda added a comment -

          Hmmm, my patch is backwards compatible, that is the whole point to show how to do the autoloading of both old and new classes.

          The debugging message is also valid, nobody is supposed to include textlib.class.php directly in any version because it was included by default from lib/setup.php

          Show
          Petr Škoda added a comment - Hmmm, my patch is backwards compatible, that is the whole point to show how to do the autoloading of both old and new classes. The debugging message is also valid, nobody is supposed to include textlib.class.php directly in any version because it was included by default from lib/setup.php
          Hide
          Damyon Wiese added a comment -

          Thanks for the patch Petr.

          Just so I write this down somewhere - this git command made the patch sane to review:

          git diff origin/master -M05 -C05

          Show
          Damyon Wiese added a comment - Thanks for the patch Petr. Just so I write this down somewhere - this git command made the patch sane to review: git diff origin/master -M05 -C05
          Hide
          Damyon Wiese added a comment -

          Thanks Petr,

          I have integrated this change (master) now. I would have waited a bit longer for discussion about the deprecation - but I missed Eloys comment above.

          As for the way forward, IMO we should create one issue to convert all uses in core at once and then add debugging notices to the deprecated classes. Otherwise we will never be able to remove the wrappers and the exceptions to the autoloader.

          Show
          Damyon Wiese added a comment - Thanks Petr, I have integrated this change (master) now. I would have waited a bit longer for discussion about the deprecation - but I missed Eloys comment above. As for the way forward, IMO we should create one issue to convert all uses in core at once and then add debugging notices to the deprecated classes. Otherwise we will never be able to remove the wrappers and the exceptions to the autoloader.
          Hide
          Petr Škoda added a comment -

          I am against the adding of deprecated warnings here, we need to convert a lot more classes and it seems very annoying to require everybody to migrate the code. We will not be able to remove the old classes in the foreseeable future...

          Show
          Petr Škoda added a comment - I am against the adding of deprecated warnings here, we need to convert a lot more classes and it seems very annoying to require everybody to migrate the code. We will not be able to remove the old classes in the foreseeable future...
          Hide
          Damyon Wiese added a comment -

          Agree that we will not be able to remove the wrappers for years. Also if we add deprecation warnings - we should wait at least until we have cleaned up all uses in core code.

          But - if we don't add the warnings we will never be able to remove the wrappers - and isn't that the point of deprecation warnings?

          The existence of the wrappers is not a big deal - the main benefit is to make the code consistent so new developers don't come in and see a mix of textlib:: core_text:: and get confused in 6 months time.

          Show
          Damyon Wiese added a comment - Agree that we will not be able to remove the wrappers for years. Also if we add deprecation warnings - we should wait at least until we have cleaned up all uses in core code. But - if we don't add the warnings we will never be able to remove the wrappers - and isn't that the point of deprecation warnings? The existence of the wrappers is not a big deal - the main benefit is to make the code consistent so new developers don't come in and see a mix of textlib:: core_text:: and get confused in 6 months time.
          Hide
          Petr Škoda added a comment -

          Our priority is BC, deprecated warnings are not fully BC. I vote for adding them in places where there is actually some real benefit, I do not mind leaving the wrappers around for years. But yes, my +10 to convert all uses in standard distribution soon.

          Show
          Petr Škoda added a comment - Our priority is BC, deprecated warnings are not fully BC. I vote for adding them in places where there is actually some real benefit, I do not mind leaving the wrappers around for years. But yes, my +10 to convert all uses in standard distribution soon.
          Hide
          Damyon Wiese added a comment -

          And linking to Dans related comment about get_context_by_id which also applies here and sounds reasonable to me.

          https://tracker.moodle.org/browse/MDL-34472?focusedCommentId=231047&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-231047

          Adding this to codechecker but not printing warnings is effectively an extra level of developer warnings which seems a nicer middle ground (although maybe it would be better to actually add an extra level of developer warning).

          Show
          Damyon Wiese added a comment - And linking to Dans related comment about get_context_by_id which also applies here and sounds reasonable to me. https://tracker.moodle.org/browse/MDL-34472?focusedCommentId=231047&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-231047 Adding this to codechecker but not printing warnings is effectively an extra level of developer warnings which seems a nicer middle ground (although maybe it would be better to actually add an extra level of developer warning).
          Hide
          Mark Nelson added a comment -

          All steps work without any issues. Passing.

          Show
          Mark Nelson added a comment - All steps work without any issues. Passing.
          Hide
          Damyon Wiese added a comment -

          This issue is fixed! Hurray! Hurray!
          Your issue is fixed, what a wonderful day!

          Cheers, Damyon

          Show
          Damyon Wiese added a comment - This issue is fixed! Hurray! Hurray! Your issue is fixed, what a wonderful day! Cheers, Damyon
          Hide
          Joseph Rézeau added a comment -

          Tim

          When a developer is upgrading their code, it is useless to just tell them what not to do. They are already frustrated because a core change broke their previously working code. To avoid just making them more frustrated, you must tell me the right thing to do instead.


          +1

          Show
          Joseph Rézeau added a comment - Tim When a developer is upgrading their code, it is useless to just tell them what not to do. They are already frustrated because a core change broke their previously working code. To avoid just making them more frustrated, you must tell me the right thing to do instead. +1
          Hide
          Petr Škoda added a comment -

          Joseph Rézeau: What is the point of repeating this here? textlib.class.php was never supposed to be included directly in plugins by developers because it was included from lib/setup.php

          Show
          Petr Škoda added a comment - Joseph Rézeau : What is the point of repeating this here? textlib.class.php was never supposed to be included directly in plugins by developers because it was included from lib/setup.php
          Hide
          Joseph Rézeau added a comment -

          Petr, point taken. I didn't realize that.

          My message was not concerned with textlib as such, I just meant to express my agreement with Tim's general statement re the necessity to tell developers what to do when some function becomes deprecated.

          Show
          Joseph Rézeau added a comment - Petr, point taken. I didn't realize that. My message was not concerned with textlib as such, I just meant to express my agreement with Tim's general statement re the necessity to tell developers what to do when some function becomes deprecated.

            People

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

              Dates

              • Created:
                Updated:
                Resolved: