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

Review the message_send() chain and normalize its return values



    • 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:


      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'
      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.



          Issue Links



              stronk7 Eloy Lafuente (stronk7)
              Component watchers:
              Amaia Anabitarte, Carlos Escobedo, Ferran Recio, Sara Arjona (@sarjona), Víctor Déniz Falcón
              0 Vote for this issue
              2 Start watching this issue