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

Calendar view.php doesn't get put through format_text() required for multilang language support

    Details

    • Testing Instructions:
      Hide

      Prerequisites:

      • Ensure that multilang filter is enabled.
      • The following assumes that your Moodle installation is setup to handle English and French language. You may need to change the languages in the strings to be inserted in order to accommodate the languages you have enabled on your test site.

      As an administrator:

      1. Add an event that would normally appear in the calendar.
      2. For the description, specify: <span lang="en" class="multilang">English</span><span lang="fr" class="multilang">French</span>
      3. View an event by clicking on a date in the calendar.
      4. If you only see English or French and not both, the filtering is working.
      5. In the navigation, click Home > Site Pages > Calendar
      6. In the URL, replace month with upcoming (sorry, I don't know how else to get there)
      7. If you only see English or French and not both in the event description, the filtering is working.
      Show
      Prerequisites: Ensure that multilang filter is enabled. The following assumes that your Moodle installation is setup to handle English and French language. You may need to change the languages in the strings to be inserted in order to accommodate the languages you have enabled on your test site. As an administrator: Add an event that would normally appear in the calendar. For the description, specify: <span lang="en" class="multilang">English</span><span lang="fr" class="multilang">French</span> View an event by clicking on a date in the calendar. If you only see English or French and not both, the filtering is working. In the navigation, click Home > Site Pages > Calendar In the URL, replace month with upcoming (sorry, I don't know how else to get there) If you only see English or French and not both in the event description, the filtering is working.
    • Affected Branches:
      MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE, MOODLE_25_STABLE, MOODLE_26_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-41257-Calendar-Day-and-Upcoming-Events-View-not-processed-through-text-filters

      Description

      Although event titles are processed through the format_text() function, event descriptions in the Day and Upcoming Events views are not in the Moodle Calendar.

      I am working on a multilingual site which makes use of multilang. Without it, the description from every language is included when rendering the page.

      Recommendation

      Add the format_text function in a couple of places in the /calendar/view.php

      ==> Day view on line 127:

      echo $renderer->show_day($calendar);

      Replace with:

      echo format_text($renderer->show_day($calendar),true, false, false);

      ==> Upcoming Events view on line 144:

      echo $renderer->show_upcoming_events($calendar, $lookahead, $maxevents);

      Replace with:

      echo format_text($renderer->show_upcoming_events($calendar, $lookahead, $maxevents), true, false, false);

        Gliffy Diagrams

          Activity

          Hide
          tsala Helen Foster added a comment -

          Michael, thanks for creating this issue and for sharing your solution. I hope it can be reviewed soon.

          Show
          tsala Helen Foster added a comment - Michael, thanks for creating this issue and for sharing your solution. I hope it can be reviewed soon.
          Hide
          michael-milette Michael Milette added a comment -

          Just a friendly reminder that the issue has yet to be reviewed.

          Show
          michael-milette Michael Milette added a comment - Just a friendly reminder that the issue has yet to be reviewed.
          Hide
          abgreeve Adrian Greeve added a comment -

          [Y] Syntax
          [Y] Whitespace
          [N] Output
          [-] Language
          [-] Databases
          [Y] Testing (instructions and automated tests)
          [-] Security
          [-] Documentation
          [N] Git
          [-] Third party code
          [N] Sanity check

          Hello Michael,

          I think that the path that you are following is correct, you just have a few minor things that need fixing.

          1. Output line 127 of calendar/view.php has an addition bracket ')' at the end. This results in errors when trying to view the calendar.
          2. Git The commit message is missing a component section. It should probably read more like the following:

            MDL-41257 calendar: Calendar Day and Upcoming-Events view not processing through....

          3. Sanity check I think that you got lucky with your call to format_text. Your call is going to lib/weblib.php format_text() The parameters for this function doesn't take booleans as you have in your code. Luckily true evaluates to 1 and it uses the constant FORMAT_HTML. I think that this part of the code needs to be changed to correctly use the function format_text.

          Sorry about the time taken getting around to peer review this issue.

          Thanks,

          Show
          abgreeve Adrian Greeve added a comment - [Y] Syntax [Y] Whitespace [N] Output [-] Language [-] Databases [Y] Testing (instructions and automated tests) [-] Security [-] Documentation [N] Git [-] Third party code [N] Sanity check Hello Michael, I think that the path that you are following is correct, you just have a few minor things that need fixing. Output line 127 of calendar/view.php has an addition bracket ')' at the end. This results in errors when trying to view the calendar. Git The commit message is missing a component section. It should probably read more like the following: MDL-41257 calendar: Calendar Day and Upcoming-Events view not processing through.... Sanity check I think that you got lucky with your call to format_text. Your call is going to lib/weblib.php format_text() The parameters for this function doesn't take booleans as you have in your code. Luckily true evaluates to 1 and it uses the constant FORMAT_HTML. I think that this part of the code needs to be changed to correctly use the function format_text. Sorry about the time taken getting around to peer review this issue. Thanks,
          Hide
          michael-milette Michael Milette added a comment -

          Thanks for the feedback. I've implemented the changes as per your recommendation and updated the Pull Master Diff URL to point to the latest version.

          Let me know if there is anything else you need me to do.

          Show
          michael-milette Michael Milette added a comment - Thanks for the feedback. I've implemented the changes as per your recommendation and updated the Pull Master Diff URL to point to the latest version. Let me know if there is anything else you need me to do.
          Hide
          abgreeve Adrian Greeve added a comment -

          Hello Michael,

          The alterations that you have made look great. I do notice however that this patch has two commits. Is the earlier commit part of this issue? It's not directly related to the calendar, but it does update code to use format_text. If it is part of this issue, then the description on this page should probably be updated to include the feedback module, and your call to format_text() should also be updated, as well as the commit message being tidied up to be similar to the second commit. If this isn't to be included, then the commit should be removed from your patch.

          Thank you.

          Show
          abgreeve Adrian Greeve added a comment - Hello Michael, The alterations that you have made look great. I do notice however that this patch has two commits. Is the earlier commit part of this issue? It's not directly related to the calendar, but it does update code to use format_text. If it is part of this issue, then the description on this page should probably be updated to include the feedback module, and your call to format_text() should also be updated, as well as the commit message being tidied up to be similar to the second commit. If this isn't to be included, then the commit should be removed from your patch. Thank you.
          Hide
          michael-milette Michael Milette added a comment - - edited

          Hi Adrian,

          Sorry about that. The feedback module commit is a separate issue listed here in the Moodle tracker. I'm not sure how they ended up connected together. The feedback one must be an old commit because my current version includes the updated calls to format_text(). I didn't even realize that one patch could include two commits.

          Any guidance you could provide on what I need to do to split the two commits would be very much appreciated and I would be happy to do it. Git is still relatively new to me and I am still trying to get a handle on it when things like this happen.

          Best regards,

          Michael

          Show
          michael-milette Michael Milette added a comment - - edited Hi Adrian, Sorry about that. The feedback module commit is a separate issue listed here in the Moodle tracker. I'm not sure how they ended up connected together. The feedback one must be an old commit because my current version includes the updated calls to format_text(). I didn't even realize that one patch could include two commits. Any guidance you could provide on what I need to do to split the two commits would be very much appreciated and I would be happy to do it. Git is still relatively new to me and I am still trying to get a handle on it when things like this happen. Best regards, Michael
          Hide
          abgreeve Adrian Greeve added a comment -

          Yes, of course, no problem.

          1. Check out the branch for this issue.
          2. On the command line you can type in the following to put you into interactive rebase:

            git rebase -i origin/master

            The first couple of lines should be the commits in this issue.

          3. Just delete all of the first line (which is the commit we want to remove).
          4. Save the file, and you will be taken back to the command line.
          5. Push the patch back to github.

          I hope this helps, feel free to ask more questions if you have any queries.

          Show
          abgreeve Adrian Greeve added a comment - Yes, of course, no problem. Check out the branch for this issue. On the command line you can type in the following to put you into interactive rebase: git rebase -i origin/master The first couple of lines should be the commits in this issue. Just delete all of the first line (which is the commit we want to remove). Save the file, and you will be taken back to the command line. Push the patch back to github. I hope this helps, feel free to ask more questions if you have any queries.
          Hide
          michael-milette Michael Milette added a comment -

          Hi Adrian,

          Thank you so much for the simple to follow instructions. I followed your instructions and it worked!

          I updated the Pull Master Diff URL with the new link.

          Thanks again. You saved me a lot of time and effort in figuring this out.

          Let me know if you need anything else.

          Best regards,

          Michael

          Show
          michael-milette Michael Milette added a comment - Hi Adrian, Thank you so much for the simple to follow instructions. I followed your instructions and it worked! I updated the Pull Master Diff URL with the new link. Thanks again. You saved me a lot of time and effort in figuring this out. Let me know if you need anything else. Best regards, Michael
          Hide
          abgreeve Adrian Greeve added a comment -

          Looks good to me. I'll send it on for integration review.

          Show
          abgreeve Adrian Greeve added a comment - Looks good to me. I'll send it on for integration review.
          Hide
          michael-milette Michael Milette added a comment -

          Thanks Adrian!

          Show
          michael-milette Michael Milette added a comment - Thanks Adrian!
          Hide
          marina Marina Glancy added a comment -

          Thanks Michael for finding the bug and working on it. I have to reopen the issue because fix is not in the right place. format_text() should be called only on texts entered by users. It is not called on html generated by renderers or language strings.

          In this particular case the format_text should be applied inside core_calendar_renderer and only where it is needed. I assume (from very quick look through) that it is the line https://github.com/moodle/moodle/blob/master/calendar/renderer.php#L358
          Also line 334 should have format_string().

          Another thing is that it is very recommended to specify context when calling format_text() or format_string(). You should decide whether you want filters to be applied in the context of event (course/module/user/etc) or in the context of the current page. Only if it is current page you can omit 'context' argument.

          Now looking at this all my first choice would be to implement functions calendar_event::get_formatted_description() and calendar_event::get_formatted_name() also since event context is calculated but not publicly availble.

          Show
          marina Marina Glancy added a comment - Thanks Michael for finding the bug and working on it. I have to reopen the issue because fix is not in the right place. format_text() should be called only on texts entered by users. It is not called on html generated by renderers or language strings. In this particular case the format_text should be applied inside core_calendar_renderer and only where it is needed. I assume (from very quick look through) that it is the line https://github.com/moodle/moodle/blob/master/calendar/renderer.php#L358 Also line 334 should have format_string(). Another thing is that it is very recommended to specify context when calling format_text() or format_string(). You should decide whether you want filters to be applied in the context of event (course/module/user/etc) or in the context of the current page. Only if it is current page you can omit 'context' argument. Now looking at this all my first choice would be to implement functions calendar_event::get_formatted_description() and calendar_event::get_formatted_name() also since event context is calculated but not publicly availble.
          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
          michael-milette Michael Milette added a comment -

          Hi Marina,

          Thanks you very much for your feedback. I've reversed the changes to the view.php and updated the renderer.php as suggested. The Pull Master Diff URL has been updated to reflect the new changes.

          I did not specify the context in the format_string function call for the following reasons:

          • The format_string function used (just a few lines away) elsewhere in the same function did not specify the context.
          • I have yet to figure out contexts enough to the point where I can even consider calculating contexts. If you have a code snippet to help here, I would be happy to implement it.
          • There will always be a better way to code Moodle. That being said, it is not my role to do that, just to fix reported issues. Naturally I do my best to work with the community and follow coding guidelines which I believe I have conformed to here.

          I sincerely hope you understand my position and integrate the proposed changes into the core Moodle which are required to support our clients bilingual sites.

          After integrating the change, please feel free to propose any further enhancements you feel are required.

          Thanks again for all your contributions to the Moodle community. I know it's because of people like you that Moodle has great coding standards.

          Best regards,

          Michael

          Show
          michael-milette Michael Milette added a comment - Hi Marina, Thanks you very much for your feedback. I've reversed the changes to the view.php and updated the renderer.php as suggested. The Pull Master Diff URL has been updated to reflect the new changes. I did not specify the context in the format_string function call for the following reasons: The format_string function used (just a few lines away) elsewhere in the same function did not specify the context. I have yet to figure out contexts enough to the point where I can even consider calculating contexts. If you have a code snippet to help here, I would be happy to implement it. There will always be a better way to code Moodle. That being said, it is not my role to do that, just to fix reported issues. Naturally I do my best to work with the community and follow coding guidelines which I believe I have conformed to here. I sincerely hope you understand my position and integrate the proposed changes into the core Moodle which are required to support our clients bilingual sites. After integrating the change, please feel free to propose any further enhancements you feel are required. Thanks again for all your contributions to the Moodle community. I know it's because of people like you that Moodle has great coding standards. Best regards, Michael
          Hide
          marina Marina Glancy added a comment -

          Hi Michael, thanks for update. It's much more correct this way. But at the same time I would really prefer to see the new functions in calendar_event class and respect to the format of description (FORMAT_MOODLE, FORMAT_HTML, etc.) and applying contexts. I agree that we unfortunately very often call format_text() in moodle without specifying context but it's not a good reason to continue this behaviour.

          Show
          marina Marina Glancy added a comment - Hi Michael, thanks for update. It's much more correct this way. But at the same time I would really prefer to see the new functions in calendar_event class and respect to the format of description (FORMAT_MOODLE, FORMAT_HTML, etc.) and applying contexts. I agree that we unfortunately very often call format_text() in moodle without specifying context but it's not a good reason to continue this behaviour.
          Hide
          abgreeve Adrian Greeve added a comment -

          Hello Michael,

          I've had a closer look at the patch and considered Marina's comments.
          Contexts are horrible things to try to get your mind around. There is a documentation here about it http://docs.moodle.org/25/en/Context, but I don't think that it does a good job explaining the complexity of them.
          the functions format_text and format_string really don't have good documentation either. It's not clear that context should be provided in most cases to these functions.

          I probably agree that the ideal solution would be to provide two new methods to the calendar_event class, but it is possible to get the context without creating these.

           
          $context = $event->properties()->context;

          This line could be inserted after line 311 and then used for format_string and format_text.
          You need format_text for the description as format_string will not have the desired outcome (multi language support).
          e.g.

          $table->data[1]->cells[1] = new html_table_cell(format_text($event->description, FORMAT_MOODLE, array('context' => $context)));

          but format_string should still be used for the event name.

          So as it stands your current patch doesn't fix your problem.
          Thanks again for persisting with this issue. We really do greatly appreciate people such as you helping make MOODLE better.

          Show
          abgreeve Adrian Greeve added a comment - Hello Michael, I've had a closer look at the patch and considered Marina's comments. Contexts are horrible things to try to get your mind around. There is a documentation here about it http://docs.moodle.org/25/en/Context , but I don't think that it does a good job explaining the complexity of them. the functions format_text and format_string really don't have good documentation either. It's not clear that context should be provided in most cases to these functions. I probably agree that the ideal solution would be to provide two new methods to the calendar_event class, but it is possible to get the context without creating these. $context = $event->properties()->context; This line could be inserted after line 311 and then used for format_string and format_text. You need format_text for the description as format_string will not have the desired outcome (multi language support). e.g. $table->data[1]->cells[1] = new html_table_cell(format_text($event->description, FORMAT_MOODLE, array('context' => $context))); but format_string should still be used for the event name. So as it stands your current patch doesn't fix your problem. Thanks again for persisting with this issue. We really do greatly appreciate people such as you helping make MOODLE better.
          Hide
          michael-milette Michael Milette added a comment -

          Andrian,

          Thank you very much for your feedback and suggestions.

          I implemented your suggestions which includes adding the context to both the event name and description, and changing the format_string to format_text for the description. Thank you for pointing that last one out. You are correct of course and this was an oversight on my part because I was testing with very short descriptions of only a few words.

          I've updated the link in the Pull Master Diff URL to reflect the latest changes.

          Thanks again for your much appreciated assistance. Seeing your solution, I'm not sure I would have figured out how to get the appropriate context in this situation.

          Best regards,

          Michael

          Show
          michael-milette Michael Milette added a comment - Andrian, Thank you very much for your feedback and suggestions. I implemented your suggestions which includes adding the context to both the event name and description, and changing the format_string to format_text for the description. Thank you for pointing that last one out. You are correct of course and this was an oversight on my part because I was testing with very short descriptions of only a few words. I've updated the link in the Pull Master Diff URL to reflect the latest changes. Thanks again for your much appreciated assistance. Seeing your solution, I'm not sure I would have figured out how to get the appropriate context in this situation. Best regards, Michael
          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
          michael-milette Michael Milette added a comment -

          As requested. Please let me know if I didn't rebase correctly.

          Show
          michael-milette Michael Milette added a comment - As requested. Please let me know if I didn't rebase correctly.
          Hide
          damyon Damyon Wiese added a comment -

          Thanks for working on this Michael,

          I will have to reopen this issue - because this solution does not address the problem. This is because $event->description goes through the magic method get_description of class calendar_event - which calls clean_text on the description - so it strips the multi-lang tags before they are processed by the filter.

          Also note: $event->properties()>context is not needed - $event>context is better because it goes through the magic method __get.

          This is why Marinas suggestion of new methods get_formatted_name + get_formatted_description is the correct solution for this issue.

          Show
          damyon Damyon Wiese added a comment - Thanks for working on this Michael, I will have to reopen this issue - because this solution does not address the problem. This is because $event->description goes through the magic method get_description of class calendar_event - which calls clean_text on the description - so it strips the multi-lang tags before they are processed by the filter. Also note: $event->properties() >context is not needed - $event >context is better because it goes through the magic method __get. This is why Marinas suggestion of new methods get_formatted_name + get_formatted_description is the correct solution for this issue.
          Hide
          damyon Damyon Wiese added a comment -

          Adrian pinged me about this and I checked again - tinymce was breaking my multilang strings when I tested this by inserting p tags. Sorry for the confusion.

          So - to clarify - this patch does work - but - there are still 2 improvements to be made.

          1. - $event->context instead of $event->properties()->context
          2. (new one) - the format_text call for the description should pass $event->format and not hard code FORMAT_MOODLE.

          Thanks for the ping Adrian.

          Show
          damyon Damyon Wiese added a comment - Adrian pinged me about this and I checked again - tinymce was breaking my multilang strings when I tested this by inserting p tags. Sorry for the confusion. So - to clarify - this patch does work - but - there are still 2 improvements to be made. 1. - $event->context instead of $event->properties()->context 2. (new one) - the format_text call for the description should pass $event->format and not hard code FORMAT_MOODLE. Thanks for the ping Adrian.
          Hide
          damyon Damyon Wiese added a comment -

          Also - this is a bug so will need to be backported to all stable branches.

          Show
          damyon Damyon Wiese added a comment - Also - this is a bug so will need to be backported to all stable branches.
          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
          michael-milette Michael Milette added a comment -

          Thanks for the feedback Damyon. I've applied the changes you requested and updated the Pull Master Diff URL.

          Best regards,

          Michael

          Show
          michael-milette Michael Milette added a comment - Thanks for the feedback Damyon. I've applied the changes you requested and updated the Pull Master Diff URL. Best regards, Michael
          Hide
          abgreeve Adrian Greeve added a comment -

          I added the other branches for backporting the fix. Using mdk I removed the git info for the master branch, so here it is again - git://github.com/michael-milette/moodle.git

          Show
          abgreeve Adrian Greeve added a comment - I added the other branches for backporting the fix. Using mdk I removed the git info for the master branch, so here it is again - git://github.com/michael-milette/moodle.git
          Hide
          damyon Damyon Wiese added a comment -

          Thanks Michael and Adrian,

          Looks good now - I tested on 24 and master with no issues. Integrated to 24, 25, 26 and master.

          Show
          damyon Damyon Wiese added a comment - Thanks Michael and Adrian, Looks good now - I tested on 24 and master with no issues. Integrated to 24, 25, 26 and master.
          Hide
          samhemelryk Sam Hemelryk added a comment -

          Tested and passed thanks guys

          Show
          samhemelryk Sam Hemelryk added a comment - Tested and passed thanks guys
          Hide
          poltawski Dan Poltawski added a comment -

          Congratulations, this change has now made its way upstream. Thanks for your contribution!

          “ Always code as if the guy who ends up maintaining your code will be a violent psychopath who knows where you live. ” - Rick Osborne

          Show
          poltawski Dan Poltawski added a comment - Congratulations, this change has now made its way upstream. Thanks for your contribution! “ Always code as if the guy who ends up maintaining your code will be a violent psychopath who knows where you live. ” - Rick Osborne
          Hide
          michael-milette Michael Milette added a comment -

          Thanks Dan Poltawski, Sam Hemelryk,Damyyon Wiese, Adrian Greeve, CiBoT, Eloy Lafuente (stronk7), Marina Glancy and Helen Foster for all your time and effort into helping me get this patch integrated into Moodle for everyone's benefit. Your time and effort is very much appreciated.

          Best regards,

          Michael Milette

          Show
          michael-milette Michael Milette added a comment - Thanks Dan Poltawski, Sam Hemelryk,Damyyon Wiese, Adrian Greeve, CiBoT, Eloy Lafuente (stronk7), Marina Glancy and Helen Foster for all your time and effort into helping me get this patch integrated into Moodle for everyone's benefit. Your time and effort is very much appreciated. Best regards, Michael Milette
          Hide
          sylvaticus Antonello Lobianco added a comment -

          Hello, I would like re-open this bug. If the event description contains a <div> the multilingual filter is removed.

          For example the following code:

          <span lang="en" class="multilang">
          <div>A test in English</div>
          </span><span lang="fr" class="multilang">
          <div>A test in French</div>
          </span>

          works perfectly in a page or in a HTML block, but it doesn't work in a calendar description (and, if it matters, also in the theme footer).

          Show
          sylvaticus Antonello Lobianco added a comment - Hello, I would like re-open this bug. If the event description contains a <div> the multilingual filter is removed. For example the following code: <span lang="en" class="multilang"> <div>A test in English</div> </span><span lang="fr" class="multilang"> <div>A test in French</div> </span> works perfectly in a page or in a HTML block, but it doesn't work in a calendar description (and, if it matters, also in the theme footer).
          Hide
          tsala Helen Foster added a comment -

          Hi Antonello,

          Rather than having this issue re-opened, please could you create a new issue for the problem you've found and then link it to this issue.

          Show
          tsala Helen Foster added a comment - Hi Antonello, Rather than having this issue re-opened, please could you create a new issue for the problem you've found and then link it to this issue.

            People

            • Votes:
              1 Vote for this issue
              Watchers:
              9 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:
                Fix Release Date:
                13/Jan/14