Moodle
  1. Moodle
  2. MDL-29848

Export entry to main glossary crashes

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 2.1.5, 2.2.2, 2.3
    • Fix Version/s: 2.1.6, 2.2.3
    • Component/s: Glossary
    • Labels:
    • Testing Instructions:
      Hide

      1. Create a main glossary (A) and a secondary glossary (B). Enable comments for both.
      2. Create a new entry in the secondary glossary.
      3. Export the entry to the main glossary.
      4. View the entry in both glossaries.

      Show
      1. Create a main glossary (A) and a secondary glossary (B). Enable comments for both. 2. Create a new entry in the secondary glossary. 3. Export the entry to the main glossary. 4. View the entry in both glossaries.
    • Affected Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE, MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-29848-master
    • Rank:
      19372

      Description

      See this discussion: http://moodle.org/mod/forum/discuss.php?d=185399
      Exporting an entry from a secondary glossary with Comments allowed on entries causes an error:
      Invalid context.

        Activity

        Hide
        Charles Fulton added a comment -

        Reproduced on the 2.2 dev beta I was using for Q&A. I'm not sure that the export part matters; I got the crash right after turning comments on and going back to the secondary glossary. Here's the stack trace:

        Stack trace:
        line 2842 of /mod/glossary/lib.php: comment_exception thrown
        line ? of unknownfile: call to glossary_comment_validate()
        line 7814 of /lib/moodlelib.php: call to call_user_func_array()
        line 852 of /comment/lib.php: call to plugin_callback()
        line 863 of /comment/lib.php: call to comment->validate()
        line 463 of /comment/lib.php: call to comment->can_view()
        line 1169 of /mod/glossary/lib.php: call to comment->output()
        line 1202 of /mod/glossary/lib.php: call to glossary_print_entry_icons()
        line 18 of /mod/glossary/formats/dictionary/dictionary_format.php: call to glossary_print_entry_lower_section()
        line 926 of /mod/glossary/lib.php: call to glossary_show_entry_dictionary()
        line 480 of /mod/glossary/view.php: call to glossary_print_entry()

        Show
        Charles Fulton added a comment - Reproduced on the 2.2 dev beta I was using for Q&A. I'm not sure that the export part matters; I got the crash right after turning comments on and going back to the secondary glossary. Here's the stack trace: Stack trace: line 2842 of /mod/glossary/lib.php: comment_exception thrown line ? of unknownfile: call to glossary_comment_validate() line 7814 of /lib/moodlelib.php: call to call_user_func_array() line 852 of /comment/lib.php: call to plugin_callback() line 863 of /comment/lib.php: call to comment->validate() line 463 of /comment/lib.php: call to comment->can_view() line 1169 of /mod/glossary/lib.php: call to comment->output() line 1202 of /mod/glossary/lib.php: call to glossary_print_entry_icons() line 18 of /mod/glossary/formats/dictionary/dictionary_format.php: call to glossary_print_entry_lower_section() line 926 of /mod/glossary/lib.php: call to glossary_show_entry_dictionary() line 480 of /mod/glossary/view.php: call to glossary_print_entry()
        Hide
        Charles Fulton added a comment -

        Okay, so I think I see what's going on. When you export an entry to the main glossary its glossaryid gets updated and the sourceglossaryid field is set so that the entry appears in both places. The comment logic always loads the context for the main id, so that you when you try to load an exported entry in the secondary glossary it will always fail the context check.

        I think the solution is to test whether there's a source id and if the calling module matches and to load that glossary instead for the context check if so. This does mean that there are two sets of comments (on both glossaries where the entry appears); I don't know if that's the desired behavior. This patch feels hackish but I'm not sure how to streamline it without local variables.

        Patch: https://github.com/mackensen/moodle/compare/master...MDL-29848.

        Show
        Charles Fulton added a comment - Okay, so I think I see what's going on. When you export an entry to the main glossary its glossaryid gets updated and the sourceglossaryid field is set so that the entry appears in both places. The comment logic always loads the context for the main id, so that you when you try to load an exported entry in the secondary glossary it will always fail the context check. I think the solution is to test whether there's a source id and if the calling module matches and to load that glossary instead for the context check if so. This does mean that there are two sets of comments (on both glossaries where the entry appears); I don't know if that's the desired behavior. This patch feels hackish but I'm not sure how to streamline it without local variables. Patch: https://github.com/mackensen/moodle/compare/master...MDL-29848 .
        Hide
        Jens Jahnke added a comment - - edited

        I can confirm that this patch works on current MOODLE_21_STABLE.

        Show
        Jens Jahnke added a comment - - edited I can confirm that this patch works on current MOODLE_21_STABLE.
        Hide
        Charles Fulton added a comment -

        Stealing this from Eloy and reviewing my older patch; hopefully no hard feelings .

        Show
        Charles Fulton added a comment - Stealing this from Eloy and reviewing my older patch; hopefully no hard feelings .
        Hide
        Sam Hemelryk added a comment -

        Thanks Charles, the changes look spot on so I'm putting this up for integration review now.

        Cheers
        Sam

        Show
        Sam Hemelryk added a comment - Thanks Charles, the changes look spot on so I'm putting this up for integration review now. Cheers Sam
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Some hours ago...

        the main moodle.git repository has 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.

        TIA and ciao

        Show
        Eloy Lafuente (stronk7) added a comment - Some hours ago... the main moodle.git repository has 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. TIA and ciao
        Hide
        Eloy Lafuente (stronk7) 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.

        TIA and ciao

        Show
        Eloy Lafuente (stronk7) 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. TIA and ciao
        Hide
        Aparup Banerjee added a comment -

        Thanks, this fix has been integrated into 21, 22 and master.
        All good to test.
        ps:I've fixed up a minor whitespace

        Show
        Aparup Banerjee added a comment - Thanks, this fix has been integrated into 21, 22 and master. All good to test. ps:I've fixed up a minor whitespace
        Hide
        Jason Fowler added a comment -

        All good Charles, thanks for the fix

        Show
        Jason Fowler added a comment - All good Charles, thanks for the fix
        Hide
        Sam Hemelryk added a comment -

        Congratulations are in order, you've made it, or at least your code has!
        It's now part of Moodle and both the git and cvs repositories have been updated.

        This issue is being marked as fixed and closed.

        Thank you.

        Show
        Sam Hemelryk added a comment - Congratulations are in order, you've made it, or at least your code has! It's now part of Moodle and both the git and cvs repositories have been updated. This issue is being marked as fixed and closed. Thank you.

          People

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

            Dates

            • Created:
              Updated:
              Resolved: