Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 2.2, 2.5.5, 2.6.2
    • Fix Version/s: 2.5.6, 2.6.3
    • Component/s: Wiki (2.x)
    • Labels:
    • Testing Instructions:
      Hide

      1. Create an individual wiki.
      2. Edit and save a page with content.
      3. Search. Verify that you get correct results.
      4. Add additional individual subwikis to that search.
      5. Switch to one. Search. Verify that you get correct results.
      6. Create a collaborative wiki.
      7. Add pages. Search. Verify that you get correct results.

      Show
      1. Create an individual wiki. 2. Edit and save a page with content. 3. Search. Verify that you get correct results. 4. Add additional individual subwikis to that search. 5. Switch to one. Search. Verify that you get correct results. 6. Create a collaborative wiki. 7. Add pages. Search. Verify that you get correct results.
    • Affected Branches:
      MOODLE_22_STABLE, MOODLE_25_STABLE, MOODLE_26_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE, MOODLE_26_STABLE
    • Pull Master Branch:
      wip-MDL-33486-master

      Description

      When using individual wikis, trying to search (/mod/wiki/search.php) returns a completely blank page - no header, footer or debug information.

        Gliffy Diagrams

          Activity

          Hide
          tsala Helen Foster added a comment -

          James, thanks for reporting this issue. Just adding a link to a forum discussion about it: http://moodle.org/mod/forum/discuss.php?d=203934

          Show
          tsala Helen Foster added a comment - James, thanks for reporting this issue. Just adding a link to a forum discussion about it: http://moodle.org/mod/forum/discuss.php?d=203934
          Hide
          cfulton Charles Fulton added a comment -

          I'm not sure if I'm taking the right approach but here's a patch. The problem is that the existing search code doesn't pass a uid parameter to wiki_get_subwiki_by_group() so that call always fails with individual wikis. For some reason it's set to return if that fails, thus the blank screen (odd).

          My approach adds a subwiki parameter to the search form, since we already know what subwiki we're on when searching and as far as I can tell search only searches within a subwiki anyway. I can't see a reason not to do it this way but I may have missed something.

          Show
          cfulton Charles Fulton added a comment - I'm not sure if I'm taking the right approach but here's a patch. The problem is that the existing search code doesn't pass a uid parameter to wiki_get_subwiki_by_group() so that call always fails with individual wikis. For some reason it's set to return if that fails, thus the blank screen (odd). My approach adds a subwiki parameter to the search form, since we already know what subwiki we're on when searching and as far as I can tell search only searches within a subwiki anyway. I can't see a reason not to do it this way but I may have missed something.
          Hide
          abgreeve Adrian Greeve added a comment -

          Hi Charles,

          Thanks for the fix. The code looks good. The only problem that I saw was in the extreme case where the user does a search on the wiki when there are no entries in the wiki. Obviously a subwiki ID hasn't been generated and an error is displayed. This may be more likely to happen when there are groups enabled, but as you pointed out, you require a subwiki ID to do a search.

          Ideally I think it would be nice to to see the search page say that it found no results rather than an error in that situation.

          Show
          abgreeve Adrian Greeve added a comment - Hi Charles, Thanks for the fix. The code looks good. The only problem that I saw was in the extreme case where the user does a search on the wiki when there are no entries in the wiki. Obviously a subwiki ID hasn't been generated and an error is displayed. This may be more likely to happen when there are groups enabled, but as you pointed out, you require a subwiki ID to do a search. Ideally I think it would be nice to to see the search page say that it found no results rather than an error in that situation.
          Hide
          cfulton Charles Fulton added a comment -

          Hi Adrian,

          Thanks for taking a look at this. I've dealt with those use cases, and added a new check to the search page which verifies that the user has the permission to view the subwiki they're searching.

          Show
          cfulton Charles Fulton added a comment - Hi Adrian, Thanks for taking a look at this. I've dealt with those use cases, and added a new check to the search page which verifies that the user has the permission to view the subwiki they're searching.
          Hide
          abgreeve Adrian Greeve added a comment -

          Hi Charles,

          Sorry about the wait for this peer review.

          After looking at this patch I have a couple of points.

          • mod/wiki/lib.php line 480: There is an extra " mark at the end which produces a quote mark between the search box and search button.
          • mod/wiki/pagelib.php line 108: When creating a page for the first you get the following notice:

          Notice: Trying to get property of non-object in /var/www/repositories/master/moodle/mod/wiki/pagelib.php on line 108

          $this->subwiki->id is blank at this stage and that is the reason for this notice.

          If you try to search the wiki from this page then you get a new notice:

          This page did not call $PAGE->set_url(...).

          One possible solution could be to do a check in pagelib.php, possibly not showing the search box at this stage. This would remove the need for the checks that you are doing in search.php.

          Show
          abgreeve Adrian Greeve added a comment - Hi Charles, Sorry about the wait for this peer review. After looking at this patch I have a couple of points. mod/wiki/lib.php line 480: There is an extra " mark at the end which produces a quote mark between the search box and search button. mod/wiki/pagelib.php line 108: When creating a page for the first you get the following notice: Notice: Trying to get property of non-object in /var/www/repositories/master/moodle/mod/wiki/pagelib.php on line 108 $this->subwiki->id is blank at this stage and that is the reason for this notice. If you try to search the wiki from this page then you get a new notice: This page did not call $PAGE->set_url(...). One possible solution could be to do a check in pagelib.php, possibly not showing the search box at this stage. This would remove the need for the checks that you are doing in search.php.
          Hide
          marina Marina Glancy added a comment -

          Hi Charles, do you plan to continue working on this issue? Thanks

          Show
          marina Marina Glancy added a comment - Hi Charles, do you plan to continue working on this issue? Thanks
          Hide
          cfulton Charles Fulton added a comment -

          Hi Marina, not in the near future.

          Show
          cfulton Charles Fulton added a comment - Hi Marina, not in the near future.
          Hide
          marina Marina Glancy added a comment -

          Thanks Charles, I'm posting here link to your branch for reference: https://github.com/mackensen/moodle/compare/master...MDL-33486-master

          Show
          marina Marina Glancy added a comment - Thanks Charles, I'm posting here link to your branch for reference: https://github.com/mackensen/moodle/compare/master...MDL-33486-master
          Hide
          marina Marina Glancy added a comment -

          Requesting peer review

          Show
          marina Marina Glancy added a comment - Requesting peer review
          Hide
          cibot CiBoT added a comment -

          Results for MDL-33486

          • Remote repository: git://github.com/marinaglancy/moodle.git
          Show
          cibot CiBoT added a comment - Results for MDL-33486 Remote repository: git://github.com/marinaglancy/moodle.git Remote branch wip- MDL-33486 -m25 to be integrated into upstream MOODLE_25_STABLE Executed job http://social.srv.in.moodle.com/job/Precheck%20remote%20branch/2497 Details: http://social.srv.in.moodle.com/job/Precheck%20remote%20branch/2497/artifact/work/smurf.html Remote branch wip- MDL-33486 -m26 to be integrated into upstream MOODLE_26_STABLE Executed job http://social.srv.in.moodle.com/job/Precheck%20remote%20branch/2498 Details: http://social.srv.in.moodle.com/job/Precheck%20remote%20branch/2498/artifact/work/smurf.html Remote branch wip- MDL-33486 -master to be integrated into upstream master Executed job http://social.srv.in.moodle.com/job/Precheck%20remote%20branch/2499 Details: http://social.srv.in.moodle.com/job/Precheck%20remote%20branch/2499/artifact/work/smurf.html
          Hide
          abgreeve Adrian Greeve added a comment -

          Thanks for picking this up Marina,

          Besides some simple stuff listed in the smurf file, everything else looks good. I particularly like the behat tests.

          Thanks again.

          Show
          abgreeve Adrian Greeve added a comment - Thanks for picking this up Marina, Besides some simple stuff listed in the smurf file, everything else looks good. I particularly like the behat tests. Thanks again.
          Hide
          marina Marina Glancy added a comment -

          Thanks Adrian for reviewing. It looks like codechecker does not know that lines are indented 4 spaces additionally after "case: " and moodlecheck does not know that phpdocs can be skipped if method overrides parent.
          The comments fullstop is unfortunately forgotten because I copied this code from view.php ...

          Show
          marina Marina Glancy added a comment - Thanks Adrian for reviewing. It looks like codechecker does not know that lines are indented 4 spaces additionally after "case: " and moodlecheck does not know that phpdocs can be skipped if method overrides parent. The comments fullstop is unfortunately forgotten because I copied this code from view.php ...
          Hide
          cibot CiBoT added a comment -

          Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!

          Show
          cibot CiBoT added a comment - Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!
          Hide
          poltawski Dan Poltawski added a comment -

          Integrated to master, 25 and 26 - thanks Marina!

          (I was getting weird issues with chrome behat on @mod_wiki, but they seemed unrelated to this issue, looked good on firefox).

          Show
          poltawski Dan Poltawski added a comment - Integrated to master, 25 and 26 - thanks Marina! (I was getting weird issues with chrome behat on @mod_wiki, but they seemed unrelated to this issue, looked good on firefox).
          Hide
          rajeshtaneja Rajesh Taneja added a comment -

          Thanks for fixing this Marina,

          Works well for me.

          Passing...

          Show
          rajeshtaneja Rajesh Taneja added a comment - Thanks for fixing this Marina, Works well for me. Passing...
          Hide
          poltawski Dan Poltawski added a comment -

          Thanks for your efforts, this change is now part of Moodle!

          Show
          poltawski Dan Poltawski added a comment - Thanks for your efforts, this change is now part of Moodle!

            People

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

              Dates

              • Created:
                Updated:
                Resolved:
                Fix Release Date:
                12/May/14