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

The activity name auto-linking filter should not create links to webpages it was created on.

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 1.9.16, 2.0.2, 2.1, 2.2.1
    • Fix Version/s: 2.5
    • Component/s: Filters
    • Labels:
    • Database:
      Any
    • Testing Instructions:
      Hide
      1. Create a page with the name 'Introduction'
      2. Fill the content of the page with something like 'This introduction is ...', check "display description on the course page"
      3. Enable the activity name auto-linking (enabled by default)
      4. Make sure that the word introduction is not changed into a link both on course page and on module page
      Show
      Create a page with the name 'Introduction' Fill the content of the page with something like 'This introduction is ...', check "display description on the course page" Enable the activity name auto-linking (enabled by default) Make sure that the word introduction is not changed into a link both on course page and on module page
    • Workaround:
      Hide

      filter/activitynames/filter.php

       
      - if ($activity->mod != "label" and $activity->visible and empty($activity->groupmembersonly)) {
       
      + if ($activity->mod != "label" and 
      +    $activity->cm != $this->context->instanceid and
      +    $activity->visible and 
      +    empty($activity->groupmembersonly)) {

      Show
      filter/activitynames/filter.php   - if ($activity->mod != "label" and $activity->visible and empty($activity->groupmembersonly)) {   + if ($activity->mod != "label" and + $activity->cm != $this->context->instanceid and + $activity->visible and + empty($activity->groupmembersonly)) {
    • Affected Branches:
      MOODLE_19_STABLE, MOODLE_20_STABLE, MOODLE_21_STABLE, MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE
    • Pull Master Branch:
      wip-MDL-31885-smartcache

      Description

      A link on a webpage that refers to the webpage itself, provide no extra information at all. Links should refer to anchors on the same page, or other pages on the site or internet.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            salvetore Michael de Raadt added a comment -

            Thanks for suggesting that and providing a fix.

            Show
            salvetore Michael de Raadt added a comment - Thanks for suggesting that and providing a fix.
            Hide
            marina Marina Glancy added a comment -

            taking this issue, I was working on the same line in MDL-33774

            Show
            marina Marina Glancy added a comment - taking this issue, I was working on the same line in MDL-33774
            Hide
            marina Marina Glancy added a comment -

            I added a check for contextlevel to submitted patch.
            Also I added second commit removing the cache inside filter_activitynames because caching prevented different logic for different modules

            Note that master branch is based on MDL-33774

            Show
            marina Marina Glancy added a comment - I added a check for contextlevel to submitted patch. Also I added second commit removing the cache inside filter_activitynames because caching prevented different logic for different modules Note that master branch is based on MDL-33774
            Hide
            damyon Damyon Wiese added a comment -

            The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

            Thanks!

            Show
            damyon Damyon Wiese added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. Thanks!
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Side note: be careful when chaining branches, for some reason MDL-33774 final commits have changed (rebase or whatever) since MDL-31885 was created and right now they are different and the compare does not take rid of them. No big problem, just fyi.

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Side note: be careful when chaining branches, for some reason MDL-33774 final commits have changed (rebase or whatever) since MDL-31885 was created and right now they are different and the compare does not take rid of them. No big problem, just fyi.
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Sorry, I'm reopening this:

            1) While I understand the reason to stop using current static cache in the filter (courseid-based), not usable anymore coz we are introducing cm-based variations, I think the filter, run without any cache can have a negative impact. Just imagine one big discussion, for example with, say, 1000 texts to process (more if filtering of headings is enabled). Without any caching, the system will have to perform 1000 fullclones, sortings and iterations, when all them are going to run for the same cm (a forum one).

            So I'd suggest to, at least, implement a cm-based caching (static or MUC, whatever) for $activitylist. And, ideally I'd also implement a course-based caching (for $sortedactivities), but surely it's harder to get 1000 texts to filter there, although not impossible.

            2) Given that you're moving the whole code, surely it would be a good moment to apply the coding-style rules to it (inline comments, no-space on function calls...)

            Ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Sorry, I'm reopening this: 1) While I understand the reason to stop using current static cache in the filter (courseid-based), not usable anymore coz we are introducing cm-based variations, I think the filter, run without any cache can have a negative impact. Just imagine one big discussion, for example with, say, 1000 texts to process (more if filtering of headings is enabled). Without any caching, the system will have to perform 1000 fullclones, sortings and iterations, when all them are going to run for the same cm (a forum one). So I'd suggest to, at least, implement a cm-based caching (static or MUC, whatever) for $activitylist. And, ideally I'd also implement a course-based caching (for $sortedactivities), but surely it's harder to get 1000 texts to filter there, although not impossible. 2) Given that you're moving the whole code, surely it would be a good moment to apply the coding-style rules to it (inline comments, no-space on function calls...) Ciao
            Hide
            marina Marina Glancy added a comment -

            Hi Eloy,

            0) sorry, did not do rebasing right. Corrected now

            1) I removed the caching also because it is excessive.
            First, filter_activitynames should not be applied to strings because strings do not have tags (and links) anyway.
            Second, there is caching in the filterlib already, the filtered text is cached for every string.
            Third, there are no database requests in this function, so we don't really save much by caching. modinfo is cached already and all we were caching was the re-arranged activities list from modinfo.
            Forth, the only situation where the cache that I removed was used is: course view page that has a lot of texts (such as labels and activities with shown description). Hehe, saying that - before my labels issue MDL-33774 labels were not filtered at all...

            2) sure

            Anyway, when I was writing all that I had an idea of per-course caching. Will post soon

            Show
            marina Marina Glancy added a comment - Hi Eloy, 0) sorry, did not do rebasing right. Corrected now 1) I removed the caching also because it is excessive. First, filter_activitynames should not be applied to strings because strings do not have tags (and links) anyway. Second, there is caching in the filterlib already, the filtered text is cached for every string. Third, there are no database requests in this function, so we don't really save much by caching. modinfo is cached already and all we were caching was the re-arranged activities list from modinfo. Forth, the only situation where the cache that I removed was used is: course view page that has a lot of texts (such as labels and activities with shown description). Hehe, saying that - before my labels issue MDL-33774 labels were not filtered at all... 2) sure Anyway, when I was writing all that I had an idea of per-course caching. Will post soon
            Hide
            marina Marina Glancy added a comment -

            I created a new branch where I don't remove the static cache but just adjust the course list for the module

            But I can not change it to MUC because of some MUC error, so I created a new issue MDL-38111 and already submitted a patch (with all code refactoring like you wanted). But it is waiting on Sam

            Show
            marina Marina Glancy added a comment - I created a new branch where I don't remove the static cache but just adjust the course list for the module But I can not change it to MUC because of some MUC error, so I created a new issue MDL-38111 and already submitted a patch (with all code refactoring like you wanted). But it is waiting on Sam
            Hide
            cibot CiBoT added a comment -

            Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

            Show
            cibot CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
            Hide
            damyon Damyon Wiese added a comment -

            The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

            Thanks!

            Show
            damyon Damyon Wiese added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. Thanks!
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Aha, I've already integrated MDL-38110, per Sam's request. Will look to this and its followup (MDL-38111) tomorrow.

            Thanks!

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Aha, I've already integrated MDL-38110 , per Sam's request. Will look to this and its followup ( MDL-38111 ) tomorrow. Thanks!
            Hide
            marina Marina Glancy added a comment -

            Eloy, MDL-38111 is not pushed for integration yet. Actually it is not finished yet because cache does not have a proper definition.

            Show
            marina Marina Glancy added a comment - Eloy, MDL-38111 is not pushed for integration yet. Actually it is not finished yet because cache does not have a proper definition.
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Clever switch to activity based cache, integrated, thanks!

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Clever switch to activity based cache, integrated, thanks!
            Hide
            fred Frédéric Massart added a comment -

            Works as described. Thanks!

            Show
            fred Frédéric Massart added a comment - Works as described. Thanks!
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Because

            A
            MARVELOUS
            A       U
            Z  YOU  P
            I  ARE  E
            N  PPL  R
            G       B
              TNKS! 
            

            Closing, ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Because A MARVELOUS A U Z YOU P I ARE E N PPL R G B TNKS! Closing, ciao

              People

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

                Dates

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