Uploaded image for project: '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
    • Status: Closed
    • Priority: 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

      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.

        Gliffy Diagrams

          Attachments

            Activity

            Hide
            quen 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
            quen 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
            quen 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
            quen 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
            timhunt 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
            timhunt 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
            quen 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
            quen 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
            quen 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
            quen 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:
                  Fix Release Date:
                  28/Jan/09