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

PCHR-3787: Fix postInstall hooks #2659

Merged
merged 3 commits into from
Jun 5, 2018

Conversation

davialexandre
Copy link
Member

@davialexandre davialexandre commented Jun 4, 2018

Overview

Right after creating a new CiviHR site (with any of the build methods available), CiviCRM would tell us that there were some extension updates available. We run all of an extension upgraders during the installation, so it's not possible for any of them to have any pending updates.

After some investigation, it turns out that this was caused by the extension schema version not being saved to the database after the installation is complete. Without that, Civi doesn't know that the extension is already in the latest version and tells the user that they should update it.

Before

Users would be informed about pending extension updates right after a CiviHR installation.

After

Users will not informed about pending extension updates right after a CiviHR installation.

Technical Details

When developing a CiviCRM extension, it's common to use the civix tool to automatically generated some skeleton code for us. Among this, is the default implementation of some hooks, which is kept in a file named <extension>.civix.php and the Base Upgrader class, which is kept in CRM/<ExtensionNamespace>/Upgrader/Base.php.

This tool is constantly updated and some of these updates change the contents and format of these files. Unfortunately, the files need to be updated manually once a new version of civix is out (by calling the generate command again) and for this reason, it is easy for files to get outdated.

Outdated files are what caused the issue this PR fixes. On totten/civix#79, they changed the way the schema version is saved to the database after an extension is installed. This is how the installation cycle would work before that PR:

  1. Civi calls hook_civicrm_install()
    1.1 The implementation of that hook in <extenstion>.php would call the implementation inside the <extension>.civix.php file
    1.2 The implementation in <extension>.civix.php would call the onInstall() method in the Base Upgrader class
    1.3 The onInstall() method would call the install() method
    1.4 After the install() finishes the installation, onInstall() would save the max revision number (the schema version) to the database
  2. Civi calls hook_civicrm_postInstall()
    2.1 Nothing would happen, as the extension didn't implement the postInstall() hook

And this is how it works after that PR:

  1. Civi calls hook_civicrm_install()
    1.1 The implementation of that hook in <extenstion>.php would call the implementation inside the <extension>.civix.php file
    1.2 The implementation in <extension>.civix.php would call the onInstall() method in the Base Upgrader class
    1.3 The onInstall() method would call the install() method
  2. Civi calls hook_civicrm_postInstall()
    2.1 The implementation of that hook in <extenstion>.php would call the implementation inside the <extension>.civix.php file
    2.2 The implementation in <extension>.civix.php would call the onPostInstall() method in the Base Upgrader class
    2.3 The onPostInstall() saves the schema version to the database

Note that now the responsibility of saving the schema version to the database belongs to the onPostInstall() method. Also, note that in order for this to work, it is expected that extensions implement the hook_civicrm_postInstall() hook. Part of the changes introduced in totten/civix#79 is exactly about making sure that new extensions created with it would implement that by default. Unfortunately, some CiviHR extension were created before that and the automatically generated files have not been updated ever since. This PR updates these files and make sure that they implement hook_civicrm_postInstall() and call the onPostInstall() method in the Base Upgrader.

Comments

Because of this problem, running drush cvapi Extension.upgrade on a CiviHR site for the first time after an installation, would run all the upgraders again and result in some duplicated data. A separate PR will be created later to remove this duplicated data from existing sites.

Only these 3 extension have been updated because they were in a unique scenario where the Base Upgrader was created after totten/civix#79, but the <extension>.php file was created before that. So the Base Upgrader already contained the change where the schema was saved in the onPostInstall() method, but there was no hook calling it. The other extensions still have the old Upgrader which saves the schema version in the onInstall() method. So, other than having some outdated files, everything is good there. In order to reduce the number of changes in this PR (and the risk of breaking something with these changes) I decided to keep them as is.

@davialexandre davialexandre force-pushed the PCHR-3787-fix-post-install-hooks branch from cfb3b16 to d7a2419 Compare June 4, 2018 17:01
* @link http://wiki.civicrm.org/confluence/display/CRMDOC/hook_civicrm_entityTypes
*/

function _hrleaveandabsences_civix_civicrm_entityTypes(&$entityTypes) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just curious, but did this change affect anything else? Seems like changing it to return an array of entities would have some impact somewhere

Copy link
Contributor

@mickadoo mickadoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a strange one to review. There are plenty of places in the code where I'd say the formatting is a bit odd, or the doc-blocks are missing. Also the whole flow using $this->revisionStorageIsDeprecated is confusing. However I can see this is either generated or boilerplate code so I didn't want to ask you to change something that would be overwritten anyway. Because of that, and because the explanation for the change makes sense, I'm approving, but if there's some area in particular that you think needs extra attention I can review it again.

@davialexandre davialexandre merged commit 8efdc99 into staging Jun 5, 2018
@davialexandre davialexandre deleted the PCHR-3787-fix-post-install-hooks branch June 5, 2018 11:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants