If an exception is thrown we should probably trigger the onUserFailure plugin event and continue. Just like if there's something returned false now.

@wilsonge Is that better?

Personally yes :)

Any reason to use $ex instead of $e as usually in core?

it's IDE autocomplete, i'll change it.

Overall I like this, however I think we need to wrap the onUserAfterLogout new plugin event in a separate try/catch. Once a user has passed through onUserLogout then they are logged out and any further exceptions should not mean that onUserLogoutFailure will get triggered. Otherwise you're going to get serious bugs

@wilsonge
if i understand you correctly, i have to do something like this:

// OK, the credentials are built. Lets fire the onLogout event.
try
{
  $results = $this->triggerEvent('onUserLogout', array($parameters, $options));
}
catch (Exception $e)
{
  $this->enqueueMessage($e->getMessage());
}

// Check if any of the plugins failed. If none did, success.
try 
{
  if (isset($results) && is_array($results) && !in_array(false, $results, true))
  {
    $options['username'] = $user->get('username');
    $this->triggerEvent('onUserAfterLogout', array($options));

    return true;
  }

  // Trigger onUserLoginFailure Event.
  $this->triggerEvent('onUserLogoutFailure', array($parameters));
}
catch(Exception $e)
{
  $this->enqueueMessage($e->getMessage());
}

@jackkum That code snippet looks good to me.

In your first try/catch you need to trigger onUserLogoutFailure in the catch and return OR define $results as false.

@wilsonge
If i do trigger onUserLogoutFailure in the catch i have to do this twise, because old plugins can return false instead throws exception.

$results is undefined, so isset() will return false, this can be not obvious, and maybe better will define $results before try/catch.

@jackkum I guess that is the downside of backwards compatibility, we have to take into account both cases.

Hello @jackkum

Thank you for your contribution.

The last comment here was on June 4th. Are you still interested in updating this pull request?
If no reply is received within 4 weeks we will close this issue.

Thanks for understanding!

@roland-d 4 weeks are gone.