Details

      Description

      We originally wanted to apply our stylesheets to all HTML content and did so via use of the $CFG->stylesheets variable, and manipulating the content of stored files as they served.

      However, the section of the code fetching the stylesheets was commented out in 2009 (MDL-20204), and the $ufo code was removed back in 2014 (MDL-36669).
      Since then this code has just been wasting cycles by performing totally unnecessary preg_match and preg_replace calls on all HTML content as it is served.

      This issue was originally about fixing that code, but is now about removing it.
      Most places where we did this in Moodle have been templatised (e.g. all outbound e-mails).

      Original description follows

      The forum module sends copy of discussion topics to people who has subscribed the forum. The mail sent is an html page. In this page the head section is empty, so no css link is reported. Subscribers receive text and images, but don't receive neither box nor font information nor colors nor any style information.

      I checked the problem subscribing a forum of moodle.org: I received only text without any css information.

      Add the style information manually in the discussion topic html doesn't solve the problem: they are automatically deleted by moodle.

      (Edited to add description provided by Luciano in a later comment.)

      http://moodle.org/mod/forum/discuss.php?d=187579
      http://tracker.moodle.org/browse/MDL-29152

      see MDL-21120 too.

      SAME AS CLOSED 29152 - OPENED BECAUSE OF SUGGESTION OF MAUNO KOPERLAINEN
      http://moodle.org/mod/forum/discuss.php?d=187581

        Gliffy Diagrams

          Attachments

            Issue Links

              Activity

              Hide
              korpelainen Mauno Korpelainen added a comment -

              MDL-29152 is a partial clone of MDL-21120 which is a subtask of meta bug MDL-20204

              so you can vote also the meta bug...

              Show
              korpelainen Mauno Korpelainen added a comment - MDL-29152 is a partial clone of MDL-21120 which is a subtask of meta bug MDL-20204 so you can vote also the meta bug...
              Hide
              luciano Luciano Biondo added a comment -

              (I will try a better description, as requested by Tim Hunt)

              The forum module sends copy of discussion topics to people who has subscribed the forum. The mail sent is an html page. In this page the head section is empty, so no css link is reported. Subscribers receive text and images, but don't receive neither box nor font information nor colors nor any style information.

              I checked the problem subscribing a forum of moodle.org: I received only text without any css information.

              Add the style information manually in the discussion topic html doesn't solve the problem: they are automatically deleted by moodle.

              The problem is probably due to moodle/mod/forum/lib.php line 1071.

              Moodle 1.9 was not affected by this problem. May be that to copy some line of code from Moodle 1.9 and paste them in Moodle 2.1 is enough.

              Show
              luciano Luciano Biondo added a comment - (I will try a better description, as requested by Tim Hunt) The forum module sends copy of discussion topics to people who has subscribed the forum. The mail sent is an html page. In this page the head section is empty, so no css link is reported. Subscribers receive text and images, but don't receive neither box nor font information nor colors nor any style information. I checked the problem subscribing a forum of moodle.org: I received only text without any css information. Add the style information manually in the discussion topic html doesn't solve the problem: they are automatically deleted by moodle. The problem is probably due to moodle/mod/forum/lib.php line 1071. Moodle 1.9 was not affected by this problem. May be that to copy some line of code from Moodle 1.9 and paste them in Moodle 2.1 is enough.
              Hide
              dobedobedoh Andrew Nicols added a comment -

              Originally commented out as part of 78946b9bdb1299a21f00b88d81c73ad8700c951e by Petr Skoda on Dec 16th 2009

              Show
              dobedobedoh Andrew Nicols added a comment - Originally commented out as part of 78946b9bdb1299a21f00b88d81c73ad8700c951e by Petr Skoda on Dec 16th 2009
              Hide
              salvetore Michael de Raadt added a comment -

              Thanks for persisting with this and clarifying the problem.

              Show
              salvetore Michael de Raadt added a comment - Thanks for persisting with this and clarifying the problem.
              Hide
              salvetore Michael de Raadt added a comment -

              Thanks for reporting this issue.

              We have detected that this issue has been inactive for over a year. It was reported as affecting versions that are no longer supported.

              If you believe that this issue is still relevant to current versions (2.5 and beyond), please comment on the issue. Issues left inactive for a further month will be closed.

              Michael d.

              TW9vZGxlDQo=

              Show
              salvetore Michael de Raadt added a comment - Thanks for reporting this issue. We have detected that this issue has been inactive for over a year. It was reported as affecting versions that are no longer supported. If you believe that this issue is still relevant to current versions (2.5 and beyond), please comment on the issue. Issues left inactive for a further month will be closed. Michael d. TW9vZGxlDQo=
              Hide
              matteo Matteo Scaramuccia added a comment -

              Hi Michael,
              this is still relevant: from the moodle.org notification emails did you notice any difference after the application of the new theme? No difference at all due to this issue: it is required to take a decision about the direction to be taken, a/close this issue as not a bug or b/add some style. See also MDL-36669 (from https://moodle.org/mod/forum/discuss.php?d=167328) where Petr was supposing to create a separate CSS as a possible direction towards (b).

              Show
              matteo Matteo Scaramuccia added a comment - Hi Michael, this is still relevant: from the moodle.org notification emails did you notice any difference after the application of the new theme? No difference at all due to this issue: it is required to take a decision about the direction to be taken, a/close this issue as not a bug or b/add some style. See also MDL-36669 (from https://moodle.org/mod/forum/discuss.php?d=167328 ) where Petr was supposing to create a separate CSS as a possible direction towards (b).
              Hide
              matteo Matteo Scaramuccia added a comment - - edited

              Andrew Nicols Do you think this issue to be address-able for 3.0 (optionally, for the other supported minors too)? Or to close it if it has no - sigh! - sense at all?

              Show
              matteo Matteo Scaramuccia added a comment - - edited Andrew Nicols Do you think this issue to be address-able for 3.0 (optionally, for the other supported minors too)? Or to close it if it has no - sigh! - sense at all?
              Hide
              matteo Matteo Scaramuccia added a comment -

              This is still an issue with Moodle HTML emails, even for https://moodle.org.
              For example, https://moodle.org/mod/forum/discuss.php?d=322852 cannot be easily read as the plain text email message: the attachment is listed above as plain HTML (in GApps):

              ...
              Moodle 2.9.3, 2.8.9 and 2.7.11 are now available
              by Marina Glancy - Monday, November 9, 2015, 2:17 AM
               
              <a href="https://moodle.org/pluginfile.php/51/mod_forum/attachment/1296193/releasechart.png"><img class="icon" alt="Image (PNG)" title="Image (PNG)" src="https://moodle.org/theme/image.php/_s/moodleorgcleaned_moodleorg/core/1446824712/f/png" /></a> <a href="https://moodle.org/pluginfile.php/51/mod_forum/attachment/1296193/releasechart.png">releasechart.png</a><br />
               
              Moodle 2.9.3, 2.8.9 and 2.7.11 are now available via the usual open download channels: https://download.moodle.org or Git.
              ...
              

              and here is the relevant quoted-printable payload:

              --b1_9dbc7532984e3952c02c1e620ae07aca
              Content-Type: text/html; charset=UTF-8
              Content-Transfer-Encoding: quoted-printable
               
              <head>
              </head>
              <body id=3D"email">
              

              while the plain text email is more friendly by ending the message with:

              ...
              Attachment releasechart.png:
              https://moodle.org/pluginfile.php/51/mod_forum/attachment/1296193/releasech=
              art.png
              ...
              

              Added Helen Foster as a watcher, for her convenience.

              Show
              matteo Matteo Scaramuccia added a comment - This is still an issue with Moodle HTML emails, even for https://moodle.org . For example, https://moodle.org/mod/forum/discuss.php?d=322852 cannot be easily read as the plain text email message: the attachment is listed above as plain HTML (in GApps): ... Moodle 2.9.3, 2.8.9 and 2.7.11 are now available by Marina Glancy - Monday, November 9, 2015, 2:17 AM   <a href="https://moodle.org/pluginfile.php/51/mod_forum/attachment/1296193/releasechart.png"><img class="icon" alt="Image (PNG)" title="Image (PNG)" src="https://moodle.org/theme/image.php/_s/moodleorgcleaned_moodleorg/core/1446824712/f/png" /></a> <a href="https://moodle.org/pluginfile.php/51/mod_forum/attachment/1296193/releasechart.png">releasechart.png</a><br />   Moodle 2.9.3, 2.8.9 and 2.7.11 are now available via the usual open download channels: https://download.moodle.org or Git. ... and here is the relevant quoted-printable payload: --b1_9dbc7532984e3952c02c1e620ae07aca Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable   <head> </head> <body id=3D"email"> while the plain text email is more friendly by ending the message with: ... Attachment releasechart.png: https://moodle.org/pluginfile.php/51/mod_forum/attachment/1296193/releasech= art.png ... Added Helen Foster as a watcher, for her convenience.
              Hide
              tsala Helen Foster added a comment -

              Sorry to hear Matteo that this issue is still a problem, and is affecting moodle.org. Updating the affects version and hoping the issue receives some attention.

              Show
              tsala Helen Foster added a comment - Sorry to hear Matteo that this issue is still a problem, and is affecting moodle.org. Updating the affects version and hoping the issue receives some attention.
              Hide
              matteo Matteo Scaramuccia added a comment -

              Hi Helen,
              guessing that there is a new issue with 3.0 even with "textual" emails: https://moodle.org/mod/forum/discuss.php?d=322862#p1297264.
              So to summarize:

              1. There's still no "theming" for the HTML email (this issue)
              2. Something new happened in 3.0: attachment links in HTML emails are written in escaped HTML at the top of the email - top == this oldest issue - and the escaping is causing troubles even for the textual representation of the emails (I'm not used to read email in plain text).
              Show
              matteo Matteo Scaramuccia added a comment - Hi Helen, guessing that there is a new issue with 3.0 even with "textual" emails: https://moodle.org/mod/forum/discuss.php?d=322862#p1297264 . So to summarize: There's still no "theming" for the HTML email (this issue) Something new happened in 3.0: attachment links in HTML emails are written in escaped HTML at the top of the email - top == this oldest issue - and the escaping is causing troubles even for the textual representation of the emails (I'm not used to read email in plain text).
              Hide
              tsala Helen Foster added a comment -

              Thanks Matteo for your comments. I checked the list of forum issues fixed in 3.0 and wondered whether the new 3.0 problem could possibly have been caused by MDL-49682?

              Show
              tsala Helen Foster added a comment - Thanks Matteo for your comments. I checked the list of forum issues fixed in 3.0 and wondered whether the new 3.0 problem could possibly have been caused by MDL-49682 ?
              Hide
              stronk7 Eloy Lafuente (stronk7) added a comment - - edited

              Just for clarification, Matteo Scaramuccia,

              would you please move the new 3.0 HTML mail annoyances to separate issue, keeping this for the original lack of theming. Just to have everything:

              Ciao

              Show
              stronk7 Eloy Lafuente (stronk7) added a comment - - edited Just for clarification, Matteo Scaramuccia , would you please move the new 3.0 HTML mail annoyances to separate issue, keeping this for the original lack of theming. Just to have everything: The discussion: https://moodle.org/mod/forum/discuss.php?d=322862#p1297264 The problems with the attachment links in HTML mails (new issue, maybe not only those links are affected). The problem with text mails ( MDL-52126 , also escaping too much, maybe it's the same issue but np handling them separatedly, IMO). Ciao
              Hide
              matteo Matteo Scaramuccia added a comment -

              TNX Eloy!

              Show
              matteo Matteo Scaramuccia added a comment - TNX Eloy!
              Hide
              brendanheywood Brendan Heywood added a comment -

              Cross linking back to MDL-52990 which adds templates for all outgoing emails.

              Show
              brendanheywood Brendan Heywood added a comment - Cross linking back to MDL-52990 which adds templates for all outgoing emails.
              Hide
              brendanheywood Brendan Heywood added a comment -

              I think this one should be closed as won't fix. Any styles in emails should be added to the new templates either at the forum template level or at the site template level, and there shouldn't be a head at all for the best cross email client compatibility (and if you add one for client specific fixes it should be in the site template not the forum one). The <head> and <body> elements that were present in the forums emails have been removed in MDL-52990.

              I have seen some applications run their web theme through a processor to inline everything for use in emails, but I think that's overkill here and it would just end up with very bloated and probably very sub-optimal html emails. Themers would want better control directly over the email styling and not want it to be the same as the web theme anyway.

              eg https://github.com/tijsverkoyen/CssToInlineStyles

              Show
              brendanheywood Brendan Heywood added a comment - I think this one should be closed as won't fix. Any styles in emails should be added to the new templates either at the forum template level or at the site template level, and there shouldn't be a head at all for the best cross email client compatibility (and if you add one for client specific fixes it should be in the site template not the forum one). The <head> and <body> elements that were present in the forums emails have been removed in MDL-52990 . I have seen some applications run their web theme through a processor to inline everything for use in emails, but I think that's overkill here and it would just end up with very bloated and probably very sub-optimal html emails. Themers would want better control directly over the email styling and not want it to be the same as the web theme anyway. eg https://github.com/tijsverkoyen/CssToInlineStyles
              Hide
              dobedobedoh Andrew Nicols added a comment -

              I'll close this issue once MDL-52990 has been integrated as this is not something that I feel that the forum should do on it's own.

              Show
              dobedobedoh Andrew Nicols added a comment - I'll close this issue once MDL-52990 has been integrated as this is not something that I feel that the forum should do on it's own.
              Hide
              dobedobedoh Andrew Nicols added a comment -

              I was about to close this, but discovered some @todo in the code referencing it. We should use this issue to remove them.

              Show
              dobedobedoh Andrew Nicols added a comment - I was about to close this, but discovered some @todo in the code referencing it. We should use this issue to remove them.
              Hide
              dobedobedoh Andrew Nicols added a comment -

              As I mentioned, I found some TODO notes for this issue.
              The function in question was partially commented out (since 2009) and has been purely wasting cycles since then with expensive calls.

              Killing it once and for all.

              Show
              dobedobedoh Andrew Nicols added a comment - As I mentioned, I found some TODO notes for this issue. The function in question was partially commented out (since 2009) and has been purely wasting cycles since then with expensive calls. Killing it once and for all.
              Hide
              cibot CiBoT added a comment -

              Code verified against automated checks.

              Checked MDL-29738 using repository: git://github.com/andrewnicols/moodle.git

              More information about this report

              Show
              cibot CiBoT added a comment - Code verified against automated checks. Checked MDL-29738 using repository: git://github.com/andrewnicols/moodle.git master (0 errors / 0 warnings) [branch: MDL-29738-master | CI Job ] More information about this report
              Hide
              markn Mark Nelson added a comment -

              Hi Andrew,

              Looks fine to remove this useless functionality.

              One thought - I am unsure about adding the '@deprecated Moodle 3.X.X See MDL-29738' messages for 3.1 and 3.0 since you aren't technically deprecating it (at least as far as the deprecation process goes in Moodle), but are simply just removing it's functionality. I would say it would be fine to leave your code changes and not bother with the @deprecated tag, but rather edit the PHPDocs to state that the function does nothing now to avoid confusion for someone looking at the code. Either way, can't see this affecting anyone drastically (perhaps the odd person browsing the Moodle source code) so I am not really fussed about the approach you choose, just my personal preference.

              Show
              markn Mark Nelson added a comment - Hi Andrew, Looks fine to remove this useless functionality. One thought - I am unsure about adding the '@deprecated Moodle 3.X.X See MDL-29738 ' messages for 3.1 and 3.0 since you aren't technically deprecating it (at least as far as the deprecation process goes in Moodle), but are simply just removing it's functionality. I would say it would be fine to leave your code changes and not bother with the @deprecated tag, but rather edit the PHPDocs to state that the function does nothing now to avoid confusion for someone looking at the code. Either way, can't see this affecting anyone drastically (perhaps the odd person browsing the Moodle source code) so I am not really fussed about the approach you choose, just my personal preference.
              Hide
              dobedobedoh Andrew Nicols added a comment -

              Thanks Mark,

              As I commented above I think that it doesn't harm either way.
              The function has been doing nothing except replace strings with themselves for the past 7 years, so there is no functionality loss by removing it and warning people not to use it in older versions.

              We can't add a proper deprecation notice via debugging though, but I think it benefits us to do this in this instance.

              I'll submit this for integration now. Much obliged!

              Show
              dobedobedoh Andrew Nicols added a comment - Thanks Mark, As I commented above I think that it doesn't harm either way. The function has been doing nothing except replace strings with themselves for the past 7 years, so there is no functionality loss by removing it and warning people not to use it in older versions. We can't add a proper deprecation notice via debugging though, but I think it benefits us to do this in this instance. I'll submit this for integration now. Much obliged!
              Hide
              stronk7 Eloy Lafuente (stronk7) added a comment -

              Just guessing... what happens with these:

              $ ag '\$CFG\->stylesheets'
              lib/filelib.php
              2626:    foreach ($CFG->stylesheets as $stylesheet) {
               
              mod/chat/gui_header_js/jsupdate.php
              103:/*foreach ($CFG->stylesheets as $stylesheet) {
               
              mod/chat/gui_header_js/jsupdated.php
              95:/*foreach ($CFG->stylesheets as $stylesheet) {
              

              Show
              stronk7 Eloy Lafuente (stronk7) added a comment - Just guessing... what happens with these: $ ag '\$CFG\->stylesheets' lib/filelib.php 2626: foreach ($CFG->stylesheets as $stylesheet) {   mod/chat/gui_header_js/jsupdate.php 103:/*foreach ($CFG->stylesheets as $stylesheet) {   mod/chat/gui_header_js/jsupdated.php 95:/*foreach ($CFG->stylesheets as $stylesheet) {
              Hide
              markn Mark Nelson added a comment -

              The first one is removed in this patch on all supported branches and the last two (belonging to mod_chat) are removed in MDL-55114 albeit only for master. MDL-55114 could be backported with no issues but technically it is an improvement. Your call.

              Show
              markn Mark Nelson added a comment - The first one is removed in this patch on all supported branches and the last two (belonging to mod_chat) are removed in MDL-55114 albeit only for master. MDL-55114 could be backported with no issues but technically it is an improvement. Your call.
              Hide
              markn Mark Nelson added a comment -

              Andrew Nicols - it seems odd to have '@deprecated Moodle 3.0.5 See MDL-29738' but then only throwing a debugging message in 3.2.

              Show
              markn Mark Nelson added a comment - Andrew Nicols - it seems odd to have '@deprecated Moodle 3.0.5 See MDL-29738 ' but then only throwing a debugging message in 3.2.
              Hide
              dobedobedoh Andrew Nicols added a comment -

              Indeed... I'll let Eloy decide
              Will try and check back here later this evening to see if I need to make changes.
              As I say, it's a pointless function. Better people see not to use it early anyway. The deprecation notice isn't there in stables so it won't actually break anything for them, but if someone is deving against 30, then they will possibly see that notice and not to use it.

              Show
              dobedobedoh Andrew Nicols added a comment - Indeed... I'll let Eloy decide Will try and check back here later this evening to see if I need to make changes. As I say, it's a pointless function. Better people see not to use it early anyway. The deprecation notice isn't there in stables so it won't actually break anything for them, but if someone is deving against 30, then they will possibly see that notice and not to use it.
              Hide
              stronk7 Eloy Lafuente (stronk7) added a comment -

              Uhm,

              this is soo minor... as far as we are deleting the only 2 possible uses in core for a function that was doing nothing... I hardly can imagine anybody was using it at all. So I'd recommend:

              • in stables, deprecated + sort of debugging('this was doing nothing so better stop using it, will be removed in 3.2')
                (completely erasing current code that does nothing). If anybody was using it there, will get 1) the same result and 2) the debugging.
              • in master, straight remove and forget. With a comment in upgrade.txt. If anybody was using it, it will start failing today. 6 months to fix it. It's documented.

              Again, assuming nobody is using it coz it was doing nothing but wasting resources. How does that sound?

              Show
              stronk7 Eloy Lafuente (stronk7) added a comment - Uhm, this is soo minor... as far as we are deleting the only 2 possible uses in core for a function that was doing nothing... I hardly can imagine anybody was using it at all. So I'd recommend: in stables, deprecated + sort of debugging('this was doing nothing so better stop using it, will be removed in 3.2') (completely erasing current code that does nothing). If anybody was using it there, will get 1) the same result and 2) the debugging. in master, straight remove and forget. With a comment in upgrade.txt. If anybody was using it, it will start failing today. 6 months to fix it. It's documented. Again, assuming nobody is using it coz it was doing nothing but wasting resources. How does that sound?
              Hide
              dobedobedoh Andrew Nicols added a comment -

              heh - okay, Agreed. It's hardly worth wasting time over.

              I've pushed new branches for you.

              Show
              dobedobedoh Andrew Nicols added a comment - heh - okay, Agreed. It's hardly worth wasting time over. I've pushed new branches for you.
              Hide
              stronk7 Eloy Lafuente (stronk7) added a comment -

              Integrated (30, 31 and master), thanks!

              Show
              stronk7 Eloy Lafuente (stronk7) added a comment - Integrated (30, 31 and master), thanks!
              Hide
              ryanwyllie Ryan Wyllie added a comment -

              Thanks Andrew, I can confirm that the function is mentioned in upgrade.txt and depricationlib.php for 31 and 30 and is only mentioned in upgrade.txt in master (which seems intentional according to the comments above).

              Testing passed.

              Show
              ryanwyllie Ryan Wyllie added a comment - Thanks Andrew, I can confirm that the function is mentioned in upgrade.txt and depricationlib.php for 31 and 30 and is only mentioned in upgrade.txt in master (which seems intentional according to the comments above). Testing passed.
              Hide
              stronk7 Eloy Lafuente (stronk7) added a comment -

              Closing this as fixed, once it's already out there, upstream. Soon the orange hordes will start enjoying with your awesome changes, many many thanks!

              Each problem that I solved became a rule,
              which served afterwards to solve other problems.
              – René Descartes

              Show
              stronk7 Eloy Lafuente (stronk7) added a comment - Closing this as fixed, once it's already out there, upstream. Soon the orange hordes will start enjoying with your awesome changes, many many thanks! Each problem that I solved became a rule, which served afterwards to solve other problems. – René Descartes

                People

                • Votes:
                  8 Vote for this issue
                  Watchers:
                  12 Start watching this issue

                  Dates

                  • Created:
                    Updated:
                    Resolved:
                    Fix Release Date:
                    11/Jul/16