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
    • Rank:
      16069

      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)

        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 Škoda 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 Škoda 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 Škoda added a comment -

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

          Show
          Petr Škoda 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: