Moodle
  1. Moodle
  2. MDL-17164

Glossary entry display does not include autolinking for glossary entries that are similar to the current entry

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 1.9.3
    • Fix Version/s: 1.9.4
    • Component/s: Glossary
    • Labels:
      None
    • Affected Branches:
      MOODLE_19_STABLE
    • Fixed Branches:
      MOODLE_19_STABLE
    • Rank:
      30274

      Description

      To reproduce:

      0) make sure glossary autolink filter is enabled
      1) create a new main glossary
      2) add two entries:

      frog
      comes from frogspawn

      keywords: frog
      autolink, whole words only

      frogspawn
      grows into a frog

      keywords: frogspawn
      autolink, whole words only

      3) look at all entries

      you will notice that in the 'frogspawn' entry, the word 'frog' is autolinked. In the 'frog' entry, however, the word 'frogspawn' is not autolinked. It appears there is code to prevent entries linking to themselves, but this seems to be a bit too eager! It is preventing links to any glossary entry that contains the current one, as well as to just the current one.

      In other places (eg I made another entry 'zombie' 'nothing to do with either frog or frogspawn') both words link as expected.

        Activity

        Hide
        Sam Marshall added a comment -

        to note, I made a silly example, but this same problem did show up (3 times!) in a genuine glossary relating to a science course.

        Show
        Sam Marshall added a comment - to note, I made a silly example, but this same problem did show up (3 times!) in a genuine glossary relating to a science course.
        Hide
        Sam Marshall added a comment -

        This is a patch which:

        removes the current (unbelievably complicated and doesn't work properly as per this bug) method of stopping glossary entries including links to themselves.

        adds a new method using a simple $GLOSSARY_EXCLUDECONCEPTS global - yes it's a global but it makes life infinity times easier, as you can see by the number of lines removed vs added in this patch...

        only files changed are within glossary (glossary/lib.php and glossary/filter.php)

        could I please get a +1 for commit to 1.9.4 (will port to 2.x at that stage assuming code is still relevant)?

        Show
        Sam Marshall added a comment - This is a patch which: removes the current (unbelievably complicated and doesn't work properly as per this bug) method of stopping glossary entries including links to themselves. adds a new method using a simple $GLOSSARY_EXCLUDECONCEPTS global - yes it's a global but it makes life infinity times easier, as you can see by the number of lines removed vs added in this patch... only files changed are within glossary (glossary/lib.php and glossary/filter.php) could I please get a +1 for commit to 1.9.4 (will port to 2.x at that stage assuming code is still relevant)?
        Hide
        Tim Hunt added a comment -

        I don't know enough about the glossary to spot any subtle problems in your new code (some unit tests would be nice, if the API of this function allows it). However, I like how much shorter it is.

        Just one things,

        foreach($concept->phrase as $concept) {
        if(!in_array($concept->phrase,$GLOSSARY_EXCLUDECONCEPTS))

        { $reducedconceptlist[]=$concept; }

        }

        That in_array inside a loop sucks from a performance point of view. If you make it

        $GLOSSARY_EXCLUDECONCEPTS=array($entry->concept => 1);

        that is, put the value in the array key, the if becomes

        if (!isset($GLOSSARY_EXCLUDECONCEPTS[$concept->phrase])) {

        Actually, the best thing would be a bigger rewrite to put the values in $conceptlist into the array keys, then leave $GLOSSARY_EXCLUDECONCEPTS alone, and that bit of code becomes

        $reducedconceptlist = $conceptlist;
        foreach($GLOSSARY_EXCLUDECONCEPTS as $concept) {
        if (isset($GLOSSARY_EXCLUDECONCEPTS[$concept]))

        { unset($reducedconceptlist[$concept]); }

        }

        and probably other bits of code becomes simpler too. However, you probably don't want to make that big a change.

        Show
        Tim Hunt added a comment - I don't know enough about the glossary to spot any subtle problems in your new code (some unit tests would be nice, if the API of this function allows it). However, I like how much shorter it is. Just one things, foreach($concept->phrase as $concept) { if(!in_array($concept->phrase,$GLOSSARY_EXCLUDECONCEPTS)) { $reducedconceptlist[]=$concept; } } That in_array inside a loop sucks from a performance point of view. If you make it $GLOSSARY_EXCLUDECONCEPTS=array($entry->concept => 1); that is, put the value in the array key, the if becomes if (!isset($GLOSSARY_EXCLUDECONCEPTS [$concept->phrase] )) { Actually, the best thing would be a bigger rewrite to put the values in $conceptlist into the array keys, then leave $GLOSSARY_EXCLUDECONCEPTS alone, and that bit of code becomes $reducedconceptlist = $conceptlist; foreach($GLOSSARY_EXCLUDECONCEPTS as $concept) { if (isset($GLOSSARY_EXCLUDECONCEPTS [$concept] )) { unset($reducedconceptlist[$concept]); } } and probably other bits of code becomes simpler too. However, you probably don't want to make that big a change.
        Hide
        Sam Marshall added a comment -

        I considered the first suggestion but decided the code was clearer this way (normal arrays are easier to understand/document than item=>true arrays). $GLOSSARY_EXCLUDECONCEPTS is unlikely to have more than approx 3 items in so it probably doesn't matter.

        btw I think array_key_exists is maybe faster than isset...

        However can change this if you like. (You're right, I don't want to make the larger change suggested, but the small performance one, can do.)

        Hoping to get this in today so I can move onto other things so will pester somebody else to give me ok to commit it

        Show
        Sam Marshall added a comment - I considered the first suggestion but decided the code was clearer this way (normal arrays are easier to understand/document than item=>true arrays). $GLOSSARY_EXCLUDECONCEPTS is unlikely to have more than approx 3 items in so it probably doesn't matter. btw I think array_key_exists is maybe faster than isset... However can change this if you like. (You're right, I don't want to make the larger change suggested, but the small performance one, can do.) Hoping to get this in today so I can move onto other things so will pester somebody else to give me ok to commit it
        Hide
        Sam Marshall added a comment -

        Since nobody else commented I checked it in and merged to 2.0. If glossary filters are suddenly broken, consider blaming me

        mod/glossary/filter.php
        mod/glossary/lib.php

        HEAD, MOODLE_19+MERGED

        Show
        Sam Marshall added a comment - Since nobody else commented I checked it in and merged to 2.0. If glossary filters are suddenly broken, consider blaming me mod/glossary/filter.php mod/glossary/lib.php HEAD, MOODLE_19+MERGED

          People

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

            Dates

            • Created:
              Updated:
              Resolved: