Moodle
  1. Moodle
  2. MDL-26425

Assorted minor improvements to lib/tablelib.php

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 2.0.1
    • Fix Version/s: 2.1
    • Component/s: Libraries
    • Labels:
    • Affected Branches:
      MOODLE_20_STABLE
    • Fixed Branches:
      MOODLE_21_STABLE

      Description

      • Fixed some use of $_GET
      • Convert some more code to use html_writer
      • Add validation of sort_columns
      • The ability to add a class="..." attribute to certain table rows, depending on the data.
      • Improved some PHP docs.
      • Fixed some minor coding style points.

      I need these for the new question engine (MDL-20636)

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            Tim Hunt added a comment -

            Right, there is a patch series here: https://github.com/timhunt/moodle/compare/master...MDL-26425

            The two changes I actually need for MDL-20636 are https://github.com/timhunt/moodle/commit/d5af1a5c089d63780dd9b07ba98b64903970107a and https://github.com/timhunt/moodle/commit/a17cc49fba7ce526805bfce65358dae3c7c4f7eb. The rest of the cleaning up I did to try to be helpful, and because I hate ugly code.

            There is only one other change in functionality in these patches. Well, it is not really a change, but I stripped out some old code that has not worked for ages. See https://github.com/timhunt/moodle/commit/42606aaa3143255623a3874a128cb7e8599f90e8.

            Apart from that, functionality should not have changed.

            To test this properly, we really need to test all the places this class is used. My searching finds this complete list:

            admin/blocks.php
            admin/localplugins.php
            admin/modules.php
            admin/qtypes.php
            admin/report/profiling/index.php
            admin/xmldb/actions/edit_table/edit_table.class.php
            admin/xmldb/actions/view_table_sql/view_table_sql.class.php
            blocks/rss_client/managefeeds.php
            course/report/participation/index.php
            enrol/authorize/locallib.php
            grade/report/overview/lib.php
            lang/en/xmldb.php
            lib/ddl/mssql_sql_generator.php
            lib/ddl/sql_generator.php
            lib/xhprof/xhprof_moodle.php
            mod/assignment/lib.php
            mod/feedback/mapcourse.php
            mod/feedback/show_entries.php
            mod/feedback/show_entries_anonym.php
            mod/feedback/show_nonrespondents.php
            mod/quiz/report/grading/report.php
            mod/quiz/report/overview/overview_table.php
            mod/quiz/report/responses/responses_table.php
            mod/quiz/report/statistics/statistics_question_table.php
            mod/quiz/report/statistics/statistics_table.php
            mod/scorm/report.php
            tag/manage.php
            user/index.php

            In practice, I think we only need to test a reasonable sample of the tables on these pages. You need to test sorting, collapsing columns, paging, and restricting the list of users by initial, and so on.

            Show
            Tim Hunt added a comment - Right, there is a patch series here: https://github.com/timhunt/moodle/compare/master...MDL-26425 The two changes I actually need for MDL-20636 are https://github.com/timhunt/moodle/commit/d5af1a5c089d63780dd9b07ba98b64903970107a and https://github.com/timhunt/moodle/commit/a17cc49fba7ce526805bfce65358dae3c7c4f7eb . The rest of the cleaning up I did to try to be helpful, and because I hate ugly code. There is only one other change in functionality in these patches. Well, it is not really a change, but I stripped out some old code that has not worked for ages. See https://github.com/timhunt/moodle/commit/42606aaa3143255623a3874a128cb7e8599f90e8 . Apart from that, functionality should not have changed. To test this properly, we really need to test all the places this class is used. My searching finds this complete list: admin/blocks.php admin/localplugins.php admin/modules.php admin/qtypes.php admin/report/profiling/index.php admin/xmldb/actions/edit_table/edit_table.class.php admin/xmldb/actions/view_table_sql/view_table_sql.class.php blocks/rss_client/managefeeds.php course/report/participation/index.php enrol/authorize/locallib.php grade/report/overview/lib.php lang/en/xmldb.php lib/ddl/mssql_sql_generator.php lib/ddl/sql_generator.php lib/xhprof/xhprof_moodle.php mod/assignment/lib.php mod/feedback/mapcourse.php mod/feedback/show_entries.php mod/feedback/show_entries_anonym.php mod/feedback/show_nonrespondents.php mod/quiz/report/grading/report.php mod/quiz/report/overview/overview_table.php mod/quiz/report/responses/responses_table.php mod/quiz/report/statistics/statistics_question_table.php mod/quiz/report/statistics/statistics_table.php mod/scorm/report.php tag/manage.php user/index.php In practice, I think we only need to test a reasonable sample of the tables on these pages. You need to test sorting, collapsing columns, paging, and restricting the list of users by initial, and so on.
            Hide
            Tim Hunt added a comment -

            Just to note, I missed one final commit, which I have now pushed. I also rebased the branch onto the latest master. I am hoping that someone will be able to peer review this at some point, but I am not expecting that until after 2.0.2 is done.

            Show
            Tim Hunt added a comment - Just to note, I missed one final commit, which I have now pushed. I also rebased the branch onto the latest master. I am hoping that someone will be able to peer review this at some point, but I am not expecting that until after 2.0.2 is done.
            Hide
            Tim Hunt added a comment -

            I just rebased https://github.com/timhunt/moodle/compare/master...MDL-26425 onto the latest master. About to do a Pull request. Note that sam has already reviewed this, so it has bas some peer review, and I have been unable to get anyone from HQ to talk an interest before now.

            Show
            Tim Hunt added a comment - I just rebased https://github.com/timhunt/moodle/compare/master...MDL-26425 onto the latest master. About to do a Pull request. Note that sam has already reviewed this, so it has bas some peer review, and I have been unable to get anyone from HQ to talk an interest before now.
            Hide
            Tim Hunt added a comment -

            Pull request done. Note that the links to individual commits above go to the commits before the latest rebase. The PULL request has up-to-date links.

            Testing instructions above, if you want more details, let me know.

            Show
            Tim Hunt added a comment - Pull request done. Note that the links to individual commits above go to the commits before the latest rebase. The PULL request has up-to-date links. Testing instructions above, if you want more details, let me know.
            Hide
            Petr Skoda added a comment -

            Reopening, there is a problem with table constructors BC, see the PULL for details.

            I like the rest, thanks a lot!

            Show
            Petr Skoda added a comment - Reopening, there is a problem with table constructors BC, see the PULL for details. I like the rest, thanks a lot!
            Hide
            Tim Hunt added a comment -

            Thanks for looking at it all. I am quite pleased that there was only one problem

            Is it OK if I change the code so it is like

            <?php
            require('config.php');
            class a {
                function __construct() {
                    echo "grrr";
                }
                /**
                 * @deprecated since Moodle 2.0. Will be removed in Moodle 2.1.
                 */
                function a() {
                    debugging('Please update your code to user PHP5-style __construct().');
                    $this->__construct();
                }
            }
            class b extends a {
                function b() {
                    parent::a();
                }
            }
            $b = new b();
            

            If that is OK, I will fix this and do a new pull request next week.

            Show
            Tim Hunt added a comment - Thanks for looking at it all. I am quite pleased that there was only one problem Is it OK if I change the code so it is like <?php require('config.php'); class a { function __construct() { echo "grrr"; } /** * @deprecated since Moodle 2.0. Will be removed in Moodle 2.1. */ function a() { debugging('Please update your code to user PHP5-style __construct().'); $this->__construct(); } } class b extends a { function b() { parent::a(); } } $b = new b(); If that is OK, I will fix this and do a new pull request next week.
            Hide
            Petr Skoda added a comment -

            I never tried that but it should be ok because some other projects do that.

            Show
            Petr Skoda added a comment - I never tried that but it should be ok because some other projects do that.
            Hide
            Tim Hunt added a comment -

            I just tried it in a simple test file, and it seemed to work. OK, I'll do that for next week. Thanks.

            Show
            Tim Hunt added a comment - I just tried it in a simple test file, and it seemed to work. OK, I'll do that for next week. Thanks.
            Hide
            Tim Hunt added a comment -

            OK, new pull request done. Note that I rebased the MDL-26425 branch, so if you pulled that before, you will have to do a pull -f to get the new branch. New PULL request done.

            I tested with a test.php script like:

             
            <?php
            require_once('config.php');
            require_once($CFG->libdir . '/tablelib.php');
             
            class test_table extends table_sql {
                function test_table() {
                    parent::table_sql('test_table');
                }
            }
             
            $table = new test_table();
            echo $table->uniqueid;
            

            and that seemed to behave OK.

            Instructions for testing the rest of the patch are above.

            Show
            Tim Hunt added a comment - OK, new pull request done. Note that I rebased the MDL-26425 branch, so if you pulled that before, you will have to do a pull -f to get the new branch. New PULL request done. I tested with a test.php script like:   <?php require_once('config.php'); require_once($CFG->libdir . '/tablelib.php');   class test_table extends table_sql { function test_table() { parent::table_sql('test_table'); } }   $table = new test_table(); echo $table->uniqueid; and that seemed to behave OK. Instructions for testing the rest of the patch are above.
            Hide
            Helen Foster added a comment -

            Thanks Tim

            Show
            Helen Foster added a comment - Thanks Tim
            Hide
            Eloy Lafuente (stronk7) added a comment - - edited

            Hi guys,

            something seems to be really wrong in the master solution applied for this issue. I was changing the xhprof_table_sql to use the new get_row_class() method instead of my current hacks.. and everything seems ok in 20_STABLE but not in master (tr tag completely borked).

            See MDL-27034 for more details... ciao

            Show
            Eloy Lafuente (stronk7) added a comment - - edited Hi guys, something seems to be really wrong in the master solution applied for this issue. I was changing the xhprof_table_sql to use the new get_row_class() method instead of my current hacks.. and everything seems ok in 20_STABLE but not in master (tr tag completely borked). See MDL-27034 for more details... ciao

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved: