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

developer hint for missing or unreadable calendar hook file

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.1, 2.2, 2.3
    • Fix Version/s: 2.4
    • Component/s: Calendar
    • Labels:
    • Testing Instructions:
      Hide
      1. Create a calendar event
      2. Edit the calendar event
      3. Set $CFG-calendar to "moodle"
      4. Create a new calendar event
      5. Edit or Delete the calendar event
      6. Check the event is still created, edited or deleted and provides the developer hint "Calendar lib file missing or not readable at /calendar/moodle/lib.php."
      7. Explore the calendar module before and after setting the CFG variable to ensure there are no other regressions
      Show
      Create a calendar event Edit the calendar event Set $CFG-calendar to "moodle" Create a new calendar event Edit or Delete the calendar event Check the event is still created, edited or deleted and provides the developer hint "Calendar lib file missing or not readable at /calendar/moodle/lib.php." Explore the calendar module before and after setting the CFG variable to ensure there are no other regressions
    • Affected Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE, MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      wip-MDL-32180-master

      Description

      Provide a developer hint if $CFG->calendar is set in config.php but MOODLE_DIRROOT/calendar/{$CFG->calendar}/lib.php is missing or can't be read.

        Gliffy Diagrams

        1. lib.php
          1 kB
          Patrick Pollet

          Issue Links

            Activity

            Hide
            brianj Brian Jorgensen added a comment -

            I will submit a proposed fix for this.

            Show
            brianj Brian Jorgensen added a comment - I will submit a proposed fix for this.
            Hide
            salvetore Michael de Raadt added a comment -

            Hi, Brian.

            Please excuse my ignorance, but I'm not sure what setting $CFG->calendar is intended to achieve? I could not find any references to this.

            Show
            salvetore Michael de Raadt added a comment - Hi, Brian. Please excuse my ignorance, but I'm not sure what setting $CFG->calendar is intended to achieve? I could not find any references to this.
            Hide
            brianj Brian Jorgensen added a comment -

            Hi, Michael:

            It is a calendar-specific variation of an event hook system; I don't know the history of it, I'm just using what is there.

            Basically, by assigning a value to $CFG->calendar, this tells the calendar lib file to look for a file at /calendar/{$CFG->calendar}/lib.php for methods called {$CFG->calendar}_add_event, {$CFG->calendar}_update_event and {$CFG->calendar}_delete_event. These methods are then called after the similar calendar methods.

            http://docs.moodle.org/dev/Calendar_API#Event_hook

            Hope this helps,

            Brian

            Show
            brianj Brian Jorgensen added a comment - Hi, Michael: It is a calendar-specific variation of an event hook system; I don't know the history of it, I'm just using what is there. Basically, by assigning a value to $CFG->calendar, this tells the calendar lib file to look for a file at /calendar/{$CFG->calendar}/lib.php for methods called {$CFG->calendar}_add_event, {$CFG->calendar}_update_event and {$CFG->calendar}_delete_event. These methods are then called after the similar calendar methods. http://docs.moodle.org/dev/Calendar_API#Event_hook Hope this helps, Brian
            Hide
            salvetore Michael de Raadt added a comment -

            Hi, Brian.

            Thanks for directing me to that. You learn something new every day.

            If you can provide a patch for that potential missing code, it would be welcomed.

            Show
            salvetore Michael de Raadt added a comment - Hi, Brian. Thanks for directing me to that. You learn something new every day. If you can provide a patch for that potential missing code, it would be welcomed.
            Hide
            brianj Brian Jorgensen added a comment - - edited

            Here is a fix for 2.1.x. In order to test, you must first set $CFG->calendar in config.php. Then, there are three fairly easy tests that should return the debug message to screen:

            1. do not create the folder MOODLE_DIRROOT/calendar/{$CFG->calendar};
            2. create the folder MOODLE_DIRROOT/calendar/{$CFG->calendar}/ but do not add the lib.php file in that folder;
            3. create the file MOODLE_DIRROOT/calendar/{$CFG->calendar}/lib.php but make sure it is not readable by the web server.

            Show
            brianj Brian Jorgensen added a comment - - edited Here is a fix for 2.1.x. In order to test, you must first set $CFG->calendar in config.php. Then, there are three fairly easy tests that should return the debug message to screen: 1. do not create the folder MOODLE_DIRROOT/calendar/{$CFG->calendar}; 2. create the folder MOODLE_DIRROOT/calendar/{$CFG->calendar}/ but do not add the lib.php file in that folder; 3. create the file MOODLE_DIRROOT/calendar/{$CFG->calendar}/lib.php but make sure it is not readable by the web server.
            Hide
            ppollet Patrick Pollet added a comment - - edited

            I would personnally suggest that the Calendar API complies with the 'standard Moodle events API' by triggering events (via events_trigger API call) instead of a specific hook. (see http://moodle.org/mod/forum/discuss.php?d=201744&parent=880325). This will make developper's life easier by handling these events the standard way in a local plugin (see http://docs.moodle.org/dev/Events_API) .

            I also noticed that when a calendar event is deleted, the argument to the function xxxxxx_delete_event is only the id of the calendar event that has already been deleted from the database, so it cannot be retrieved by the handler for additional processing. It would be nicer to send the database record that has just been deleted.

            Cheers.

            Show
            ppollet Patrick Pollet added a comment - - edited I would personnally suggest that the Calendar API complies with the 'standard Moodle events API' by triggering events (via events_trigger API call) instead of a specific hook. (see http://moodle.org/mod/forum/discuss.php?d=201744&parent=880325 ). This will make developper's life easier by handling these events the standard way in a local plugin (see http://docs.moodle.org/dev/Events_API ) . I also noticed that when a calendar event is deleted, the argument to the function xxxxxx_delete_event is only the id of the calendar event that has already been deleted from the database, so it cannot be retrieved by the handler for additional processing. It would be nicer to send the database record that has just been deleted. Cheers.
            Hide
            ppollet Patrick Pollet added a comment - - edited

            attached a sample code of a lib.php function that's trap all calls to the calendar's events hook and dumps their arguments to a log file.
            to use it create a directory in calendar/ named pphooks, copy the lib.php into it and set CFG->calendar='pphooks' in config.php.

            Show
            ppollet Patrick Pollet added a comment - - edited attached a sample code of a lib.php function that's trap all calls to the calendar's events hook and dumps their arguments to a log file. to use it create a directory in calendar/ named pphooks, copy the lib.php into it and set CFG->calendar='pphooks' in config.php.
            Hide
            brianj Brian Jorgensen added a comment -

            OK, I have moved the commit onto master, MOODLE_22_STABLE and MOODLE_21_STABLE to make it easier to integrate.

            Show
            brianj Brian Jorgensen added a comment - OK, I have moved the commit onto master, MOODLE_22_STABLE and MOODLE_21_STABLE to make it easier to integrate.
            Hide
            phalacee Jason Fowler added a comment -

            cherry-picked the change to the current master (this was an older patch) - up for peer-review now

            Show
            phalacee Jason Fowler added a comment - cherry-picked the change to the current master (this was an older patch) - up for peer-review now
            Hide
            rajeshtaneja Rajesh Taneja added a comment - - edited

            Patch looks good Jason,

            Not sure about the debug string, DEVELOPER HINT: is probably not required.

            Feel free to push it for integration if you feel otherwise.

            FYI: Please add test instructions and probably cheery-pick it for 23 and 22

            Show
            rajeshtaneja Rajesh Taneja added a comment - - edited Patch looks good Jason, Not sure about the debug string, DEVELOPER HINT: is probably not required. Feel free to push it for integration if you feel otherwise. FYI: Please add test instructions and probably cheery-pick it for 23 and 22
            Hide
            phalacee Jason Fowler added a comment -

            Developer Hint text remove, back ported, and test instructions added.

            According to Tim this is likely to end up being part of an exploratory test session.

            Show
            phalacee Jason Fowler added a comment - Developer Hint text remove, back ported, and test instructions added. According to Tim this is likely to end up being part of an exploratory test session.
            Hide
            poltawski Dan Poltawski 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
            poltawski Dan Poltawski 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
            phalacee Jason Fowler added a comment -

            rebased

            Show
            phalacee Jason Fowler added a comment - rebased
            Hide
            poltawski Dan Poltawski added a comment -

            Sorry, but this is not quite right.

            You are missing the (usual) case where $CFG->calendar is not set. This change will change the behaviour such that $extcalendarinc is not set to false and so it'll go ahead and try and call the hook anyway (and it won't cache the result).

            This is demonstrated if you try the testing instructions without setting $CFG->calendar.

            Notice: Undefined property: stdClass::$calendar in /Users/danp/git/integration/calendar/lib.php on line 2448

            Please make sure that case is covered and also is present in the testing instructions.

            Show
            poltawski Dan Poltawski added a comment - Sorry, but this is not quite right. You are missing the (usual) case where $CFG->calendar is not set. This change will change the behaviour such that $extcalendarinc is not set to false and so it'll go ahead and try and call the hook anyway (and it won't cache the result). This is demonstrated if you try the testing instructions without setting $CFG->calendar. Notice: Undefined property: stdClass::$calendar in /Users/danp/git/integration/calendar/lib.php on line 2448 Please make sure that case is covered and also is present in the testing instructions.
            Hide
            phalacee Jason Fowler added a comment -

            Thanks for spotting that Dan, I didn't even think of that.

            Show
            phalacee Jason Fowler added a comment - Thanks for spotting that Dan, I didn't even think of that.
            Hide
            phalacee Jason Fowler added a comment -

            Changes made and pushed for integration again

            Show
            phalacee Jason Fowler added a comment - Changes made and pushed for integration again
            Hide
            poltawski Dan Poltawski added a comment -

            Integrated, thanks

            Show
            poltawski Dan Poltawski added a comment - Integrated, thanks
            Hide
            dmonllao David Monllaó added a comment -

            It passes, tested in master

            Show
            dmonllao David Monllaó added a comment - It passes, tested in master
            Hide
            poltawski Dan Poltawski added a comment -

            Congratulations, you've done it!

            Nf n erjneq sbe fhpprfshy vagrtengvba vagb guvf jrrxf eryrnfr, V pna abj qvfpybfr gb lbh gur rkvfgnapr bs shapgvba fge_ebg13(), gb tb va lbhe gbbyxvg nybat jvgu uggc://cuc.arg/znahny/ra/shapgvba.tmtrgff.cuc

            Cyrnfr qb abg nyybj guvf vasbezngvba gb cnff shegure.

            Show
            poltawski Dan Poltawski added a comment - Congratulations, you've done it! Nf n erjneq sbe fhpprfshy vagrtengvba vagb guvf jrrxf eryrnfr, V pna abj qvfpybfr gb lbh gur rkvfgnapr bs shapgvba fge_ebg13(), gb tb va lbhe gbbyxvg nybat jvgu uggc://cuc.arg/znahny/ra/shapgvba.tmtrgff.cuc Cyrnfr qb abg nyybj guvf vasbezngvba gb cnff shegure.

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  3/Dec/12