Moodle
  1. Moodle
  2. MDL-32180

developer hint for missing or unreadable calendar hook file

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor 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
    • Rank:
      38928

      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.

      1. lib.php
        1 kB
        Patrick Pollet

        Issue Links

          Activity

          Hide
          Brian Jorgensen added a comment -

          I will submit a proposed fix for this.

          Show
          Brian Jorgensen added a comment - I will submit a proposed fix for this.
          Hide
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          Jason Fowler added a comment -

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

          Show
          Jason Fowler added a comment - cherry-picked the change to the current master (this was an older patch) - up for peer-review now
          Hide
          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
          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
          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
          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
          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
          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
          Jason Fowler added a comment -

          rebased

          Show
          Jason Fowler added a comment - rebased
          Hide
          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
          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
          Jason Fowler added a comment -

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

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

          Changes made and pushed for integration again

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

          Integrated, thanks

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

          It passes, tested in master

          Show
          David Monllaó added a comment - It passes, tested in master
          Hide
          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
          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: