Moodle
  1. Moodle
  2. MDL-30541

PHP error shown when viewing single blog entry

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.0.6, 2.1.3, 2.2, 2.3
    • Fix Version/s: 2.0.7, 2.1.4, 2.2.1
    • Component/s: Blog
    • Labels:
    • Environment:
      Appeared on qa.moodle.net and my own test servers
    • 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_20_STABLE, MOODLE_21_STABLE, MOODLE_22_STABLE, MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_20_STABLE, MOODLE_21_STABLE, MOODLE_22_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      wip-MDL-30541-master
    • Rank:
      33255

      Description

      The following PHP error is displayed as a page is loading for a single blog entry.

      Notice: Trying to get property of non-object in D:\xampp\htdocs\moodle_testing\blog\rsslib.php on line 60
      

      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 is displayed as the page is loaded.

      Unlike MDL-30540, this affects all 2.x branches, but it must be a recent regression as my master from a couple of weeks ago did not show this problem.

        Issue Links

          Activity

          Hide
          Jason Fowler added a comment -

          if this passes peer review, I will cherry pick/port it to 2.0 and 2.1 if needed.

          Show
          Jason Fowler added a comment - if this passes peer review, I will cherry pick/port it to 2.0 and 2.1 if needed.
          Hide
          Sam Hemelryk added a comment -

          Hi Jason,

          I've just been looking at this now.
          The solution you have does stop the error but in turn it prevents the RSS header functionality from being run and in fact masks the actual problem.
          I reproduced this as an admin and received the following two errors on that page under master:

          Notice: Trying to get property of non-object in /private/var/www/integration/blog/rsslib.php on line 60

          and mid page

          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()

          I added debugging to get a stack trace on the first issue, by the looks $rsscontext is empty, and judging by a quick glance at the following code, it should never be empty and should instead default to system context.

          The second issue already has a stack trace so is easier to debug.
          Line line 317 of /blog/lib.php

          $context->contextlevel = CONTEXT_SYSTEM;

          That is insanely evil code, under no circumstances should the level ever be updated by itself... thats 100% creating an invalid context.
          In 2.2 Petr implemented better context handling (from memory 2.2) and now evil hacks like this can be detected and prevented.

          I'll leave the rest up to you, I'd consider fixing both of these on one issue (as both prevent the code).
          It'd pay to update the test instructions as well to make sure that a valid RSS header for alternative content is added.
          Also keep in mind that because the context stuff was implemented in 2.2 your 20, and 21 branches will differ from your 22 and master branches.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi Jason, I've just been looking at this now. The solution you have does stop the error but in turn it prevents the RSS header functionality from being run and in fact masks the actual problem. I reproduced this as an admin and received the following two errors on that page under master: Notice: Trying to get property of non-object in /private/var/www/integration/blog/rsslib.php on line 60 and mid page 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() I added debugging to get a stack trace on the first issue, by the looks $rsscontext is empty, and judging by a quick glance at the following code, it should never be empty and should instead default to system context. The second issue already has a stack trace so is easier to debug. Line line 317 of /blog/lib.php $context->contextlevel = CONTEXT_SYSTEM; That is insanely evil code, under no circumstances should the level ever be updated by itself... thats 100% creating an invalid context. In 2.2 Petr implemented better context handling (from memory 2.2) and now evil hacks like this can be detected and prevented. I'll leave the rest up to you, I'd consider fixing both of these on one issue (as both prevent the code). It'd pay to update the test instructions as well to make sure that a valid RSS header for alternative content is added. Also keep in mind that because the context stuff was implemented in 2.2 your 20, and 21 branches will differ from your 22 and master branches. Cheers Sam
          Hide
          Sam Hemelryk added a comment -

          Found MDL-30541 and have commented there as well.

          Show
          Sam Hemelryk added a comment - Found MDL-30541 and have commented there as well.
          Hide
          Sam Hemelryk added a comment -

          Thanks Jason, looks good to me now.
          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Thanks Jason, looks good to me now. Cheers Sam
          Hide
          Jason Fowler added a comment -

          Code cherry picked across to the other branches

          Show
          Jason Fowler added a comment - Code cherry picked across to the other branches
          Hide
          Aparup Banerjee added a comment -

          Hi Jason,
          I've just had a look at this and i've got some small but still pressing points

          • we need to think about code readability (clarity over brevity) when we're coding (that and why are we giving the instruction $rsscontext = $rsscontext (when its not empty) )
          • also is this a security issue? is that why a fix for 2.0 branch was included? i'm not sure here.

          I'll reopen so that we can move towards more readability :-D

          cheers,
          Apu

          Show
          Aparup Banerjee added a comment - Hi Jason, I've just had a look at this and i've got some small but still pressing points we need to think about code readability ( clarity over brevity) when we're coding (that and why are we giving the instruction $rsscontext = $rsscontext (when its not empty) ) also is this a security issue? is that why a fix for 2.0 branch was included? i'm not sure here. I'll reopen so that we can move towards more readability :-D cheers, Apu
          Hide
          Jason Fowler added a comment -

          Hey Aparup,

          I'll make the code easier to understand, removing the ternary operator in favour of a simple if statement.
          I provided a patch for 2.0 because everytime I've asked "when do we stop supporting 2.0?" I've been told December. And when I ask "when? December 1st or December 31st?" I've been told "Yes" ... and with such a trivial one-line patch, I thought I'd include it, to allow some one with more authority on the matter to decide.

          Show
          Jason Fowler added a comment - Hey Aparup, I'll make the code easier to understand, removing the ternary operator in favour of a simple if statement. I provided a patch for 2.0 because everytime I've asked "when do we stop supporting 2.0?" I've been told December. And when I ask "when? December 1st or December 31st?" I've been told "Yes" ... and with such a trivial one-line patch, I thought I'd include it, to allow some one with more authority on the matter to decide.
          Hide
          Jason Fowler added a comment -

          All repatched now, with greater clarity, Aparup, could you please look at this before I push it any further through the process?

          Note: I also cleaned up the variable initialization, to make it cleaner and easier to read.

          Show
          Jason Fowler added a comment - All repatched now, with greater clarity, Aparup, could you please look at this before I push it any further through the process? Note: I also cleaned up the variable initialization, to make it cleaner and easier to read.
          Hide
          Jason Fowler added a comment -

          Just squished the commits for 2.0 and 2.1 to make it easier to integrate

          Show
          Jason Fowler added a comment - Just squished the commits for 2.0 and 2.1 to make it easier to integrate
          Hide
          Aparup Banerjee added a comment -

          Thanks Jason, thats been integrated now.

          I've included it in 2.0 as you're patch applies cleanly and is minor. (and its still December by your argument :-p)

          ps: also a missing context really doesn't sound good at all.

          Show
          Aparup Banerjee added a comment - Thanks Jason, thats been integrated now. I've included it in 2.0 as you're patch applies cleanly and is minor. (and its still December by your argument :-p) ps: also a missing context really doesn't sound good at all.
          Hide
          Michael de Raadt added a comment -

          Test result: passed Tested in 2.1 and master

          Show
          Michael de Raadt added a comment - Test result: passed Tested in 2.1 and master
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Whoever decided one week was worth 14 days had really one bad idea. Anyway, the nightmare is over, so thanks for your, once again, amazing contributions. Many, many thanks!

          Now... disconnect, relax and enjoy the next days, yay!

          Closing...ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Whoever decided one week was worth 14 days had really one bad idea. Anyway, the nightmare is over, so thanks for your, once again, amazing contributions. Many, many thanks! Now... disconnect, relax and enjoy the next days, yay! Closing...ciao

            People

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

              Dates

              • Created:
                Updated:
                Resolved: