Moodle
  1. Moodle
  2. MDL-28599

Improve textlib to allow locale aware sorting of objects

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.1.1
    • Fix Version/s: 2.2
    • Component/s: Language
    • Labels:
    • Testing Instructions:
      Hide
      1. Log in as an admin
      2. Browse to settings > Appearance > Theme's > Theme settings
      3. Enabled user theme's, course themes, and category themes.
      4. Edit a course and category and check the theme selector shows the correct theme names (coming from themes pluginname string).
      5. Log in as a normal user and check when editing your profile the theme selector shows the correct names.
      Show
      Log in as an admin Browse to settings > Appearance > Theme's > Theme settings Enabled user theme's, course themes, and category themes. Edit a course and category and check the theme selector shows the correct theme names (coming from themes pluginname string). Log in as a normal user and check when editing your profile the theme selector shows the correct names.
    • Affected Branches:
      MOODLE_21_STABLE
    • Fixed Branches:
      MOODLE_22_STABLE
    • Pull Master Branch:
      wip-MDL-28599-master-r3
    • Rank:
      18277

      Description

      Title says it all!

        Issue Links

          Activity

          Hide
          Sam Hemelryk added a comment -

          Hi guys,

          I've talked to Petr a little bit about this, I've been working on textlib improvement to support locale aware sorting of objects by property and method.
          I've created two patches, I'll explain why below:

          https://github.com/samhemelryk/moodle/compare/master...wip-MDL-28599-master-r1
          This is the first patch that simply adds the functionality to the textlib class and works like asort does presently.

          https://github.com/samhemelryk/moodle/compare/master...wip-MDL-28599-master-r2
          This is the second patch which introduces the same functionality in a different way. In this patch textlib->asort plus the new methods have been split out to a new abstract class collatorlib and at the same time converted to static methods. Why do this? Because the collator operations don't actually require the typo libs to be initialised, so on scripts where textlib isn't otherwise used it is also a performance win.
          I've also refactored all core use of textlib->asort.

          Please I'd like to get feedback and thoughts on this, as a working example I have fixed the theme selector for course, category and user themes in their editing forms (MDL-24567).

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi guys, I've talked to Petr a little bit about this, I've been working on textlib improvement to support locale aware sorting of objects by property and method. I've created two patches, I'll explain why below: https://github.com/samhemelryk/moodle/compare/master...wip-MDL-28599-master-r1 This is the first patch that simply adds the functionality to the textlib class and works like asort does presently. https://github.com/samhemelryk/moodle/compare/master...wip-MDL-28599-master-r2 This is the second patch which introduces the same functionality in a different way. In this patch textlib->asort plus the new methods have been split out to a new abstract class collatorlib and at the same time converted to static methods. Why do this? Because the collator operations don't actually require the typo libs to be initialised, so on scripts where textlib isn't otherwise used it is also a performance win. I've also refactored all core use of textlib->asort. Please I'd like to get feedback and thoughts on this, as a working example I have fixed the theme selector for course, category and user themes in their editing forms ( MDL-24567 ). Cheers Sam
          Hide
          Petr Škoda added a comment -

          +1 for static library methods, yay!

          Show
          Petr Škoda added a comment - +1 for static library methods, yay!
          Hide
          Sam Hemelryk added a comment -

          Putting this up for integration and making Eloy the integrator.... haha Eloy can't ignore it now!

          Show
          Sam Hemelryk added a comment - Putting this up for integration and making Eloy the integrator.... haha Eloy can't ignore it now!
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Some comments:

          A) +1 to move all the collation stuff to new collationlib, using static methods there always. I don't like to mix static/non static in textlib.

          Some objections to r2 patch (that seems the way to go, so I skip commenting about r1):

          1) Trivial: The method names "asort_objects_xxxxx". Would be better to add one "by" keyword there? It causes the functionality to be clearer here (i.e. "asort_objects_by_method").

          2) Important: Current implementation does not support "changing of locale" along the request life. Once one collator has been instantiated... it's the same for all the uses. I can imagine this causing problems if, at any time, we run collator stuff from cron or similar (where the user locale changes over the time).

          3) Detail, could be important: It sounds to me that both the procedural and OOP instantiation of locales were returning ALWAYS one Collate object, no matter if the locale passed was a correct/incorrect one or no, defaulting to "something" (not sure what) always. I remember myself using some "get_error" or "is_error" functions in order to decide if the Collate object was the desired one or no. Please check that.

          4) Isn't the "&" needed both in asort_objects_method and asort_objects_property instead of returning it? The API should be consistent with all XXXsort() methods IMO.

          5) Tests, tests, tests. Any new lib like this should be tested, covering aspects like 2) and 3) above for sure.

          Said that, it is a good start, but needs some more work... so I'm reopening this now. Thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - Some comments: A) +1 to move all the collation stuff to new collationlib, using static methods there always. I don't like to mix static/non static in textlib. Some objections to r2 patch (that seems the way to go, so I skip commenting about r1): 1) Trivial: The method names "asort_objects_xxxxx". Would be better to add one "by" keyword there? It causes the functionality to be clearer here (i.e. "asort_objects_by_method"). 2) Important: Current implementation does not support "changing of locale" along the request life. Once one collator has been instantiated... it's the same for all the uses. I can imagine this causing problems if, at any time, we run collator stuff from cron or similar (where the user locale changes over the time). 3) Detail, could be important: It sounds to me that both the procedural and OOP instantiation of locales were returning ALWAYS one Collate object, no matter if the locale passed was a correct/incorrect one or no, defaulting to "something" (not sure what) always. I remember myself using some "get_error" or "is_error" functions in order to decide if the Collate object was the desired one or no. Please check that. 4) Isn't the "&" needed both in asort_objects_method and asort_objects_property instead of returning it? The API should be consistent with all XXXsort() methods IMO. 5) Tests, tests, tests. Any new lib like this should be tested, covering aspects like 2) and 3) above for sure. Said that, it is a good start, but needs some more work... so I'm reopening this now. Thanks!
          Hide
          Sam Hemelryk added a comment -

          Hi Eloy,

          Thanks for the feedback.
          I've gone through the points you've raised and fixed them all up.
          Could you please review the changes for me?

          https://github.com/samhemelryk/moodle/compare/master...wip-MDL-28599-master-r2

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi Eloy, Thanks for the feedback. I've gone through the points you've raised and fixed them all up. Could you please review the changes for me? https://github.com/samhemelryk/moodle/compare/master...wip-MDL-28599-master-r2 Cheers Sam
          Hide
          Sam Hemelryk added a comment -

          Making you peer reviewer Eloy

          Show
          Sam Hemelryk added a comment - Making you peer reviewer Eloy
          Hide
          Sam Hemelryk added a comment -

          Hi Eloy,

          If you are happy with this could you please put it up for integration.

          Integrator (perhaps also you Eloy ) feel free to cherry-pick, squash, whatever you want.

          Cheers fella's

          Show
          Sam Hemelryk added a comment - Hi Eloy, If you are happy with this could you please put it up for integration. Integrator (perhaps also you Eloy ) feel free to cherry-pick, squash, whatever you want. Cheers fella's
          Hide
          Sam Hemelryk added a comment -

          Hi Eloy, have you had a chance to look at this?
          If not and you have a minute spare could you please? if you are happy with it I'll put it up for integration.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi Eloy, have you had a chance to look at this? If not and you have a minute spare could you please? if you are happy with it I'll put it up for integration. Cheers Sam
          Hide
          Eloy Lafuente (stronk7) added a comment -

          LOL,

          note we have changed textlib right TODAY a lot, perhaps this need to be re-merged/whatever. Ping me once updated, plz. (it sounds to me I looked some time ago and was nice, the trickier thing was the split of locale stuff out from textlib).

          Let's see... awaiting for ping. Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - LOL, note we have changed textlib right TODAY a lot, perhaps this need to be re-merged/whatever. Ping me once updated, plz. (it sounds to me I looked some time ago and was nice, the trickier thing was the split of locale stuff out from textlib). Let's see... awaiting for ping. Ciao
          Hide
          Petr Škoda added a comment -

          hehe, I thought Sam would not be around yet, that is why I rushed it before my own vacation...

          Show
          Petr Škoda added a comment - hehe, I thought Sam would not be around yet, that is why I rushed it before my own vacation...
          Hide
          Sam Hemelryk added a comment -

          LOL conspiracies!
          Haha thats fine I'll check out the changes after the weekly release and see whether this is still relevant and if it is rebasing/rewriting this change.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - LOL conspiracies! Haha thats fine I'll check out the changes after the weekly release and see whether this is still relevant and if it is rebasing/rewriting this change. Cheers Sam
          Hide
          Sam Hemelryk added a comment -

          Hi guys,
          Got this rebased and fixed the conflicts - I think you had done about half of what I had done in the end Petr (obviously you did much more as well)

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi guys, Got this rebased and fixed the conflicts - I think you had done about half of what I had done in the end Petr (obviously you did much more as well) Cheers Sam
          Hide
          Eloy Lafuente (stronk7) added a comment -

          I wil be happy to integrate this if:

          1) The change is explained in some upgrade.txt (@ /lib/upgrade.php perhaps?)
          2) Some unit tests are added to cover as much as possible.

          It's enough if you create one followup for 1 & 2 (not need to have them done NOW - but next week).

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - I wil be happy to integrate this if: 1) The change is explained in some upgrade.txt (@ /lib/upgrade.php perhaps?) 2) Some unit tests are added to cover as much as possible. It's enough if you create one followup for 1 & 2 (not need to have them done NOW - but next week). Ciao
          Hide
          Sam Hemelryk added a comment - - edited

          Hi Eloy,

          I've gone through and added some unit tests now and pushed them to a new branch so you can see what has changed:
          Branch: wip-MDL-28599-master-r4
          DiffURL: https://github.com/samhemelryk/moodle/compare/master...wip-MDL-28599-master-r4

          I didn't quite get where I should add upgrade comments sorry so I have left that for the time being.

          Cheers
          Sam

          p.s. Man I greatly dislike the Jira shortcuts... GRrRRR I don't expect ctrl+s to add my comment!

          Show
          Sam Hemelryk added a comment - - edited Hi Eloy, I've gone through and added some unit tests now and pushed them to a new branch so you can see what has changed: Branch: wip- MDL-28599 -master-r4 DiffURL: https://github.com/samhemelryk/moodle/compare/master...wip-MDL-28599-master-r4 I didn't quite get where I should add upgrade comments sorry so I have left that for the time being. Cheers Sam p.s. Man I greatly dislike the Jira shortcuts... GRrRRR I don't expect ctrl+s to add my comment!
          Hide
          Petr Škoda added a comment -

          There is a file mod/upgrade.txt that lists changes in APIs, that should help us with release notes later.

          This should be some changelog for developers, ideally there should be one global and then one in each plugin type.

          Show
          Petr Škoda added a comment - There is a file mod/upgrade.txt that lists changes in APIs, that should help us with release notes later. This should be some changelog for developers, ideally there should be one global and then one in each plugin type.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Yeah, Petr, that was the reason I suggested above to create the lib/upgrade.txt as the "global" one (apart from the plugin/subsystem ones).

          Although having this noted in the mod/upgrade.txt one is (worse but) ok too, to let mod devs what to change.

          Going to integrate this now (adding 1-line comment to mod/upgrade.txt)... ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Yeah, Petr, that was the reason I suggested above to create the lib/upgrade.txt as the "global" one (apart from the plugin/subsystem ones). Although having this noted in the mod/upgrade.txt one is (worse but) ok too, to let mod devs what to change. Going to integrate this now (adding 1-line comment to mod/upgrade.txt)... ciao
          Hide
          Eloy Lafuente (stronk7) added a comment -

          r4 integrated, yay-thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - r4 integrated, yay-thanks!
          Hide
          Eloy Lafuente (stronk7) added a comment - - edited

          Ops, just tried my test integration site after adding this and I'm getting constant (in all pages) debugging:

          Locale collator generated warnings (not fatal) "U_ZERO_ERROR" falling back to en
          line 620 of /lib/textlib.class.php: call to debugging()
          line 642 of /lib/textlib.class.php: call to collatorlib::ensure_collator_available()
          line 522 of /lib/filterlib.php: call to collatorlib::asort()
          line 221 of /admin/settings/plugins.php: call to filter_get_all_installed()
          line 5905 of /lib/adminlib.php: call to require()
          line 2955 of /lib/navigationlib.php: call to admin_get_root()
          line 2863 of /lib/navigationlib.php: call to settings_navigation->load_administration_settings()
          line 601 of /lib/pagelib.php: call to settings_navigation->initialise()
          line 617 of /lib/pagelib.php: call to moodle_page->magic_get_settingsnav()
          line 132 of /blocks/settings/block_settings.php: call to moodle_page->__get()
          line 280 of /blocks/moodleblock.class.php: call to block_settings->get_content()
          line 232 of /blocks/moodleblock.class.php: call to block_base->formatted_contents()
          line 924 of /lib/blocklib.php: call to block_base->get_content_for_output()
          line 976 of /lib/blocklib.php: call to block_manager->create_block_contents()
          line 349 of /lib/blocklib.php: call to block_manager->ensure_content_created()
          line 3 of /theme/base/layout/frontpage.php: call to block_manager->region_has_content()
          line 685 of /lib/outputrenderers.php: call to include()
          line 637 of /lib/outputrenderers.php: call to core_renderer->render_page_layout()
          line ? of unknownfile: call to core_renderer->header()
          line 1291 of /lib/setuplib.php: call to call_user_func_array()
          line 89 of /index.php: call to bootstrap_renderer->__call()
          line 89 of /index.php: call to bootstrap_renderer->header()
          

          Perhaps we should be using intl_is_failure() to control other non-error error-codes like U_ZERO_ERROR (0) or U_USING_DEFAULT_WARNING (-127) ?

          Also note I've added one extra commit to fix one minor problem with the error shown in debugging() not being the correct one:

          http://git.moodle.org/gw?p=integration.git;a=commitdiff;h=20079ee7bcf2bae01f9e06d5816b51a8c16c5250

          Show
          Eloy Lafuente (stronk7) added a comment - - edited Ops, just tried my test integration site after adding this and I'm getting constant (in all pages) debugging: Locale collator generated warnings (not fatal) "U_ZERO_ERROR" falling back to en line 620 of /lib/textlib.class.php: call to debugging() line 642 of /lib/textlib.class.php: call to collatorlib::ensure_collator_available() line 522 of /lib/filterlib.php: call to collatorlib::asort() line 221 of /admin/settings/plugins.php: call to filter_get_all_installed() line 5905 of /lib/adminlib.php: call to require() line 2955 of /lib/navigationlib.php: call to admin_get_root() line 2863 of /lib/navigationlib.php: call to settings_navigation->load_administration_settings() line 601 of /lib/pagelib.php: call to settings_navigation->initialise() line 617 of /lib/pagelib.php: call to moodle_page->magic_get_settingsnav() line 132 of /blocks/settings/block_settings.php: call to moodle_page->__get() line 280 of /blocks/moodleblock.class.php: call to block_settings->get_content() line 232 of /blocks/moodleblock.class.php: call to block_base->formatted_contents() line 924 of /lib/blocklib.php: call to block_base->get_content_for_output() line 976 of /lib/blocklib.php: call to block_manager->create_block_contents() line 349 of /lib/blocklib.php: call to block_manager->ensure_content_created() line 3 of /theme/base/layout/frontpage.php: call to block_manager->region_has_content() line 685 of /lib/outputrenderers.php: call to include() line 637 of /lib/outputrenderers.php: call to core_renderer->render_page_layout() line ? of unknownfile: call to core_renderer->header() line 1291 of /lib/setuplib.php: call to call_user_func_array() line 89 of /index.php: call to bootstrap_renderer->__call() line 89 of /index.php: call to bootstrap_renderer->header() Perhaps we should be using intl_is_failure() to control other non-error error-codes like U_ZERO_ERROR (0) or U_USING_DEFAULT_WARNING (-127) ? Also note I've added one extra commit to fix one minor problem with the error shown in debugging() not being the correct one: http://git.moodle.org/gw?p=integration.git;a=commitdiff;h=20079ee7bcf2bae01f9e06d5816b51a8c16c5250
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Really those U_USING_DEFAULT_WARNING messages seem a bit anarchic:

          1) In my Mac I get: "Locale collator generated warnings (not fatal) "U_USING_DEFAULT_WARNING" falling back to en", when switching to English in the lang menu. Note it's fallbacking exactly to the one that has failed, hehe. And returns it as valid. Other langs (de, es...) work pefectly.

          2) In qa.moodle.net I get: "Locale collator generated warnings (not fatal) "U_USING_DEFAULT_WARNING" falling back to pt", when I switch to pt_br or pt. In theory, when picking pt_br we should get U_USING_FALLBACK_WARNING (fallback to pt) and nothing when picking pt.

          Other langs with no proper locale (eu, for example) return proper falling back message to the system default.

          So seems to be really dependent of how the locales are named on each system and also, which locales are installed.

          Hence, not sure how we should handle this, but in my case, getting the header borked on each page when using English is really annoying (and I've all the english locales installed here afaik). Perhaps we should, simply ignore any U_USING_DEFAULT_WARNING (-127) and done?

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Really those U_USING_DEFAULT_WARNING messages seem a bit anarchic: 1) In my Mac I get: "Locale collator generated warnings (not fatal) "U_USING_DEFAULT_WARNING" falling back to en", when switching to English in the lang menu. Note it's fallbacking exactly to the one that has failed, hehe. And returns it as valid. Other langs (de, es...) work pefectly. 2) In qa.moodle.net I get: "Locale collator generated warnings (not fatal) "U_USING_DEFAULT_WARNING" falling back to pt", when I switch to pt_br or pt. In theory, when picking pt_br we should get U_USING_FALLBACK_WARNING (fallback to pt) and nothing when picking pt. Other langs with no proper locale (eu, for example) return proper falling back message to the system default. So seems to be really dependent of how the locales are named on each system and also, which locales are installed. Hence, not sure how we should handle this, but in my case, getting the header borked on each page when using English is really annoying (and I've all the english locales installed here afaik). Perhaps we should, simply ignore any U_USING_DEFAULT_WARNING (-127) and done? Ciao
          Hide
          Rajesh Taneja added a comment -

          Works Great Sam
          Thanks for improving code.

          Show
          Rajesh Taneja added a comment - Works Great Sam Thanks for improving code.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          git & cvs repositories updated with your gorgeous code. Many thanks!

          Closing and ciao

          Show
          Eloy Lafuente (stronk7) added a comment - git & cvs repositories updated with your gorgeous code. Many thanks! Closing and ciao
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Linking with MDL-29269, where some points related with this will be fixed/improved.

          Show
          Eloy Lafuente (stronk7) added a comment - Linking with MDL-29269 , where some points related with this will be fixed/improved.

            People

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

              Dates

              • Created:
                Updated:
                Resolved: