Uploaded image for project: '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

      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.

        Gliffy Diagrams

          Issue Links

            Activity

            Show
            tlevi Tony Levi added a comment - https://github.com/tlevi/moodle/tree/mdl29981
            Hide
            stronk7 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
            stronk7 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
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Adding Petr and Sam here...

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Adding Petr and Sam here...
            Hide
            tlevi 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
            tlevi 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
            tlevi Tony Levi added a comment -

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

            Show
            tlevi Tony Levi added a comment - Added a warning to this setting on my branch for admins due to severity of the impact.
            Hide
            dougiamas 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
            dougiamas 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
            tlevi 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
            tlevi 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
            skodak Petr Skoda added a comment -

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

            Show
            skodak Petr Skoda added a comment - Could you please test this in latest master? I suppose MDL-32109 could resolve this.
            Hide
            salvetore 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
            salvetore 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
            salvetore 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
            salvetore 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: