Moodle
  1. Moodle
  2. MDL-7541

Diff on history page not always working

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.6.3, 1.7
    • Fix Version/s: 1.8.1, 1.9
    • Component/s: None
    • Labels:
      None
    • Environment:
      PHP5/MySQL5/Apache2 on Debian Linux server
      Browsing with firefox 2 on Win2K
    • Database:
      MySQL
    • Affected Branches:
      MOODLE_16_STABLE, MOODLE_17_STABLE
    • Fixed Branches:
      MOODLE_18_STABLE, MOODLE_19_STABLE
    • Rank:
      27773

      Description

      The diff on the history page is not always working (= doesn't show the added content or doesn't show anything at all anymore)
      I could reproduce this by pasting some content from a Word document in the wiki.
      Since teachers find it important to be able to know what each student add, I've set the priority to major.
      Tested on 1.6.3 and 1.7
      Also found this forum discussion describing the same problem: http://moodle.org/mod/forum/discuss.php?d=54529

        Activity

        Hide
        Eric Merrill added a comment -

        <P>This appears to effect all versions, at least 1.5.3+ to 1.8
        <P>I believe I've found the fix for the problem.
        <P>In the file:<BR>
        moodle/mod/wiki/ewiki/plugins/moodle/diff.php
        <P>Line 17 and 18 (just below this label):
        /// Replace <p>&nbsp;</p>
        <P>The regexes are:<BR>
        <STRONG>(<p.<EM>>(&nbsp;|\s+)</p>|<p.</EM>></p>)</STRONG>i
        <P>That is bad, they should be:<BR>
        <STRONG>(<p.<EM>>(&nbsp;|\s+)</p>|<p.</EM>^>></p>)</STRONG>i
        <P>Basically the part:<BR>
        <p.*></p> will also match <p><b>test</b></p> because the "><b>test</b" counts as *, so you have <p"><b>test</b"></p> which it thinks is an empty <p> block.
        <P>The ^> says that there cant be any >'s in the *, which eliminates the problem

        Show
        Eric Merrill added a comment - <P>This appears to effect all versions, at least 1.5.3+ to 1.8 <P>I believe I've found the fix for the problem. <P>In the file:<BR> moodle/mod/wiki/ewiki/plugins/moodle/diff.php <P>Line 17 and 18 (just below this label): /// Replace <p>&nbsp;</p> <P>The regexes are:<BR> <STRONG>(<p.<EM>>(&nbsp;|\s+)</p>|<p.</EM>></p>)</STRONG>i <P>That is bad, they should be:<BR> <STRONG>(<p.<EM>>(&nbsp;|\s+)</p>|<p.</EM>^>></p>)</STRONG>i <P>Basically the part:<BR> <p.*></p> will also match <p><b>test</b></p> because the "><b>test</b" counts as *, so you have <p"><b>test</b"></p> which it thinks is an empty <p> block. <P>The ^> says that there cant be any >'s in the *, which eliminates the problem
        Hide
        Eric Merrill added a comment -

        This appears to effect all versions, at least 1.5.3+ to 1.8

        I believe I've found the fix for the problem.

        In the file:
        moodle/mod/wiki/ewiki/plugins/moodle/diff.php

        Line 17 and 18 (just below this label):
        /// Replace <p> </p>

        The regexes are:
        #(<p.>( |\s+)</p>|<p.></p>)#i

        That is bad, they should be:
        #(<p.>( |\s+)</p>|<p.^>></p>)#i

        Basically the part:
        <p.*></p> will also match <p><b>test</b></p> because the "><b>test</b" counts as *, so you have <p"><b>test</b"></p> which it thinks is an empty <p> block.

        The ^> says that there cant be any >'s in the *, which eliminates the problem

        Show
        Eric Merrill added a comment - This appears to effect all versions, at least 1.5.3+ to 1.8 I believe I've found the fix for the problem. In the file: moodle/mod/wiki/ewiki/plugins/moodle/diff.php Line 17 and 18 (just below this label): /// Replace <p> </p> The regexes are: #(<p. >( |\s+)</p>|<p. ></p>)#i That is bad, they should be: #(<p. >( |\s+)</p>|<p. ^>></p>)#i Basically the part: <p.*></p> will also match <p><b>test</b></p> because the "><b>test</b" counts as *, so you have <p"><b>test</b"></p> which it thinks is an empty <p> block. The ^> says that there cant be any >'s in the *, which eliminates the problem
        Hide
        Eric Merrill added a comment -

        Sorry, didn't realize it would convert the tags...

        Show
        Eric Merrill added a comment - Sorry, didn't realize it would convert the tags...
        Hide
        Michael Spall added a comment -

        Actually the lines should be changed to the following:
        /// Replace <p> </p>
        $content0 = preg_replace('#(<p.>( |\s+)</p>|<p[^>]></p>)#i', "\n", $content0);
        $content = preg_replace('#(<p.>( |\s+)</p>|<p[^>]></p>)#i', "\n", $content);

        Show
        Michael Spall added a comment - Actually the lines should be changed to the following: /// Replace <p> </p> $content0 = preg_replace('#(<p. >( |\s+)</p>|<p [^>] ></p>)#i', "\n", $content0); $content = preg_replace('#(<p. >( |\s+)</p>|<p [^>] ></p>)#i', "\n", $content);
        Hide
        Sam Marshall added a comment -

        Unfortunately I didn't look for this bug until after already tracking down the issue on my own, would have saved me some time as the problems identified here were the correct ones.

        Anyway I have fixed this and applied the patch to 1.8 branch and HEAD.

        I used the regexp:

        <p( [^>]*)?>

        In other words, <p followed optionally by space and some other non-> characters, then >. The space ensures that this won't match other HTML tags beginning with <p> (<param> and <pre> are the only ones, but still). Now that I think of it, it should really be \s not space, but nobody would in practice use other character there, so it's probably ok Anyway it seems to fix the problem at least in my example case.

        Show
        Sam Marshall added a comment - Unfortunately I didn't look for this bug until after already tracking down the issue on my own, would have saved me some time as the problems identified here were the correct ones. Anyway I have fixed this and applied the patch to 1.8 branch and HEAD. I used the regexp: <p( [^>] *)?> In other words, <p followed optionally by space and some other non-> characters, then >. The space ensures that this won't match other HTML tags beginning with <p> (<param> and <pre> are the only ones, but still). Now that I think of it, it should really be \s not space, but nobody would in practice use other character there, so it's probably ok Anyway it seems to fix the problem at least in my example case.
        Hide
        Eric Merrill added a comment -

        "Actually the lines should be changed to the following:"

        Yeah, I was just tinkering and came up with my version. After actually thinking about it, yours is definitely the 'correct' way to do it (hey, mine worked too in my testing )

        Show
        Eric Merrill added a comment - "Actually the lines should be changed to the following:" Yeah, I was just tinkering and came up with my version. After actually thinking about it, yours is definitely the 'correct' way to do it (hey, mine worked too in my testing )

          People

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

            Dates

            • Created:
              Updated:
              Resolved: