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

      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

          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: