Moodle
  1. Moodle
  2. MDL-30540

Viewing a single blog entry shows an error

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.2, 2.3
    • Fix Version/s: 2.2.2
    • Component/s: Blog
    • Labels:
    • Environment:
      Appeared on qa.moodle.net and a test server of 2.2 beta.
    • Database:
      MySQL
    • Testing Instructions:
      Hide
      1. Log in in any role
      2. Navigate to Site pages -> Blogs
      3. If there are no blog entries, add one
      4. Click on the title for a blog entry

      No errors should be displayed.

      Show
      Log in in any role Navigate to Site pages -> Blogs If there are no blog entries, add one Click on the title for a blog entry No errors should be displayed.
    • Affected Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_22_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      wip-MDL-30540-master
    • Rank:
      33254

      Description

      When viewing a single blog entry, the following error is shown.

      Can not change context instance properties!
      
          line 4642 of \lib\accesslib.php: call to debugging()
          line 317 of \blog\lib.php: call to context->__set()
          line 698 of \blog\lib.php: call to blog_get_context_url()
          line 824 of \blog\locallib.php: call to blog_get_headers()
          line 234 of \blog\index.php: call to blog_listing->print_entries()
      

      The entry still displays.

      Replication steps:

      1. Log in in any role
      2. Navigate to Site pages -> Blogs
      3. If there are no blog entries, add one
      4. Click on the title for a blog entry

      The error above should appear.

        Issue Links

          Activity

          Hide
          Aparup Banerjee added a comment -

          $PAGE->set_context() for CONTEXT_SYSTEM have been known to be missing here and there due to changes way in the begining of this year i think, do check for other pages around that might be missing contexts.

          Show
          Aparup Banerjee added a comment - $PAGE->set_context() for CONTEXT_SYSTEM have been known to be missing here and there due to changes way in the begining of this year i think, do check for other pages around that might be missing contexts.
          Hide
          Sam Hemelryk added a comment -

          Just found this... see my comment on MDL-30541.
          The patch on this issue certainly is an improvement and 100% worth doing thanks Jason, but it doesn't fix the actual problem, it avoids it.
          Simple change on blog/lib.php line 317 to remove the dirty hack causing this problem.

          Again testing instructions should include checking there is an alternative link in the head.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Just found this... see my comment on MDL-30541 . The patch on this issue certainly is an improvement and 100% worth doing thanks Jason, but it doesn't fix the actual problem, it avoids it. Simple change on blog/lib.php line 317 to remove the dirty hack causing this problem. Again testing instructions should include checking there is an alternative link in the head. Cheers Sam
          Hide
          Sam Hemelryk added a comment -

          Ohh one more thing:

          get_system_context();
          

          Handy

          Show
          Sam Hemelryk added a comment - Ohh one more thing: get_system_context(); Handy
          Hide
          Sam Hemelryk added a comment -

          Hi Jason,

          I've just been looking closely at this now and I've spotted one more regression in your changes in the context changes in blog/index.php.

          The code used to do:

          if (!empty($courseid)) {
              $PAGE->set_context(get_context_instance(CONTEXT_COURSE, $courseid));
          }
          if (!empty($modid)) {
              $PAGE->set_context(get_context_instance(CONTEXT_MODULE, $modid));
          }
          

          After you changes we have:

          if (!empty($courseid) && ($courseid != SITEID)) {
              $PAGE->set_context(get_context_instance(CONTEXT_COURSE, $courseid));
          } else if (!empty($modid)) {
              $PAGE->set_context(get_context_instance(CONTEXT_MODULE, $modid));
          } else if (empty($entryid) && empty($modid) && empty($groupid)) {
              $PAGE->set_context(get_context_instance(CONTEXT_USER, $USER->id));
          } else {
              $PAGE->set_context(get_context_instance(CONTEXT_SYSTEM, 0));
          }
          

          Before if both $courseid and $modid were !empty then page->url would end up being set to the module context.
          After your changes if both $courseid and $modid are !empty then page->url would be set to course context.

          The blog code was lazy before (too lazy to use an else). Certainly using if..else is clearer however it needs to match what was happening before.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi Jason, I've just been looking closely at this now and I've spotted one more regression in your changes in the context changes in blog/index.php. The code used to do: if (!empty($courseid)) { $PAGE->set_context(get_context_instance(CONTEXT_COURSE, $courseid)); } if (!empty($modid)) { $PAGE->set_context(get_context_instance(CONTEXT_MODULE, $modid)); } After you changes we have: if (!empty($courseid) && ($courseid != SITEID)) { $PAGE->set_context(get_context_instance(CONTEXT_COURSE, $courseid)); } else if (!empty($modid)) { $PAGE->set_context(get_context_instance(CONTEXT_MODULE, $modid)); } else if (empty($entryid) && empty($modid) && empty($groupid)) { $PAGE->set_context(get_context_instance(CONTEXT_USER, $USER->id)); } else { $PAGE->set_context(get_context_instance(CONTEXT_SYSTEM, 0)); } Before if both $courseid and $modid were !empty then page->url would end up being set to the module context. After your changes if both $courseid and $modid are !empty then page->url would be set to course context. The blog code was lazy before (too lazy to use an else). Certainly using if..else is clearer however it needs to match what was happening before. Cheers Sam
          Hide
          Jason Fowler added a comment -

          Fixed the order of the if-then-else so it now sets the context the same way it did originally, and still covers the added scenarios.

          Show
          Jason Fowler added a comment - Fixed the order of the if-then-else so it now sets the context the same way it did originally, and still covers the added scenarios.
          Hide
          Sam Hemelryk added a comment -

          Looks good thanks Jason. Cheery-pick and get it up for integration

          Show
          Sam Hemelryk added a comment - Looks good thanks Jason. Cheery-pick and get it up for integration
          Hide
          Jason Fowler added a comment -

          Cherry-picked to 2.2 and pushed for integration

          Show
          Jason Fowler added a comment - Cherry-picked to 2.2 and pushed for integration
          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
          Eloy Lafuente (stronk7) added a comment -

          Uhm, note that, since 22_STABLE, the get_context_instance() function is deprecated and we should start using context_xxxx::instance() instead.

          So surely this is not a reason for being rejected, but I'll hold this for some hours in case you want to amend it to use the new API.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Uhm, note that, since 22_STABLE, the get_context_instance() function is deprecated and we should start using context_xxxx::instance() instead. So surely this is not a reason for being rejected, but I'll hold this for some hours in case you want to amend it to use the new API. Ciao
          Hide
          Jason Fowler added a comment -

          Thanks for pointing that out Eloy, I will indeed update it to use the new API

          Show
          Jason Fowler added a comment - Thanks for pointing that out Eloy, I will indeed update it to use the new API
          Hide
          Jason Fowler added a comment -

          Okay Eloy, those changes have been made, can you please have a look over them, make sure they are what you expected. I've tested it in terms of the bug presented here and it works.

          Show
          Jason Fowler added a comment - Okay Eloy, those changes have been made, can you please have a look over them, make sure they are what you expected. I've tested it in terms of the bug presented here and it works.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Hi Jason,

          I'm reopening this for next week because:

          1) get_system_context() is also deprecated, so should be context_system::instance() too.
          2) not critical, but for normal use context_system::instance() does not need the 0-param to be specified, you are using it once.

          So it's 99% ok, let's raise it to 100%, thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - Hi Jason, I'm reopening this for next week because: 1) get_system_context() is also deprecated, so should be context_system::instance() too. 2) not critical, but for normal use context_system::instance() does not need the 0-param to be specified, you are using it once. So it's 99% ok, let's raise it to 100%, thanks!
          Hide
          Michael de Raadt added a comment -

          Carrying this over to the next STABLE Sprint.

          Show
          Michael de Raadt added a comment - Carrying this over to the next STABLE Sprint.
          Hide
          Jason Fowler added a comment -

          All those changes have been made now Eloy, thanks for being so thorough

          Show
          Jason Fowler added a comment - All those changes have been made now Eloy, thanks for being so thorough
          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
          Jason Fowler added a comment -

          All rebased now Eloy!

          Show
          Jason Fowler added a comment - All rebased now Eloy!
          Hide
          Eloy Lafuente (stronk7) added a comment -

          And this has found its way, integrated, thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - And this has found its way, integrated, thanks!
          Hide
          Andrew Davis added a comment -

          Looks good.

          Show
          Andrew Davis added a comment - Looks good.
          Hide
          Jason Fowler added a comment -

          Thanks Andrew

          Show
          Jason Fowler added a comment - Thanks Andrew
          Hide
          Eloy Lafuente (stronk7) added a comment -

          A bit later this week, but finally your changes have been accepted and are now available in all the upstream git/cvs servers.

          Many thanks & ciao

          Show
          Eloy Lafuente (stronk7) added a comment - A bit later this week, but finally your changes have been accepted and are now available in all the upstream git/cvs servers. Many thanks & ciao

            People

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

              Dates

              • Created:
                Updated:
                Resolved: