Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.2
    • Fix Version/s: None
    • Component/s: Themes
    • Labels:
    • Affected Branches:
      MOODLE_22_STABLE
    • Rank:
      17290

      Description

      As discussed at HQ meeting today, a review of the current theme code would be nice to have.

        Activity

        Hide
        David Mudrak added a comment -

        I was asked by Martin D. to look at PHP code of the MyMobile theme. Marina did a great detailed review couple of days ago and I can just second all the points she mentioned. Here I add some other notes gathered while reading the code.

        • All $strings with the 'desc' suffix to be renamed to '_desc' suffix, eg $string['mtext_desc']
        • Files, classes, methods and functions must have proper and usefule phpdoc blocks. Files must start with standard header incluging GNU/GPL legal notice
        • General code cleanup - trailing whitespaces, code alignment
        • The code contains hard-coded strings - eg "Search Settings.."
        • Moodle uses single quotes, double quotes are used for SQL statements and when they help overall readibility of the code
        • Output methods should use html_writer methods instead of hard-coding HTML
        • Code like
          html_writer::tag('li data-role="list-divider"', $content)

          is unacceptable

        • Using optional_param() in files like theme's config.php and layout/general.php is generally pretty hacky and may easily lead to conflicts with other plugins that may use params like 'blocks', 'settings' or 'loadedby' for their purpose. If passing these params is really needed, please choose a better name, for example with a frankenstyle prefix.
        • We use different patterns for AJAX support. AJAX scripts must define AJAX_SCRIPT constant. The hack with 'loadedby' parameter is not correct from this point of view.
        • The modifications of $this->page->url are not valid in layout/general.php as the URL is instance of moodle_url class and should not be treated this way as a string
        • The theme claims DOCTYPE to be XHTML 1.0 Strict using the standard namespace xmlns="http://www.w3.org/1999/xhtml". But then, additional HTML tags attributes (those data-*) used by jquery-mobile framework are not valid. AFAIK the custom namespace should be used.
        Show
        David Mudrak added a comment - I was asked by Martin D. to look at PHP code of the MyMobile theme. Marina did a great detailed review couple of days ago and I can just second all the points she mentioned. Here I add some other notes gathered while reading the code. All $strings with the 'desc' suffix to be renamed to '_desc' suffix, eg $string ['mtext_desc'] Files, classes, methods and functions must have proper and usefule phpdoc blocks. Files must start with standard header incluging GNU/GPL legal notice General code cleanup - trailing whitespaces, code alignment The code contains hard-coded strings - eg "Search Settings.." Moodle uses single quotes, double quotes are used for SQL statements and when they help overall readibility of the code Output methods should use html_writer methods instead of hard-coding HTML Code like html_writer::tag('li data-role= "list-divider" ', $content) is unacceptable Using optional_param() in files like theme's config.php and layout/general.php is generally pretty hacky and may easily lead to conflicts with other plugins that may use params like 'blocks', 'settings' or 'loadedby' for their purpose. If passing these params is really needed, please choose a better name, for example with a frankenstyle prefix. We use different patterns for AJAX support. AJAX scripts must define AJAX_SCRIPT constant. The hack with 'loadedby' parameter is not correct from this point of view. The modifications of $this->page->url are not valid in layout/general.php as the URL is instance of moodle_url class and should not be treated this way as a string The theme claims DOCTYPE to be XHTML 1.0 Strict using the standard namespace xmlns="http://www.w3.org/1999/xhtml". But then, additional HTML tags attributes (those data-*) used by jquery-mobile framework are not valid. AFAIK the custom namespace should be used.
        Hide
        Martin Dougiamas added a comment -

        John, I just wanted to make sure you'd seen this subtask of the original bug, with some extra review from David Mudrak.

        Show
        Martin Dougiamas added a comment - John, I just wanted to make sure you'd seen this subtask of the original bug, with some extra review from David Mudrak.
        Hide
        John Stabinger added a comment - - edited

        Thanks David. I think I've got most of these earlier, and some no longer exist (like the AJAX issue). However others were very helpful/insightful. Thanks.

        Quick question on two things:

        I'm not sure how use the html_writer to write something like: html_writer::tag('li data-role="list-divider"', $content) in a proper way. Can you point me to some references on using it? Would writing the above like below work for standards purposes?

        $attributes['data-role'] = 'list-divider';
        $content = html_writer::tag('li', $content, $attributes);

        I'm also not sure what you mean in reference to the $this->page->url. Is there some other way of getting the URL?

        Show
        John Stabinger added a comment - - edited Thanks David. I think I've got most of these earlier, and some no longer exist (like the AJAX issue). However others were very helpful/insightful. Thanks. Quick question on two things: I'm not sure how use the html_writer to write something like: html_writer::tag('li data-role="list-divider"', $content) in a proper way. Can you point me to some references on using it? Would writing the above like below work for standards purposes? $attributes ['data-role'] = 'list-divider'; $content = html_writer::tag('li', $content, $attributes); I'm also not sure what you mean in reference to the $this->page->url. Is there some other way of getting the URL?
        Hide
        David Mudrak added a comment -

        Yes, html_writer does not have a whitelist of allowed attributes. So the code like

        $content = html_writer::tag('li', $content, array('data-role' => 'list-divider'));
        

        is the valid usage - though there is still the issue with the DOCTYPE XHTML 1.1 but I am not sure how the jquery-mobile framework approaches that.

        Look at the public methods provided by moodle_url class in lib.weblib.php that deal with params. Shortly, to get an URL with a param added, you may want to use something like

        $myurl = new moodle_url($this->page->url, array('theme_mymobile_blocks' => true));
        

        and to get that URL as a string, use $myurl->out() method with the parameter $escaped set as needed.

        Show
        David Mudrak added a comment - Yes, html_writer does not have a whitelist of allowed attributes. So the code like $content = html_writer::tag('li', $content, array('data-role' => 'list-divider')); is the valid usage - though there is still the issue with the DOCTYPE XHTML 1.1 but I am not sure how the jquery-mobile framework approaches that. Look at the public methods provided by moodle_url class in lib.weblib.php that deal with params. Shortly, to get an URL with a param added, you may want to use something like $myurl = new moodle_url($ this ->page->url, array('theme_mymobile_blocks' => true )); and to get that URL as a string, use $myurl->out() method with the parameter $escaped set as needed.
        Hide
        Martin Dougiamas added a comment -

        See parent for followups.

        Show
        Martin Dougiamas added a comment - See parent for followups.

          People

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

            Dates

            • Created:
              Updated:
              Resolved: