Moodle
  1. Moodle
  2. MDL-42339

Remove double formatting set_title/set_heading over codebase

    Details

    • Testing Instructions:
      Hide

      List of modules to test:

      1. chat
      2. choice
      3. data
      4. feedback
      5. forum
      6. glossary
      7. lesson
      8. lti
      9. page
      10. quiz
      11. survey
      12. wiki
      13. badges
      14. user/emailupdate.php

      Browse around the modules above and make sure there is no page title and header error displayed. Sample pages need to be looked at: view, edit, report, forum post, and other that specifically created for the module.

      Show
      List of modules to test: chat choice data feedback forum glossary lesson lti page quiz survey wiki badges user/emailupdate.php Browse around the modules above and make sure there is no page title and header error displayed. Sample pages need to be looked at: view, edit, report, forum post, and other that specifically created for the module.
    • Affected Branches:
      MOODLE_26_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE, MOODLE_26_STABLE, MOODLE_27_STABLE
    • Pull from Repository:
    • Pull 2.6 Branch:
      MDL-42339_m26
    • Pull Master Branch:
    • Story Points (Obsolete):
      13
    • Sprint:
      FRONTEND Sprint 9

      Description

      Both $PAGE->set_title() and $PAGE->set_heading() already process the contents passed with format_string(). So any call of this type:

      • $PAGE->set_title(format_string(… (there are 42 of this, at least)
      • $PAGE->set_heading(format_string(… (there are 25 of this, at least)

      Context in format_string() calls is only needed if it does not match current $PAGE->context OR if it has not been set. And any of these operations set it:

      • require_login() calls passing the 3rd, $cm parameter.
      • $PAGE->set_cm() calls.
      • $PAGE->set_context() explicit calls.

      This is about to search and destroy all those incorrect uses, looking to each carefully, ensuring that context is always set.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            Jérôme Mouneyrac added a comment -

            Just confirm that in forum/post.php the double format_string were actually not useful, except that everything is good

            Show
            Jérôme Mouneyrac added a comment - Just confirm that in forum/post.php the double format_string were actually not useful, except that everything is good
            Hide
            Rossiani Wijaya added a comment -

            Hi Jerome,

            I think it is not necessary to have format_string for each string. I tried both with double format_string and removing them all, it is outputting the same result.

            Thank you for reviewing.

            Show
            Rossiani Wijaya added a comment - Hi Jerome, I think it is not necessary to have format_string for each string. I tried both with double format_string and removing them all, it is outputting the same result. Thank you for reviewing.
            Hide
            Rossiani Wijaya added a comment -

            Submitting for integration review.

            Show
            Rossiani Wijaya added a comment - Submitting for integration review.
            Hide
            Eloy Lafuente (stronk7) added a comment -

            The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

            TIA and ciao

            Show
            Eloy Lafuente (stronk7) added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
            Hide
            CiBoT added a comment -

            Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!

            Show
            CiBoT added a comment - Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!
            Hide
            Sam Hemelryk added a comment -

            Hi Rosie,

            Thanks for making these changes, things look 95% there, but I did note a coding error in mod/feedback/mapcourse.php.
            Could you please make the following two changes:

            1. Fix the coding errors in mod/feedback/mapcourse.php
            2. Improve the test instructions so that the tester has an indication of each page that needs to be viewed, presently the tester would likely just visit the module frontpage and call it passed leaving coding errors like the above going unnoticed.

            Cheers
            Sam

            Show
            Sam Hemelryk added a comment - Hi Rosie, Thanks for making these changes, things look 95% there, but I did note a coding error in mod/feedback/mapcourse.php. Could you please make the following two changes: Fix the coding errors in mod/feedback/mapcourse.php Improve the test instructions so that the tester has an indication of each page that needs to be viewed, presently the tester would likely just visit the module frontpage and call it passed leaving coding errors like the above going unnoticed. Cheers Sam
            Hide
            CiBoT added a comment -

            Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

            Show
            CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
            Hide
            Rossiani Wijaya added a comment -

            Hi Sam,

            1. Fixed the patch that caused by an extra bracket.
            2. Improved the testing instructions.

            Thanks.

            Sending for integration review.

            Show
            Rossiani Wijaya added a comment - Hi Sam, 1. Fixed the patch that caused by an extra bracket. 2. Improved the testing instructions. Thanks. Sending for integration review.
            Hide
            CiBoT added a comment -

            Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!

            Show
            CiBoT added a comment - Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!
            Hide
            Marina Glancy added a comment -

            Thanks Rossi, this is a very interesting fact that you've spotted about double formatting when using those functions.

            But I'm not 100% sure that I agree with your solution.
            For the start, not every heading needs to be formatted, examples:
            $PAGE->set_title(get_string(...)) - does not need any formatting

            Also when title consists of course/module/whatever name PLUS some string, for example:
            $PAGE->set_heading(format_string($course->fullname, true, array('context' => $coursecontext)) . ': ' . $hdr);

            Another example is when we need non-page context. For example, course name on the module page should be formatted to course context but the page context is module, so calling format_text() without arguments is not correct (and this is what set_title() is doing).

            Also to note that there are many places that you did not change, because grep would not show them, for example:
            https://github.com/moodle/moodle/blob/master/badges/newbadge.php#L52..L54

            BTW I have a plugin for testing format_string() / format_text()
            https://github.com/marinaglancy/moodle-filter_testcontext

            What I would suggest to do is (for master only): add argument to the function set_title() and set_heading() specifying whether the string is already formatted (default to false). And use it wisely.
            Maybe we should not grep all usages of set_title/set_heading but instead go by component and correct all wrong formatting in headings/navigation.

            Please tell me what you think.

            Show
            Marina Glancy added a comment - Thanks Rossi, this is a very interesting fact that you've spotted about double formatting when using those functions. But I'm not 100% sure that I agree with your solution. For the start, not every heading needs to be formatted, examples: $PAGE->set_title(get_string(...)) - does not need any formatting Also when title consists of course/module/whatever name PLUS some string, for example: $PAGE->set_heading(format_string($course->fullname, true, array('context' => $coursecontext)) . ': ' . $hdr); Another example is when we need non-page context. For example, course name on the module page should be formatted to course context but the page context is module, so calling format_text() without arguments is not correct (and this is what set_title() is doing). Also to note that there are many places that you did not change, because grep would not show them, for example: https://github.com/moodle/moodle/blob/master/badges/newbadge.php#L52..L54 BTW I have a plugin for testing format_string() / format_text() https://github.com/marinaglancy/moodle-filter_testcontext What I would suggest to do is (for master only): add argument to the function set_title() and set_heading() specifying whether the string is already formatted (default to false). And use it wisely. Maybe we should not grep all usages of set_title/set_heading but instead go by component and correct all wrong formatting in headings/navigation. Please tell me what you think.
            Hide
            Rossiani Wijaya added a comment -

            Hi Marina,

            I'm just following Eloy's suggestion from MDL-41797.

            I agree on fixing the set_title() and set_heading() for master only and I think it should be address as separate issue.

            so for this issue, it should be for 2.5 and 2.6 only.

            Show
            Rossiani Wijaya added a comment - Hi Marina, I'm just following Eloy's suggestion from MDL-41797 . I agree on fixing the set_title() and set_heading() for master only and I think it should be address as separate issue. so for this issue, it should be for 2.5 and 2.6 only.
            Hide
            Marina Glancy added a comment -

            Sorry, Rossi, we can not integrate code in stables only. This way it can be very easily forgotten to amend master. And especially since you will be leaving us soon.
            Most of the changes in this branch can go to master as well. There are just string concatenations that need other approach.

            I have a suggestion to you: how about you leave in this branch only cases without string concatenations, such as "set_title(format_string(XXX))"
            The rest we will convert in master properly.

            Show
            Marina Glancy added a comment - Sorry, Rossi, we can not integrate code in stables only. This way it can be very easily forgotten to amend master. And especially since you will be leaving us soon. Most of the changes in this branch can go to master as well. There are just string concatenations that need other approach. I have a suggestion to you: how about you leave in this branch only cases without string concatenations, such as "set_title(format_string(XXX))" The rest we will convert in master properly.
            Hide
            Rossiani Wijaya added a comment -

            Updated all patches to leave format_string() for concatenation strings.

            Thanks Marina

            Show
            Rossiani Wijaya added a comment - Updated all patches to leave format_string() for concatenation strings. Thanks Marina
            Hide
            Marina Glancy added a comment -

            Thanks Rossi, integrated in 2.5, 2.6 and master

            Show
            Marina Glancy added a comment - Thanks Rossi, integrated in 2.5, 2.6 and master
            Hide
            Rajesh Taneja added a comment -

            Thanks for fixing this Rossie,

            No errors/warning appeared on listed modules. Works fine.

            Passing...

            Show
            Rajesh Taneja added a comment - Thanks for fixing this Rossie, No errors/warning appeared on listed modules. Works fine. Passing...
            Hide
            Sam Hemelryk added a comment -

            This weeks weekly release is now available and includes your code.
            A big pat on the back to you again for once more being a cog in the Moodle machine.

            Best wishes, the Moodle integration team.

            Show
            Sam Hemelryk added a comment - This weeks weekly release is now available and includes your code. A big pat on the back to you again for once more being a cog in the Moodle machine. Best wishes, the Moodle integration team.

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved:

                  Agile