Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-49660

Review the message_send() chain and normalize its return values

    XMLWordPrintable

    Details

    • Type: Bug
    • Status: Open
    • Priority: Minor
    • Resolution: Unresolved
    • Affects Version/s: 2.8.5, 2.9
    • Fix Version/s: None
    • Component/s: Messages
    • Labels:
    • Affected Branches:
      MOODLE_28_STABLE, MOODLE_29_STABLE

      Description

      This was discovered while testing MDL-48499, that implements the display of failed messages in bulk messaging.

      In order to send any message in moodle we have a long chain of code being used (details below). And, right now, the chain does not handle return values in a consistent way. Parts of the chain do assume message id or false are the returned values. Others do return true or false. And others do return always, unconditionally the message id.

      The chain is as follows:

      1. message_post_message() => supports return false and calls
      2. message_send() => supports return false and calls
      3. \core\message\manager::send_message() => NO FALSE support and calls
      4. \core\message\manager::send_message_to_processors() => NO FALSE support and calls
      5. message_output->send_message() on every processor that apparently support false.

      As a result of that, the original specifications of both 1. and 2. above are not observed and they only return false "sometimes".

      So this is about to guarantee that 1. and 2. do return false consistently, no matter where in the chain the fail happened.

      Note that there are a number of uses of those 2 functions using the returned value for various things (some of them to check if it's valid or no), and potentially, all them may be missing failures. I looked quickly for them with:

      grep -rP '(= *|if \( *!* *)message_post_message'
       
      and
       
      grep -rP '(= *|if \( *!* *)message_send'
      

      Some of them (ajax, external function, forum... ) seem to be expecting a false there and simply, they are not, because of the broken chain.

      Note that this does not imply that all the methods in the chain must return false, maybe it's not needed to change that. What we need is to be able to inform to callers about the known failures. if that's done via return values or in any other way... is one of the things to decide here.

      So the GOAL of this is to guarantee that the outer APIs (1. and 2.) do know about failures as often as possible. And return the expected false in those cases.

      Ciao

        Attachments

          Issue Links

            Activity

              People

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

                Dates

                • Created:
                  Updated: