Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RFC: Remove class based message-types #183

Open
bummzack opened this issue Feb 10, 2018 · 3 comments
Open

RFC: Remove class based message-types #183

bummzack opened this issue Feb 10, 2018 · 3 comments
Labels

Comments

@bummzack
Copy link
Collaborator

This is a follow up to this question, which didn't get any feedback: #123

Motivation

The gateway message classes don't contain any logic or custom data and their only purpose at the moment is to generate a custom value in the ClassName ENUM.
Instead of cluttering the source-files with lots of individual class files, I'd like to propose this new architecture:

Merge two top level message classes

PaymentMessage and GatewayMessage get merged into one class, named PaymentMessage.

I don't see a future use for payment messages without a Gateway, since all our messages are going to revolve around Gateways anyway. For other types of messages, we have separate means of logging now.

The third class that has separate data GatewayRequestMessage can remain as a separate class (maybe rename to PaymentRequestMessage for consistency).

Using a separate naming scheme (PaymentMessage instead of GatewayMessage) also helps differentiate between native omnipay classes and our own classes.

Replace subclasses with a DB-field

All the existing subclasses are basically there to build the ENUM in the PaymentMessage table. I propose to remove these subclasses and create a Type field on PaymentMessage. This can be an indexed string field or an enum.

All message types will be defined as const in the codebase and can be used to create and look up messages by type. These constants should be defined in the service that generates the messages, so that new services can add their own.

Advantages

  • Less classes, which makes the architecture easier to understand
  • Better readability of the payment message type. With the namespaces, the types have now become really long, which makes it harder to read (eg. in the CMS or when looking at the Database or a logfile). Example: SilverStripe\Omnipay\Model\Message\PurchaseRequest instead of just PurchaseRequest
  • Types will be defined by the classes that generate these messages

Disadvantages

  • Upgrading will become more complex.
  • There's no longer a tight coupling between a PaymentMessage subclass and its "type".
  • You can't decorate or use Injector to alter a single message-type (since the message-types don't contain any logic or data, I currently don't see any use-case for this)
@bummzack bummzack added the RFC label Feb 10, 2018
@mlewis-everley
Copy link
Contributor

@bummzack I read issue #123 first so have posted a comment there.

Basically I agree with you, I was thinking rather than an enum though, use something like a varchar and assign possible types as a config variable.

This means if users wanted to add more statuses (though I am unsure why), they can do it fairly easily?

Also enum's are a little annoying (see also @dhensby's rant on slack a few weeks ago) :-)

@bummzack
Copy link
Collaborator Author

bummzack commented Jul 12, 2018

What is the problem with Enums? I think Varchar is a poor replacement for Enum.

If not using an Enum, I think the proper substitution would be a relation.

@dhensby
Copy link
Contributor

dhensby commented Jul 13, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants