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

      Description

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

        Gliffy Diagrams

        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: