Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-26425

Assorted minor improvements to lib/tablelib.php

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: 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

          Attachments

            Issue Links

              Activity

              Hide
              timhunt 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
              timhunt 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
              timhunt 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
              timhunt 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
              timhunt 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
              timhunt 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
              timhunt 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
              timhunt 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
              skodak 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
              skodak 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
              timhunt 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
              timhunt 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
              skodak Petr Skoda added a comment -

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

              Show
              skodak Petr Skoda added a comment - I never tried that but it should be ok because some other projects do that.
              Hide
              timhunt 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
              timhunt 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
              timhunt 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
              timhunt 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
              tsala Helen Foster added a comment -

              Thanks Tim

              Show
              tsala Helen Foster added a comment - Thanks Tim
              Hide
              stronk7 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
              stronk7 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:
                    Fix Release Date:
                    1/Jul/11