Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Trivial Trivial
    • Resolution: Fixed
    • Affects Version/s: 2.0.2, 2.1.1, 2.2
    • Fix Version/s: 2.0.5, 2.1.2
    • Component/s: Blocks
    • Labels:
    • Environment:
      All
    • Testing Instructions:
      Hide

      1. Open "Plugins | Blocks | Manage blocks"
      2. Ensure that blocks names in the list are sorted alphabetically

      Ideally this should be tested both with "intl" PHP extension available and not.

      Show
      1. Open "Plugins | Blocks | Manage blocks" 2. Ensure that blocks names in the list are sorted alphabetically Ideally this should be tested both with "intl" PHP extension available and not.
    • Affected Branches:
      MOODLE_20_STABLE, MOODLE_21_STABLE, MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_20_STABLE, MOODLE_21_STABLE
    • Pull from Repository:
      git@github.com:marinaglancy/moodle.git
    • Pull Master Branch:
      wip-MDL-26754-master
    • Rank:
      16374

      Description

      Plugins | Blocks | Manage blocks are not displayed in alphabetically order.

      1. MDL-26754.patch
        3 kB
        Priyanka M
      2. MDL-26754.patch
        2 kB
        Priyanka M

        Issue Links

          Activity

          Hide
          Priyanka M added a comment -

          The code responsible for this lies in admin/blocks.php. The behaviour is like this because, while fetching the sorting is being done on 'blockname', which happens correctly. But for each fetched block, a description is fetched for that block. For eg., the description of 'blog_recent' block is 'Recent blog entries'. This description is stored in $strblockname. The html is pushed into an array, $table, and is pushed in the order of the original 'blockname', with $strblockname being the actual block name being pushed. However, $strblockname is not necessarily same as the original blockname on which the sorting was done. As a result, alphabetical order is not preserved.

          Attaching the patch for the same. Please check and revert.
          Thanks & Regards.

          Show
          Priyanka M added a comment - The code responsible for this lies in admin/blocks.php. The behaviour is like this because, while fetching the sorting is being done on 'blockname', which happens correctly. But for each fetched block, a description is fetched for that block. For eg., the description of 'blog_recent' block is 'Recent blog entries'. This description is stored in $strblockname. The html is pushed into an array, $table, and is pushed in the order of the original 'blockname', with $strblockname being the actual block name being pushed. However, $strblockname is not necessarily same as the original blockname on which the sorting was done. As a result, alphabetical order is not preserved. Attaching the patch for the same. Please check and revert. Thanks & Regards.
          Hide
          Priyanka M added a comment -

          Fix for MDL-26754

          Show
          Priyanka M added a comment - Fix for MDL-26754
          Hide
          Helen Foster added a comment -

          Alex, thanks for your report and Priyanka, thanks for your comment and patch.

          Show
          Helen Foster added a comment - Alex, thanks for your report and Priyanka, thanks for your comment and patch.
          Hide
          David Mudrak added a comment -

          Priyanka, thanks for the patch. Unfortunately we can't accept it. The function array_multisort() does not natively support locale based sorting so it works reliably with ASCII only. Please see lib/textlib.class.php and its usage across Moodle on how we deal with sorting localized strings.

          Show
          David Mudrak added a comment - Priyanka, thanks for the patch. Unfortunately we can't accept it. The function array_multisort() does not natively support locale based sorting so it works reliably with ASCII only. Please see lib/textlib.class.php and its usage across Moodle on how we deal with sorting localized strings.
          Hide
          Priyanka M added a comment -

          Patch with locale sensitive sort function (from lib/textlib.class.php)

          Show
          Priyanka M added a comment - Patch with locale sensitive sort function (from lib/textlib.class.php)
          Hide
          Priyanka M added a comment -

          Hi David,
          Thanks for your input. Please find attached the latest patch.

          Show
          Priyanka M added a comment - Hi David, Thanks for your input. Please find attached the latest patch.
          Hide
          David Mudrak added a comment -

          Errr - sorry. Do I read well that you basically replaced array_multisort() with asort() ?! So you do not use the textlib at all. I was hoping you would realize how textlib_get_instance() is used in Moodle code. Also note that asort() actually does not support SORT_ASC flag as array_multisort() does.

          Other details: Why to use array_push() in these trivial situations when $arrayvar[]= ... is more readable? And please note that $variables should not have underscore (according to Moodle coding guidelines).

          So sorry, I must refuse the patch again.

          Show
          David Mudrak added a comment - Errr - sorry. Do I read well that you basically replaced array_multisort() with asort() ?! So you do not use the textlib at all. I was hoping you would realize how textlib_get_instance() is used in Moodle code. Also note that asort() actually does not support SORT_ASC flag as array_multisort() does. Other details: Why to use array_push() in these trivial situations when $arrayvar[]= ... is more readable? And please note that $variables should not have underscore (according to Moodle coding guidelines). So sorry, I must refuse the patch again.
          Hide
          David Mudrak added a comment -

          Thanks for the patch Marina! Looking good. The only potentially tricky part of this solution is that the behaviour of that asort() function is not well documented for cases when sorting array of arrays. Let us hope that it obeys the definition at http://php.net/manual/en/language.operators.comparison.php

          As the textlib implementation uses the "intl" extension when available, with fallback to normal asort() + SORT_LOCALE_STRING, this should be ideally tested in both situations (with and without the extension) and some non-English (ideally non-latin like azbuka) languages.

          Said that, I give it +1

          Show
          David Mudrak added a comment - Thanks for the patch Marina! Looking good. The only potentially tricky part of this solution is that the behaviour of that asort() function is not well documented for cases when sorting array of arrays. Let us hope that it obeys the definition at http://php.net/manual/en/language.operators.comparison.php As the textlib implementation uses the "intl" extension when available, with fallback to normal asort() + SORT_LOCALE_STRING, this should be ideally tested in both situations (with and without the extension) and some non-English (ideally non-latin like azbuka) languages. Said that, I give it +1
          Hide
          Marina Glancy added a comment -

          Thanks, David. I tested with non-latin language and made sure asort works.

          Show
          Marina Glancy added a comment - Thanks, David. I tested with non-latin language and made sure asort works.
          Hide
          Sam Hemelryk added a comment -

          Hi Marina,

          Reopening this issue just to tidy up one minor point, the require of textlib.php isn't needed as it is included through lib/setup.php

          Normally I would tidy this up on integration however right now I am preparing to move all issues currently in integration that haven't already been reviewed out of integration as we are out of time, but before doing that I am giving a quick review just for anything obvious.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi Marina, Reopening this issue just to tidy up one minor point, the require of textlib.php isn't needed as it is included through lib/setup.php Normally I would tidy this up on integration however right now I am preparing to move all issues currently in integration that haven't already been reviewed out of integration as we are out of time, but before doing that I am giving a quick review just for anything obvious. Cheers Sam
          Hide
          Marina Glancy added a comment -

          Thanks, Sam. I removed include

          Show
          Marina Glancy added a comment - Thanks, Sam. I removed include
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Integrated, thanks! (curious how it sorts by 1st element)

          Show
          Eloy Lafuente (stronk7) added a comment - Integrated, thanks! (curious how it sorts by 1st element)
          Hide
          Rossiani Wijaya added a comment -

          Hi Marina,

          When I tested without 'Intl' extension, the list is not sorted properly. Once I install the intl extension, it works great.

          Link to install intl extension: http://docs.moodle.org/20/en/admin/environment/php_extension/intl

          I will pendent the decision to passed/failed this issue until the end of today.

          Show
          Rossiani Wijaya added a comment - Hi Marina, When I tested without 'Intl' extension, the list is not sorted properly. Once I install the intl extension, it works great. Link to install intl extension: http://docs.moodle.org/20/en/admin/environment/php_extension/intl I will pendent the decision to passed/failed this issue until the end of today.
          Hide
          David Mudrak added a comment -

          Given that what we say about intl:

          $string['intlrecommended'] = 'Intl extension is used to improve internationalization support, such as locale aware sorting.';
          

          my +1 to pass the test if it works with intl. Without intl installed, I would expect only English (latin1) texts being sorted properly.

          Show
          David Mudrak added a comment - Given that what we say about intl: $string['intlrecommended'] = 'Intl extension is used to improve internationalization support, such as locale aware sorting.'; my +1 to pass the test if it works with intl. Without intl installed, I would expect only English (latin1) texts being sorted properly.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          From Rossie chat, it seems that fallback to asort() causes NO sort at all (not English/ASCII sorting either).

          Perhaps asort() is not able to handle sorting of arrays by 1st element and intl->asort is?

          +1 to pass this (does not break) anything and investigate the thing in another followup issue

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - From Rossie chat, it seems that fallback to asort() causes NO sort at all (not English/ASCII sorting either). Perhaps asort() is not able to handle sorting of arrays by 1st element and intl->asort is? +1 to pass this (does not break) anything and investigate the thing in another followup issue Ciao
          Hide
          Rossiani Wijaya added a comment -

          Test passed.

          New issue is created to fix sort with uninstall intl extension (MDL-28973).

          Show
          Rossiani Wijaya added a comment - Test passed. New issue is created to fix sort with uninstall intl extension ( MDL-28973 ).
          Hide
          David Mudrak added a comment -

          Really? Plain asort() does not work? That is weird. I just tested the behaviour using the following test script and it seems to work well:

          $a = array(
              array('delta', array(1, 'd')),
              array('bravo', array(2, 'b')),
              array('charlie', array(3, 'c')),
              array('omega', array(4, 'o')),
          );
          
          asort($a);
          var_dump($a);
          

          The asort() compares two elements of $a using the logic described at http://php.net/manual/en/language.operators.comparison.php

          Array with fewer members is smaller, if key from operand 1 is not found in operand 2 then arrays are uncomparable, otherwise - compare value by value

          In our case, all elements of the main array have just two keys 0 and 1 so asort() is able to compare each of them by using their first element. Which is the trick we use in Marina's patch.

          Can somebody try the example above at their machines? I tested it on PHP 5.3.6 with intl installed (though it should not matter theoretically).

          Show
          David Mudrak added a comment - Really? Plain asort() does not work? That is weird. I just tested the behaviour using the following test script and it seems to work well: $a = array( array('delta', array(1, 'd')), array('bravo', array(2, 'b')), array('charlie', array(3, 'c')), array('omega', array(4, 'o')), ); asort($a); var_dump($a); The asort() compares two elements of $a using the logic described at http://php.net/manual/en/language.operators.comparison.php Array with fewer members is smaller, if key from operand 1 is not found in operand 2 then arrays are uncomparable, otherwise - compare value by value In our case, all elements of the main array have just two keys 0 and 1 so asort() is able to compare each of them by using their first element. Which is the trick we use in Marina's patch. Can somebody try the example above at their machines? I tested it on PHP 5.3.6 with intl installed (though it should not matter theoretically).
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Such an effort has get its prize. Universal Award to the very-best Moodle developer in the family. Thanks!

          Closing. Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Such an effort has get its prize. Universal Award to the very-best Moodle developer in the family. Thanks! Closing. Ciao

            People

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

              Dates

              • Created:
                Updated:
                Resolved: