compucorp / uk.co.compucorp.membershipextras

Membership Extras for CiviCRM
Other
5 stars 8 forks source link

MAE-636: Reorganizing the upgrader #417

Closed omarabuhussein closed 2 years ago

omarabuhussein commented 2 years ago

Before

The code that we run during the extension life cycle (installation, disabling, enabling, uninstallation) as well as the upgraders code was all in one large file, the upgrader.php file. Also we have a lot of old upgraders that are no longer relevant since we upgraded all of our clients to a version > 4.6.0.

After

1- Upgraders for Membershipextras version <= 4.6.0 are removed, the remaining two are moved to Upgrader/Steps/ directory, and new code is added to Upgrader.php to allow CiviCRM to process these files, the extension now follows our standard that is established here: https://compucorp.atlassian.net/wiki/spaces/SD/pages/835092529/Adding+a+CiviCRM+Upgrader

2- I also moved the code that gets called during the extension life cycle (during the installation, disabling, enabling, uninstallation of this extension), so each thing the extension do is moved to new folder called Setup, the Setup directory contains two directories: Configure and Manage which are to be used as following:

an example:

    $creationSteps = [
      new CRM_MembershipExtras_Setup_Manage_OfflineAutoRenewalScheduledJob(),
      new CRM_MembershipExtras_Setup_Manage_ManualPaymentProcessorType(),
      new CRM_MembershipExtras_Setup_Manage_ManualPaymentProcessor(),
      new CRM_MembershipExtras_Setup_Manage_PaymentPlanActivityTypes(),
      new CRM_MembershipExtras_Setup_Manage_FutureMembershipStatusRules(),
    ];
    foreach ($creationSteps as $step) {
      $step->create();
    }

3- I also moved the code that creates the External ID custom fields/groups on the Recur Contributions, Contributions, Memberships and Line items from Membershipextras extension to uk.co.compucorp.membershipextrasimporterapi extension, given that these custom fields/groups are only created for the importer use, so it make more sense to have them there.

Aside from that I did some improvements to some entities to ensure they get created, disabled, enabled and removed properly such as the custom groups defined by this extension, since it was not the case before and resulted in errors if you uninstall then try to install the extension again.

olayiwola-compucorp commented 2 years ago

some clarification @omarabuhussein, is there any reason the methods in CRM_MembershipExtras_Setup_Manage_AbstractManager are not named postInstall(), uninstall(), enable() and disable() instead of the names used create(), remove() e.t.c. I'm thinking the names used are a bit restrictive.

secondly, Is it also safe to assume that there wouldn't be a need to undo any configuration applied by a class that implements this interface CRM_MembershipExtras_Setup_Configure_ConfigurerInterface when the extension is being uninstalled?

omarabuhussein commented 2 years ago
some clarification @omarabuhussein, is there any reason the methods in CRM_MembershipExtras_Setup_Manage_AbstractManager are not named postInstall(), uninstall(), enable() and disable() instead of the names used create(), remove() e.t.c. I'm thinking the names used are a bit restrictive.

secondly, Is it also safe to assume that there wouldn't be a need to undo any configuration applied by a class that implements this interface CRM_MembershipExtras_Setup_Configure_ConfigurerInterface when the extension is being uninstalled?

You could argue that I could have just created one interface such as CRM_MembershipExtras_Setup_UpgradeInterface instead of the two I created above, with the following methods:

install, uninstall, disable and enable and use it for both Entities and Configurations, but I think this mix different concepts together and does not do a good job when it comes to separation of concerns.

The way I see it, managing entities is different than applying configurations, you "create", "enable", "disable" and "uninstall" entities, but when it comes to configurations you "apply" them, and hence that is why I created the two interfaces. Also if it happen we want to undo certain configurations when uninstalling, disabling or enabling the extension in the future, I would rather change the CRM_MembershipExtras_Setup_Configure_ConfigurerInterface interface to accommodate for such cases rather than combining it with one interface that will be used for both entities and configurations, because each is different concept and it does not make sense to me (at least for now) for both to be under the same interface.

Also I don't recall that we before needed to undo or apply certain configs when we uninstall, disable or enable extensions, we have live example here, Membershipextras has been under active development for years and we never needed to do such thing.

olayiwola-compucorp commented 2 years ago
some clarification @omarabuhussein, is there any reason the methods in CRM_MembershipExtras_Setup_Manage_AbstractManager are not named postInstall(), uninstall(), enable() and disable() instead of the names used create(), remove() e.t.c. I'm thinking the names used are a bit restrictive.

secondly, Is it also safe to assume that there wouldn't be a need to undo any configuration applied by a class that implements this interface CRM_MembershipExtras_Setup_Configure_ConfigurerInterface when the extension is being uninstalled?

You could argue that I could have just created one interface such as CRM_MembershipExtras_Setup_UpgradeInterface instead of the two I created above, with the following methods:

install, uninstall, disable and enable and use it for both Entities and Configurations, but I think this mix different concepts together and does not do a good job when it comes to separation of concerns.

The way I see it, managing entities is different than applying configurations, you "create", "enable", "disable" and "uninstall" entities, but when it comes to configurations you "apply" them, and hence that is why I created the two interfaces. Also if it happen we want to undo certain configurations when uninstalling, disabling or enabling the extension in the future, I would rather change the CRM_MembershipExtras_Setup_Configure_ConfigurerInterface interface to accommodate for such cases rather than combining it with one interface that will be used for both entities and configurations, because each is different concept and it does not make sense to me (at least for now) for both to be under the same interface.

Also I don't recall that we before needed to undo or apply certain configs when we uninstall, disable or enable extensions, we have live example here, Membershipextras has been under active development for years and we never needed to do such thing.

Thanks @omarabuhussein for the detailed explanation, It's fine for now for our current use case.