Moodle
  1. Moodle
  2. MDL-27034

Simplify xhprof_table_sql implementation

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.0.2, 2.1
    • Fix Version/s: 2.0.3
    • Component/s: Libraries
    • Labels:
    • Difficulty:
      Easy
    • Affected Branches:
      MOODLE_20_STABLE, MOODLE_21_STABLE
    • Fixed Branches:
      MOODLE_20_STABLE
    • Rank:
      17072

      Description

      Once all the improvements present @ MDL-26425 have been committed, it will be possible to simplify a bit the xhprof_table_sql implementation by using standard build_table() method and, instead, override the newly available get_row_class() for stylizing rows.

      Right now the improvements are only available @ 20_STABLE so the initial patch will be for it. Once the change is available @ master, the patch will be applied there and this issue requested for pull.

      Ciao

        Issue Links

          Activity

          Hide
          Eloy Lafuente (stronk7) added a comment -
          Show
          Eloy Lafuente (stronk7) added a comment - Done, the change for 20_STABLE is available @ https://github.com/stronk7/moodle/compare/MOODLE_20_STABLE...MDL-27034_xhprof_table_sql_20
          Hide
          Eloy Lafuente (stronk7) added a comment -
          Show
          Eloy Lafuente (stronk7) added a comment - Done, the change for master is available @ https://github.com/stronk7/moodle/compare/master...MDL-27034_xhprof_table_sql_master
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Weird, something is definitively wrong in the master implementation of tablelib:

          1) Under 20_STABLE, once the patch above is applied I get correct:

          <tr class="r0 referencerun">
          

          2) The same patch, in master, is returning incorrect:

          <tr 0="class" 1="r0 referencerun">
          

          Crazy! Going to comment @ original MDL-26425

          Show
          Eloy Lafuente (stronk7) added a comment - Weird, something is definitively wrong in the master implementation of tablelib: 1) Under 20_STABLE, once the patch above is applied I get correct: <tr class= "r0 referencerun" > 2) The same patch, in master, is returning incorrect : <tr 0= "class" 1= "r0 referencerun" > Crazy! Going to comment @ original MDL-26425
          Hide
          Tim Hunt added a comment -

          It looks like some done in one place is doing array('class' => 'r0 referencerun'), and in the other places is doing array('class', 'r0 referencerun').

          If this code was using html_writer, you would get a debugging message explaining the error.

          Show
          Tim Hunt added a comment - It looks like some done in one place is doing array('class' => 'r0 referencerun'), and in the other places is doing array('class', 'r0 referencerun'). If this code was using html_writer, you would get a debugging message explaining the error.
          Hide
          Tim Hunt added a comment -

          Yes. Me == idiot:

          http://git.moodle.org/gw?p=integration.git;a=commitdiff;h=76dc1e2527b7b682e332c84cb2d38e84f47755d2#patch1

          echo html_writer::start_tag('tr', array('class', implode(' ', $rowclasses)));
          

          Need to chagne , to =>.

          Can you do that before the weekly release is out?

          Show
          Tim Hunt added a comment - Yes. Me == idiot: http://git.moodle.org/gw?p=integration.git;a=commitdiff;h=76dc1e2527b7b682e332c84cb2d38e84f47755d2#patch1 echo html_writer::start_tag('tr', array('class', implode(' ', $rowclasses))); Need to chagne , to =>. Can you do that before the weekly release is out?
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Done! Thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - Done! Thanks!
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Resolving as fixed. PULL-597 and PULL-598 have been created for integration.

          Show
          Eloy Lafuente (stronk7) added a comment - Resolving as fixed. PULL-597 and PULL-598 have been created for integration.

            People

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

              Dates

              • Created:
                Updated:
                Resolved: