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

Simplify xhprof_table_sql implementation

    Details

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

      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

        Gliffy Diagrams

          Attachments

            Issue Links

              Activity

              Show
              stronk7 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
              stronk7 Eloy Lafuente (stronk7) added a comment -
              Show
              stronk7 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
              stronk7 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
              stronk7 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
              timhunt 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
              timhunt 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
              timhunt 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
              timhunt 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
              stronk7 Eloy Lafuente (stronk7) added a comment -

              Done! Thanks!

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

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

              Show
              stronk7 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:
                    Fix Release Date:
                    5/May/11