MessageNotifierBase API is confusing [#2817857]

Problem/Motivation

Issue 1: Send method does too many things (render + sending process enforcement). Complicating customization in each of them due to code duplication.

Issue 2: The method postSend method does too many things (decides to save or not, assigns rendered output to fields based on configuration, performs the actual saving of the message to DB). Complicating customization in each of them due to code duplication.

issue 3: On successful delivery and missing fields, an exception is thrown, preventing the save of the message (even if it should be saved). If there is an exception, it would have been better to not deliver the message or silence the error when the message was already delivered. This way some tracking could have been left in the DB for auditing / debugging.

issue 4:
Exposing the use of entity metadata wrappers, due to insufficient validation of whether the field is present on the particular message instance type. When there is a field in the system and it is configured to be used, but it is not part of the particular message instance's type, EntityMetadataWrapper exception is thrown instead of MessageNotifyException.

Proposed resolution

Proposals:
1. Add a designated render method that will handle that and just call it on send. (implemented in PR)
2. Move the sub-actions to separate methods and use them in postSend. (implemented in PR)
3.1. Expose the exception triggering, so custom plugins can decide to silence them in a way. (implemented in PR)
3.2. Move the fields assignment before the delivery of the message.
4. Add some more validation there and throw our exception. (implemented in PR)

Remaining tasks

Discussion for the API changes proposed.
Review the PR in github (https://github.com/Gizra/message_notify/pull/36).
Patch (if needed).

User interface changes

None.

API changes

API addition for anyone that wants to implement custom notifiers.

Data model changes

None.

Comments

Comment #1

13 October 2016 at 14:59

ndobromirov created an issue. See original summary.

Comment #2

ndobromirov CreditAttribution: ndobromirov at FFW commented 14 October 2016 at 16:15

This reflects the following idea:
https://github.com/Gizra/message_notify/pull/36

Comment #3

ndobromirov CreditAttribution: ndobromirov at FFW commented 17 October 2016 at 08:51

There are other problems with the current code-base in regards to - separation of concerns.

If a plugin wants to change rendering logic, at the moment this is going to be hard, as it will have to overwrite the send method (quite unintuitive) or hack it in the deliver (breaking the rendered fields functionality after that).

In the proposed PR on github, the send and post-send methods are fully abstracted.
Each separate concern is moved in it's own protected function to allow plugins to modify it (if needed).

This improves both readability & maintainability of the current code-base, but also adds greater flexibility to anyone that will need to implement custom notifier plugins.