Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-64729

glossary_get_entries_search does not respect case sensitivity

    XMLWordPrintable

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 3.6.2
    • Fix Version/s: 3.5.5, 3.6.3
    • Component/s: Glossary
    • Labels:
    • Testing Instructions:
      Hide
      1. Go to a course.
      2. Make note of the course id.
        • You can find the course id in the address bar. The address will look like course/view.php?id=2 Where the 2 at the end is the ID.
      3. Create a glossary activity.
        • Leave all the settings as default.
      4. Add a new entry.
        1. Set the concept to start with a capital letter (such as "Fish").
        2. Go to the Auto-linking section.
        3. Check first "This entry should be automatically linked" and then "This entry is case sensitive"
        4. Save changes.
      5. Add a second entry.
        1. Set the concept to start with a capital letter (such as "Chips").
        2. Go to the Auto-linking section.
        3. Only check "This entry should be automatically linked"
        4. Save changes.
      6. Change the URL to the following address: [yourmoodleaddress]/mod/glossary/showentry.php?courseid=2&concept=fish (Change the 2 to the course ID that you made note of from step 2) and press enter to go to this page. See the image below for an example of the URL.
      7. Change the word "fish" to the concept name from the first entry. This entry is case sensitive, so "fish" will show nothing while "Fish" will show the entry. Check this is true.
      8. Now change the concept to the second entry.
      9. Check that both upper and lower case words work to show the entry.

      Show
      Go to a course. Make note of the course id. You can find the course id in the address bar. The address will look like course/view.php?id=2 Where the 2 at the end is the ID. Create a glossary activity. Leave all the settings as default. Add a new entry. Set the concept to start with a capital letter (such as "Fish"). Go to the Auto-linking section. Check first "This entry should be automatically linked" and then "This entry is case sensitive" Save changes. Add a second entry. Set the concept to start with a capital letter (such as "Chips"). Go to the Auto-linking section. Only check "This entry should be automatically linked" Save changes. Change the URL to the following address: [yourmoodleaddress] /mod/glossary/showentry.php?courseid=2&concept=fish (Change the 2 to the course ID that you made note of from step 2) and press enter to go to this page. See the image below for an example of the URL. Change the word "fish" to the concept name from the first entry. This entry is case sensitive, so "fish" will show nothing while "Fish" will show the entry. Check this is true. Now change the concept to the second entry. Check that both upper and lower case words work to show the entry.
    • Affected Branches:
      MOODLE_36_STABLE
    • Fixed Branches:
      MOODLE_35_STABLE, MOODLE_36_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      wip-MDL-64729-master

      Description

      The glossary lib function glossary_get_entries_search does not respect the casesensitive setting correctly.

      I found this bug when trying to use mod/glossary/showentry_ajax with just a $concept rather than the usual $eid in some custom code. Basically I wanted to get the definition from the concept rather than its id.

      The library function glossary_get_entries_search is only used in two places showentry.php and showentry_ajax.php. In both of these cases the function is probably never called as $eid is normally provided.

      To reproduce this bug:
      Create a course, ensure the glossary filter is on for the course.
      Add a course main glossary.
      Add a concept e.g. Aardvark (note the concept has an initial capital letter), with definition 'A medium-sized, burrowing, nocturnal mammal native to Africa' or similar, ensure concept is automatically linked (not default) and is not marked as case sensitive (default).
      Add a label to the course that has some text including the word aardvark in it.
      Check the label now has a link around the glossary concept and that it works (popup).
      Edit the glossary term Aardvark, making it case sensitive.
      Check the label now does not have a link around the glossary concept.
      That proves showentry/showentry_ajax are working correctly when calling glossary_get_entries_search with an $eid.
      So to prove the bug try this to call the function with a $concept instead.
      Create a new test script (called mod/glossary/a.php):

      <?php
       require('../../config.php');
       require('lib.php');
       echo 'Start<br/>';
       $concept = 'Aardvark';
       $courseid = <course id created above>;
       echo $concept.'<br/>';
       $a = glossary_get_entries_search($concept, $courseid);
       print_object($a); 

      Now changing the case sensitivity and the case of the $concept shows that the function glossary_get_entries_search does not respect the casesensitive setting correctly.
      Alternatively the bug can be proven by adding this phpunit test to the end of mod/glossary/tests/lib_test.php 

       

       

          public function test_glossary_get_entries_search() {
              $this->resetAfterTest();
              $this->setAdminUser();
              // Turn on glossary autolinking (usedynalink).
              set_config('glossary_linkentries', 1);
              $glossarygenerator = $this->getDataGenerator()->get_plugin_generator('mod_glossary');
              $course = $this->getDataGenerator()->create_course();
              $glossary = $this->getDataGenerator()->create_module('glossary', array('course' => $course->id));
              // Note this entry is not case sensitive by default (casesensitive = 0).
              $entry = $glossarygenerator->create_content($glossary);
              // Check that a search for the concept return the entry.
              $concept = $entry->concept;
              $search = glossary_get_entries_search($concept, $course->id);
              $this->assertCount(1, $search);
              $foundentry = array_shift($search);
              $this->assertEquals($foundentry->concept, $entry->concept);
              // Now try the same search but with a lowercase term.
              $concept = strtolower($entry->concept);
              $search = glossary_get_entries_search($concept, $course->id);
              $this->assertCount(1, $search);
              $foundentry = array_shift($search);
              $this->assertEquals($foundentry->concept, $entry->concept);
              // Make an entry that is case sensitive (casesensitive = 1).
              set_config('glossary_casesensitive', 1);
              $entry = $glossarygenerator->create_content($glossary);
              $concept = $entry->concept;
              $search = glossary_get_entries_search($concept, $course->id);
              $this->assertCount(1, $search);
              $foundentry = array_shift($search);
              $this->assertEquals($foundentry->concept, $entry->concept);
              // Now try the same search but with a lowercase term.
              $concept = strtolower($entry->concept);
              $search = glossary_get_entries_search($concept, $course->id);
              $this->assertCount(0, $search);
          }
      

       

       

      Of course this test fails, however the fix for this is simple:

      mod/glossary/lib.php line 1037

                                               ( (e.casesensitive != 1 AND LOWER(concept) = :conceptlower) OR
                                                (e.casesensitive = 1 and concept = :concept)) AND

      When applied the unit test passes, as does the visual check above.

      However the question is whether this is valuable to fix at all. It is a 'bug' that has existed since 2005, and clearly has not affected anyone. So maybe it might be an idea to remove the function, and the two places it is called in showentry and showentry_ajax?

      However fixing the function as above is arguably quicker and easier than removing it (I will add a fix on github shortly), and it then would allow other developers to use the function with a concept if they found a use in the future.

      To fix or to remove? Any thoughts/opinions?

        Attachments

          Activity

            People

            • Assignee:
              jb23347 John Beedell
              Reporter:
              jb23347 John Beedell
              Peer reviewer:
              Tim Hunt
              Integrator:
              Adrian Greeve
              Tester:
              Adrian Greeve
              Participants:
              Component watchers:
              Adrian Greeve, Jake Dallimore, Mathew May, Mihail Geshoski, Peter Dias
            • Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:
                Fix Release Date:
                11/Mar/19