Uploaded image for project: '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 Master Branch:
    • Sprint:
      FRONTEND Sprint 9
    • 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

          Attachments

            Issue Links

              Activity

              Hide
              jerome 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
              jerome 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
              rwijaya 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
              rwijaya 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
              rwijaya Rossiani Wijaya added a comment -

              Submitting for integration review.

              Show
              rwijaya Rossiani Wijaya added a comment - Submitting for integration review.
              Hide
              stronk7 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
              stronk7 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 CiBoT added a comment -

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

              Show
              cibot CiBoT added a comment - Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!
              Hide
              samhemelryk 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
              samhemelryk 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 CiBoT added a comment -

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

              Show
              cibot CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
              Hide
              rwijaya 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
              rwijaya 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 CiBoT added a comment -

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

              Show
              cibot CiBoT added a comment - Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!
              Hide
              marina 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 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
              rwijaya 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
              rwijaya 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 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 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
              rwijaya Rossiani Wijaya added a comment -

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

              Thanks Marina

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

              Thanks Rossi, integrated in 2.5, 2.6 and master

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

              Thanks for fixing this Rossie,

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

              Passing...

              Show
              rajeshtaneja Rajesh Taneja added a comment - Thanks for fixing this Rossie, No errors/warning appeared on listed modules. Works fine. Passing...
              Hide
              samhemelryk 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
              samhemelryk 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:
                    Fix Release Date:
                    10/Mar/14