Moodle
  1. Moodle
  2. MDL-29981

Turning formatstringstriptags off significantly harms site performance

    Details

    • Testing Instructions:
      Hide

      1. Create fresh moodle installation, time load for "My home"
      2. disable the setting formatstringstriptags.
      3. Reload "my home" - load time may be 2x longer.

      Show
      1. Create fresh moodle installation, time load for "My home" 2. disable the setting formatstringstriptags. 3. Reload "my home" - load time may be 2x longer.
    • Affected Branches:
      MOODLE_20_STABLE, MOODLE_21_STABLE, MOODLE_22_STABLE
    • Rank:
      19526

      Description

      When disabling the option formatstringstriptags, site performance is significantly impaired, up to 2x or even worse.

      Profiling runs show a large amount of time (multiple seconds) can be spent inside the navigation block cleaning up text using purify_html.
      This becomes especially bad on sites with many navigation nodes.

      Something needs to be done to avoid doing format_string=>clean_text=>purify_html, or a significant reduction in purify_html run times.

      One suggestion is to disallow HTML in navigation nodes, i.e act as if formatstringstriptags is always on for navigation items - this can be done by always doing strip_tags and skipping clean_text in this case. See the linked patch for one attempt at this.

        Issue Links

          Activity

          Show
          Tony Levi added a comment - https://github.com/tlevi/moodle/tree/mdl29981
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Uhm...

          not really sure if we can take rid of any cleaning in the navigation menu... for some items surely yes, but others can come from DB... although they are going to be stripped-tags always...

          Show
          Eloy Lafuente (stronk7) added a comment - Uhm... not really sure if we can take rid of any cleaning in the navigation menu... for some items surely yes, but others can come from DB... although they are going to be stripped-tags always...
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Adding Petr and Sam here...

          Show
          Eloy Lafuente (stronk7) added a comment - Adding Petr and Sam here...
          Hide
          Tony Levi added a comment -

          Well, the patch doesn't eliminate any cleaning, just always do it the same way for navigation items (or anyone else who asks).
          Most sites will have $CFG->formatstringstriptags (following default) so I hope strip_tags is enough since forever.

          Show
          Tony Levi added a comment - Well, the patch doesn't eliminate any cleaning, just always do it the same way for navigation items (or anyone else who asks). Most sites will have $CFG->formatstringstriptags (following default) so I hope strip_tags is enough since forever.
          Hide
          Tony Levi added a comment -

          Added a warning to this setting on my branch for admins due to severity of the impact.

          Show
          Tony Levi added a comment - Added a warning to this setting on my branch for admins due to severity of the impact.
          Hide
          Martin Dougiamas added a comment - - edited

          Do we really ever need any HTML in navigation nodes? Surely not.

          My +1 to substitute complex purification with drastic and easy purifying wherever possible.

          Show
          Martin Dougiamas added a comment - - edited Do we really ever need any HTML in navigation nodes? Surely not. My +1 to substitute complex purification with drastic and easy purifying wherever possible.
          Hide
          Tony Levi added a comment -

          At the moment you can toggle this and any tags in a module name will work in the navigation block - personally, it seems a highly undesirable 'feature' that this is even possible

          (example - try making a new module with named "<h1>somemodulename</h1>")

          Show
          Tony Levi added a comment - At the moment you can toggle this and any tags in a module name will work in the navigation block - personally, it seems a highly undesirable 'feature' that this is even possible (example - try making a new module with named "<h1>somemodulename</h1>")
          Hide
          Petr Škoda added a comment -

          Could you please test this in latest master? I suppose MDL-32109 could resolve this.

          Show
          Petr Škoda added a comment - Could you please test this in latest master? I suppose MDL-32109 could resolve this.
          Hide
          Michael de Raadt added a comment -

          Hi, Tony and all.

          It looks like this one slipped through.

          It would be good to get a response to Petr's last question.

          Show
          Michael de Raadt added a comment - Hi, Tony and all. It looks like this one slipped through. It would be good to get a response to Petr's last question.
          Hide
          Michael de Raadt added a comment -

          I suspect this has been resolved, so I'm closing this issue.

          I will continue watching this issue in case it has not been resolved.

          Show
          Michael de Raadt added a comment - I suspect this has been resolved, so I'm closing this issue. I will continue watching this issue in case it has not been resolved.

            People

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

              Dates

              • Created:
                Updated:
                Resolved: