Payment method controllers only contain logic and some configuration to identify the controllers. Within the class there is no relationship with data outside the class. The class's methods accept any input of a certain type, generate and return output and then throw away the input. Never is external data stored in the class.

This means we don't need multiple instances of controller classes. Actually we don't even need a single instance. We currently have payment_method_controller_load_multiple() that statically caches a single instance so we don't have unnecessary identical controllers lying around (identical in terms of functionality, because they are different resources). However, this is a rather ugly factory/singleton-like solution.

A better solution may be to use static methods. These clearly show developers that controller classes are not meant to be instanced and it also practically prevents the classes from being instanced, because instances simply won't work within the system.

Postponed until version 7.x-2.x, but discussion is welcome here.

CommentFileSizeAuthor
#6 payment_1578020_6.patch42.48 KBxano

Comments

xano’s picture

Title: Make payment method controllers use static methods » Rework payment method controllers

This issue boils down to the fact that the current situation is not ideal. It works, but there are a few possible pitfalls for people who don't fully read or understand the documentation:

  1. It's possible to give a PaymentMethod controller data and a controller that don't match, causing execution will fail.
  2. It's possible to omit default values for a PaymentMethod's controller data, causing execution to fail.
  3. As mentioned before, controllers are instantiated, but only once per page, while they were designed to always do the same thing.

Based on those possibilities, we can write down some requirements:

  1. For every payment method, its type of controller and its controller data should have a 'circular dependency', meaning that if the controller type is changed, the controller data automatically needs to reflect the new controller type and vice versa, so those two never get out of sync.
  2. The previous requirement is closely related to having a way to easily and surely set default configs for payment methods depending on their controller types.
  3. If controllers are instantiated only once, make sure the code reflects that. A solution is to call all controller methods statically. However, some sources say this hurts testing and extensibility. We need to confirm if that is the case with our current setup, where we can easily replace controllers using hook_payment_method_controller_info_alter().
  4. In order to make it easier to export or clone a payment method, or change its controller type, we need a consistent and preferably simple way to get its config, so it can be used for a new payment method.
xano’s picture

xano’s picture

Make payment methods be instances of payment method controllers, so we lose a abstraction and add flexibility.

xano’s picture

Title: Rework payment method controllers » Convert payment methods to plugins

As the next major branch will be for Drupal 8, we can leverage its plugin API:

  • Convert payment methods to config entities
  • Convert payment method types to plugins. These classes should extend the payment method entity class. This means that controller data properties are now well-documented first class properties of payment methods.
  • Convert payment method controllers to payment execution controllers for payment method plugins
  • Store payment methods' plugin names in an entity property, possibly as the bundle name.
  • Write a custom config storage controller that decides what class to use for payment method entities based on the plugin name that is optionally stored as entity bundle name.

These steps will solve all the issues raised in #1.

xano’s picture

Version: 7.x-1.x-dev » 8.x-2.x-dev
Status: Postponed » Active
xano’s picture

Status: Active » Fixed
StatusFileSize
new42.48 KB

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.