Moodle
  1. Moodle
  2. MDL-27192

User profile fields drop-down (menu) causes errors on pages with no context using get_complete_user_data() e.g. calendar export

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.0.2
    • Fix Version/s: 2.0.4
    • Component/s: Calendar
    • Environment:
      All
    • Rank:
      16825

      Description

      If you create a new user profile field that is a menu type (drop-down select menu) then calendar exporting starts throwing an error message.

      This is because calendar/export_execute.php uses the get_complete_user_data function; which is getting the information for any user profile fields configured in the installation.

      Within the 'menu' user profile field there is a call to format_string() - format_string errors when called without a valid page context.

      Error:

      Coding problem: this page does not set $PAGE->context properly.
      line 341 of /lib/pagelib.php: call to debugging()
      line 599 of /lib/pagelib.php: call to moodle_page->magic_get_context()
      line 1236 of /lib/weblib.php: call to moodle_page->__get()
      line 23 of /user/profile/field/menu/field.class.php: call to format_string()
      line 496 of /user/profile/lib.php: call to profile_field_menu->profile_field_menu()
      line 517 of /user/profile/lib.php: call to profile_user_record()
      line 3820 of /lib/moodlelib.php: call to profile_load_custom_fields()
      line 16 of /calendar/export_execute.php: call to get_complete_user_data()

      Does calendar need to use get_complete_user_data function?
      A get_record call to user table might suffice instead and would stop this error.

      I haven't seen any evidence of this issue occurring elsewhere where get_complete_user_data() is called (e.g. rss/file.php); but of course, there may be other areas affected.

        Issue Links

          Activity

          Hide
          Jenny Gray added a comment -

          Steps to reproduce:

          1 clean install Moodle 2.0 stable branch
          2 create new custom user profile field
          a menu of choices
          b any data doesnt matter
          3 go to calendar
          4 press ical button

          Show
          Jenny Gray added a comment - Steps to reproduce: 1 clean install Moodle 2.0 stable branch 2 create new custom user profile field a menu of choices b any data doesnt matter 3 go to calendar 4 press ical button
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Hi Jenny,

          I don't think setting the page context is the best way here because:

          1) That script is really wrong, ignoring completely mnet stuff. We should change the "username" param by "userid" ASAP (while keeping BC, of course) to avoid the mnet problem. I'd create a separate issue for this.
          2) I can imagine the context for that page changing in the future, being invoked with other params to get other information, by course or whatever (more selective), so I don't think to "impose" the CONTEXT_SYTEM is a good solution.

          So I'd propose to fix this, one of these (alternatively):

          1) Avoid using the get_complete_user_data() call, I've gone along all the '$user' uses and it seems that only a few fields are required (username, password and id).
          2) Modify the profile field, so the offending format_string() call includes one proper $options['context'] = CONTEXT_SYTEM option.

          I think 1) is better than 2) as far as it both solves this issue and make things quick. Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Hi Jenny, I don't think setting the page context is the best way here because: 1) That script is really wrong, ignoring completely mnet stuff. We should change the "username" param by "userid" ASAP (while keeping BC, of course) to avoid the mnet problem. I'd create a separate issue for this. 2) I can imagine the context for that page changing in the future, being invoked with other params to get other information, by course or whatever (more selective), so I don't think to "impose" the CONTEXT_SYTEM is a good solution. So I'd propose to fix this, one of these (alternatively): 1) Avoid using the get_complete_user_data() call, I've gone along all the '$user' uses and it seems that only a few fields are required (username, password and id). 2) Modify the profile field, so the offending format_string() call includes one proper $options ['context'] = CONTEXT_SYTEM option. I think 1) is better than 2) as far as it both solves this issue and make things quick. Ciao
          Hide
          Eloy Lafuente (stronk7) added a comment -

          (closing for reopening... blame the new workflow)

          Show
          Eloy Lafuente (stronk7) added a comment - (closing for reopening... blame the new workflow)
          Hide
          Eloy Lafuente (stronk7) added a comment -

          reopened for your consideration. Feel free to submit fix as far as this continues being part of this week integration.

          TIA and ciao

          Show
          Eloy Lafuente (stronk7) added a comment - reopened for your consideration. Feel free to submit fix as far as this continues being part of this week integration. TIA and ciao
          Hide
          Jenny Gray added a comment -

          I did this work as a demo to train Jason Platts (one of the other lead developers here at the OU). He's going make Eloy's suggested himself, to prove he knows how to submit changes for integration now!

          Show
          Jenny Gray added a comment - I did this work as a demo to train Jason Platts (one of the other lead developers here at the OU). He's going make Eloy's suggested himself, to prove he knows how to submit changes for integration now!
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Yay, thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - Yay, thanks!
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Updating git repo URL to proper one (welcome Jason, btw!).

          Show
          Eloy Lafuente (stronk7) added a comment - Updating git repo URL to proper one (welcome Jason, btw!).
          Hide
          Eloy Lafuente (stronk7) added a comment -

          This has been integrated, thanks! Testing will happen next.

          Note1: I've added one extra commit to delete the global $DB line in your patch. It's global scope already, hence, it is not needed.

          Note2: I've create one followup of this with some of the problems found when looking at current calendar export code: MDL-27542

          Show
          Eloy Lafuente (stronk7) added a comment - This has been integrated, thanks! Testing will happen next. Note1: I've added one extra commit to delete the global $DB line in your patch. It's global scope already, hence, it is not needed. Note2: I've create one followup of this with some of the problems found when looking at current calendar export code: MDL-27542
          Hide
          Glenn Ansley added a comment -

          Confirmed error and confirmed fix. Looks good.

          Show
          Glenn Ansley added a comment - Confirmed error and confirmed fix. Looks good.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Closing this, it's already part of upstream, thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - Closing this, it's already part of upstream, thanks!

            People

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

              Dates

              • Created:
                Updated:
                Resolved: