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

Bring structural consistency to the plugin #138

Merged
merged 3 commits into from
Oct 24, 2018

Conversation

christianwach
Copy link
Member

Overview

The purpose of this PR is fourfold:

  1. To improve the way this plugin bootstraps.
  2. To improve the activation process.
  3. To improve encapsulation in child classes.
  4. To lay the groundwork for "Clean URLs" in CiviCRM.

There aren't any changes to functionality in this PR - just structural changes.

Before

N/A

After

N/A

Technical Details

A bit of background: I accidentally discovered that the civicrm_instance_loaded action fires prior to plugins_loaded even when "late loaded" via the CIVICRM_LATE_LOAD constant. This led to an unfortunate plugin "missing" the civicrm_instance_loaded action by registering its callback after the hook had fired.

Given that the load order of other plugins cannot necessarily be guaranteed, plugins should be wary of doing anything other than assembling themselves and registering hook callbacks prior to plugins_loaded. It makes sense to me that CiviCRM should follow a similarly standard load procedure - hence this PR.

One immediate benefit of these changes is that plugins which are dependent on CiviCRM can reliably hook into civicrm_instance_loaded and do what they need to do knowing that CiviCRM is available. No more if ( function_exists('civi_wp') ) because the action won't fire if this plugin is deactivated or deleted.

FWIW, setting the CIVICRM_LATE_LOAD constant had no effect because civi_wp() was called in the register_activation_hook and register_deactivation_hook calls directly below the plugins_loaded hook registration. Oops, my bad :)

The load procedure after this refactoring is therefore as follows:

  1. Instantiate main plugin class when this plugin is first included.
  2. Hook plugin setup to plugins_loaded at the end of which civicrm_instance_loaded fires, thus ensuring that all other plugins can receive callbacks from it.
  3. Hook common routines and admin setup to the init hook, since nothing of interest to CiviCRM happens before then. Also, "Clean URLs" is going to need to register its rewrite rules at this point.
  4. Delay everything else until query has been parsed. "Clean URLs" is going to need to access query vars which should be set up at this point.

The improvements to encapsulation have been made via:

  1. Introducing two new actions: civicrm_activation and civicrm_deactivation which child classes can listen for.
  2. Basepage creation methods have been moved to the CiviCRM_For_WordPress_Basepage class and are triggered by the new actions.
  3. Assigning minimum capabilities for all WordPress roles on activation has likewise been delegated to CiviCRM_For_WordPress_Users.

Comments

The PR also:

  • Adds some previously-missing docblocks for actions and filters.
  • Merges the duplicate civicrm_menu_item_position filters into one.

I have left the three commits separate so that it is easier to see each step.

@bhahumanists
Copy link

bhahumanists commented Oct 23, 2018

We're running this on our live site and haven't seen any issues.

@christianwach
Copy link
Member Author

Thanks for testing @bhahumanists - much appreciated!

@kcristiano
Copy link
Member

Tested a Fresh install and upgrade as well as ran through adding Contribtions, New memberships, renewal, and Event Registration.

Code improvements make sense. Looks good to merge.

@kcristiano kcristiano merged commit 807ef20 into civicrm:master Oct 24, 2018
@christianwach christianwach deleted the plugin-init branch November 12, 2018 13:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants