Moodle
  1. Moodle
  2. MDL-39398

Notice while sending Jabber notifications

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 2.3.6, 2.4.3, 2.5
    • Fix Version/s: 2.3.7, 2.4.4
    • Component/s: Messages
    • Labels:
    • Rank:
      50037

      Description

      Following MDLQA-5431, the following notice appears when:

      • Cron is sending forum notifications
      • Cron is sending assignments notifications
      • Sending private message to user
      Notice: Undefined offset: 1 in /home/fred/www/repositories/im/moodle/lib/jabber/XMPP/Roster.php on line 121
      
      Call Stack:
          0.0005     828904   1. {main}() /home/fred/www/repositories/im/moodle/message/index.php:0
          0.1334   40055608   2. message_post_message() /home/fred/www/repositories/im/moodle/message/index.php:180
          0.1340   40059984   3. message_send() /home/fred/www/repositories/im/moodle/message/lib.php:2078
          0.1669   40959112   4. message_output_jabber->send_message() /home/fred/www/repositories/im/moodle/lib/messagelib.php:212
          2.9818   41005840   5. XMPPHP_XMLStream->disconnect() /home/fred/www/repositories/im/moodle/message/output/jabber/message_output_jabber.php:82
          2.9819   41006064   6. XMPPHP_XMLStream->processUntil() /home/fred/www/repositories/im/moodle/lib/jabber/XMPP/XMLStream.php:358
          3.2669   41007112   7. XMPPHP_XMLStream->__process() /home/fred/www/repositories/im/moodle/lib/jabber/XMPP/XMLStream.php:471
          3.2670   41012960   8. xml_parse() /home/fred/www/repositories/im/moodle/lib/jabber/XMPP/XMLStream.php:420
          3.2672   41018488   9. XMPPHP_XMLStream->endXML() /home/fred/www/repositories/im/moodle/lib/jabber/XMPP/XMLStream.php:0
          3.2673   41020024  10. XMPPHP_XMPP->presence_handler() /home/fred/www/repositories/im/moodle/lib/jabber/XMPP/XMLStream.php:567
          3.2673   41021608  11. Roster->setPresence() /home/fred/www/repositories/im/moodle/lib/jabber/XMPP/XMPP.php:252
      

        Issue Links

          Activity

          Hide
          Frédéric Massart added a comment -

          When calling the disconnect() method, the Jabber API attempts to subscribe the server to the presence of Jabber username specified in the admin settings. Not only this is probably not necessary at all, but also it creates a notice because the server name, which should be blah@jabber.org/resource, does not have a resource identifier. Therefore an explode('/') fails.

          I think it's safe to remove the presence tracking at all. Subscribing the server to our user is unnecessary, and sending 'available' to the server during a disconnect() method is pointless (which is what happens after the subscribe attempt).

          Pushing for peer review.

          Show
          Frédéric Massart added a comment - When calling the disconnect() method, the Jabber API attempts to subscribe the server to the presence of Jabber username specified in the admin settings. Not only this is probably not necessary at all, but also it creates a notice because the server name, which should be blah@jabber.org/resource, does not have a resource identifier. Therefore an explode('/') fails. I think it's safe to remove the presence tracking at all. Subscribing the server to our user is unnecessary, and sending 'available' to the server during a disconnect() method is pointless (which is what happens after the subscribe attempt). Pushing for peer review.
          Hide
          Dan Poltawski added a comment -

          Hi Fred,

          I think your solution is pragmatic.. but nitpicking:

          1. Surely it'd be better to disable the presence tracking earlier in the process than right before the disconnect
          2. I'm slightly suspicious that we are the ones who added that explode(), MDL-20876
          3. I don't think that level of commenting is required (that'd be perfect for a commit message, though).
          Show
          Dan Poltawski added a comment - Hi Fred, I think your solution is pragmatic.. but nitpicking: Surely it'd be better to disable the presence tracking earlier in the process than right before the disconnect I'm slightly suspicious that we are the ones who added that explode(), MDL-20876 I don't think that level of commenting is required (that'd be perfect for a commit message, though).
          Hide
          Frédéric Massart added a comment -

          Thanks Dan,

          1/ I wasn't sure about it, I played it safe by only disabling that at the moment causing an issue. Though I amended my patch to reflect your POV.
          2/ You're right, the original API uses a split(), though it would still have caused a notice to be raised due to the use of list().
          3/ Done.

          Pushing for integration, cheers!

          Show
          Frédéric Massart added a comment - Thanks Dan, 1/ I wasn't sure about it, I played it safe by only disabling that at the moment causing an issue. Though I amended my patch to reflect your POV. 2/ You're right, the original API uses a split(), though it would still have caused a notice to be raised due to the use of list(). 3/ Done. Pushing for integration, cheers!
          Hide
          Sam Hemelryk added a comment -

          Thanks Fred, this has been integrated now.

          Show
          Sam Hemelryk added a comment - Thanks Fred, this has been integrated now.
          Hide
          Andrew Davis added a comment -

          All seems fine now. Passing.

          Show
          Andrew Davis added a comment - All seems fine now. Passing.
          Hide
          Dan Poltawski added a comment -

          Thanks! You're changes are now spread to the world through this git and our source control repositories.

          No time to rest though, we've got days to make 2.5 the best yet!

          ciao

          Show
          Dan Poltawski added a comment - Thanks! You're changes are now spread to the world through this git and our source control repositories. No time to rest though, we've got days to make 2.5 the best yet! ciao

            People

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

              Dates

              • Created:
                Updated:
                Resolved: