From 34c278a282891bb5d135b418c65b6773f2415bd1 Mon Sep 17 00:00:00 2001 From: Marco Villegas Date: Fri, 19 Nov 2010 01:37:42 -0500 Subject: [PATCH] third review squashed - Remove backend() methods from VersioncontrolEntityInterface. "All methods declared in an interface must be public, this is the nature of an interface." - http://php.net/manual/en/language.oop5.interfaces.php - Non-abstract method VersioncontrolEntity::backendInsert() must contain body. - Make VersioncontrolAccount class use interface signature, but defering the real logic chnage until we have a real one-PK for account. - update versioncontrol_account_status module to the new crud and hooks. - actually return item revisions on VersioncontrolOperation::loadItemRevisions(). - Use consistent signature for save() methos at VersioncontrolEntityInterface. - Pass parameter to backend() and remove unnecessary over-writes from repo class. --- includes/VersioncontrolAccount.php | 134 ++++++------------ includes/VersioncontrolBranch.php | 6 +- includes/VersioncontrolItem.php | 6 +- includes/VersioncontrolOperation.php | 6 +- includes/VersioncontrolRepository.php | 17 +-- includes/VersioncontrolTag.php | 6 +- includes/controllers.inc | 7 +- versioncontrol.pages.inc | 19 ++- .../versioncontrol_account_status.module | 152 ++++++++++++-------- versioncontrol_fakevcs/includes/classes.inc | 1 + 10 files changed, 161 insertions(+), 193 deletions(-) diff --git includes/VersioncontrolAccount.php includes/VersioncontrolAccount.php index 63f5c81..c17aaf3 100644 --- includes/VersioncontrolAccount.php +++ includes/VersioncontrolAccount.php @@ -33,7 +33,7 @@ abstract class VersioncontrolAccount extends VersioncontrolEntity { /** * Repo user id * - * @var VersioncontrolRepository + * @var VersioncontrolRepository */ public $repository; @@ -72,83 +72,57 @@ abstract class VersioncontrolAccount extends VersioncontrolEntity { * Update a VCS user account in the database, and call the necessary * module hooks. The account repository and uid must stay the same values as * the one given on account creation, whereas vcs_username and - * @p $additional_data may change. + * data data members can change. * - * @param $vcs_username - * The VCS specific username (a string). Here we are using an explicit - * parameter instead of taking the vcs_username data member to be able to - * verify is it changed, there would be lots of operations, so we do not - * want to update them if it's not necessary. - * @param $additional_data - * An array of additional author information. Modules can fill this array - * by implementing hook_versioncontrol_account_submit(). - * - * FIXME the function sig here is incompatible with VersioncontrolEntity, and - * needs the logic needs to be fixed to suit. + * FIXME use same logic as in other classes, this probably would + * be changed only when we get account_id PK schema change in. + * TODO review performance impact of updating the vcs_username since + * original jpetso work try to avoid to update if not needed by passing + * it as parameter. */ - public final function update($vcs_username, $additional_data = array()) { + public final function update($options = array()) { $repo_id = $this->repository->repo_id; - $username_changed = ($vcs_username != $this->vcs_username); - - if ($username_changed) { - $this->vcs_username = $vcs_username; - db_query("UPDATE {versioncontrol_accounts} - SET vcs_username = '%s' - WHERE uid = %d AND repo_id = %d", - $this->vcs_username, $this->uid, $repo_id - ); - } - // Provide an opportunity for the backend to add its own stuff. - $this->_update($additional_data); - - if ($username_changed) { - db_query("UPDATE {versioncontrol_operations} - SET uid = 0 - WHERE uid = %d AND repo_id = %d", - $this->uid, $repo_id); - db_query("UPDATE {versioncontrol_operations} - SET uid = %d - WHERE committer = '%s' AND repo_id = %d", - $this->uid, $this->vcs_username, $repo_id); - } - - // Everything's done, let the world know about it! - module_invoke_all('versioncontrol_account', - 'update', $this->uid, $this->vcs_username, $this->repository, $additional_data + db_query("UPDATE {versioncontrol_accounts} + SET vcs_username = '%s' + WHERE uid = %d AND repo_id = %d", + $this->vcs_username, $this->uid, $repo_id ); - watchdog('special', - 'Version Control API: updated @username account in repository @repository', - array('@username' => $this->vcs_username, '@repository' => $this->repository->name), - WATCHDOG_NOTICE, l('view', 'admin/project/versioncontrol-accounts') - ); - } + db_query("UPDATE {versioncontrol_operations} + SET uid = 0 + WHERE uid = %d AND repo_id = %d", + $this->uid, $repo_id); + // not using data field for now, but backends can + db_query("UPDATE {versioncontrol_operations} + SET uid = %d + WHERE committer = '%s' AND repo_id = %d", + $this->uid, $this->vcs_username, $repo_id); - /** - * Let child backend account classes update information. - */ - protected function _update($additional_data) { + // Let the backend take action. + $this->backendUpdate($options); + + // Everything's done, invoke the hook. + module_invoke_all('versioncontrol_entity_account_update', $this); + return $this; } /** * Insert a VCS user account into the database, * and call the necessary module hooks. * - * @param $additional_data - * An array of additional author information. Modules can fill this array - * by implementing hook_versioncontrol_account_submit(). - * - * FIXME function sig & logic incompatibilities with VersioncontrolEntity + * FIXME use same logic as in other classes, this probably would + * be changed only when we get account_id PK schema change in. */ - public final function insert($additional_data = array()) { + public final function insert($options = array()) { + // not using data field for now, but backends can db_query( "INSERT INTO {versioncontrol_accounts} (uid, repo_id, vcs_username) VALUES (%d, %d, '%s')", $this->uid, $this->repository->repo_id, $this->vcs_username ); // Provide an opportunity for the backend to add its own stuff. - $this->_insert($additional_data); + $this->backendInsert($options); // Update the operations table. // FIXME differentiate author and commiter @@ -157,29 +131,19 @@ abstract class VersioncontrolAccount extends VersioncontrolEntity { WHERE author = '%s' AND repo_id = %d", $this->uid, $this->vcs_username, $this->repository->repo_id); - // Everything's done, let the world know about it! - module_invoke_all('versioncontrol_account', - 'insert', $this->uid, $this->vcs_username, $this->repository, $additional_data - ); - - watchdog('special', - 'Version Control API: added @vcs_username account in repository @repository', - array('@vcs_username' => $this->vcs_username, '@repository' => $this->repository->name), - WATCHDOG_NOTICE, l('view', 'admin/project/versioncontrol-accounts') - ); - } - - /** - * Let child backend account classes add information - */ - protected function _insert($additional_data) { + // Everything's done, invoke the hook. + module_invoke_all('versioncontrol_entity_account_insert', $this); + return $this; } /** * Delete a VCS user account from the database, set all commits with this * account as author to user 0 (anonymous), and call the necessary hooks. + * + * FIXME use same logic as in other classes, this probably would + * be changed only when we get account_id PK schema change in. */ - public final function delete() { + public final function delete($options = array()) { // Update the operations table. db_query('UPDATE {versioncontrol_operations} SET uid = 0 @@ -187,30 +151,14 @@ abstract class VersioncontrolAccount extends VersioncontrolEntity { $this->uid, $this->repository->repo_id); // Announce deletion of the account before anything has happened. - module_invoke_all('versioncontrol_account', - 'delete', $this->uid, $this->vcs_username, $this->repository, array() - ); + module_invoke_all('versioncontrol_entity_account_delete', $this); // Provide an opportunity for the backend to delete its own stuff. - $this->_delete(); + $this->backendDelete($options); db_query('DELETE FROM {versioncontrol_accounts} WHERE uid = %d AND repo_id = %d', $this->uid, $this->repository->repo_id); - - watchdog('special', - 'Version Control API: deleted @username account in repository @repository', - array('@username' => $this->vcs_username, '@repository' => $this->repository->name), - WATCHDOG_NOTICE, l('view', 'admin/project/versioncontrol-accounts') - ); - } - - /** - * Let child backend account classes delete information. - */ - protected function _delete() { } - public function save() {} - public function buildSave(&$query) {} } diff --git includes/VersioncontrolBranch.php includes/VersioncontrolBranch.php index 968b8a3..94d4ef9 100644 --- includes/VersioncontrolBranch.php +++ includes/VersioncontrolBranch.php @@ -79,7 +79,7 @@ class VersioncontrolBranch extends VersioncontrolEntity { drupal_write_record('versioncontrol_labels', $this, 'label_id'); // Let the backend take action. - $this->backendUpdate(); + $this->backendUpdate($options); // Everything's done, invoke the hook. module_invoke_all('versioncontrol_entity_branch_update', $this); @@ -97,7 +97,7 @@ class VersioncontrolBranch extends VersioncontrolEntity { drupal_write_record('versioncontrol_labels', $this); - $this->backendInsert(); + $this->backendInsert($options); // Everything's done, invoke the hook. module_invoke_all('versioncontrol_entity_branch_insert', $this); @@ -123,7 +123,7 @@ class VersioncontrolBranch extends VersioncontrolEntity { ->condition('label_id', $this->label_id) ->execute(); - $this->backendDelete(); + $this->backendDelete($options); module_invoke_all('versioncontrol_entity_branch_delete', $this); } diff --git includes/VersioncontrolItem.php includes/VersioncontrolItem.php index e309d18..2cca313 100644 --- includes/VersioncontrolItem.php +++ includes/VersioncontrolItem.php @@ -713,7 +713,7 @@ abstract class VersioncontrolItem extends VersioncontrolEntity { drupal_write_record('versioncontrol_item_revisions', $this, 'item_revision_id'); // Let the backend take action. - $this->backendUpdate(); + $this->backendUpdate($options); // Everything's done, invoke the hook. module_invoke_all('versioncontrol_entity_item_update', $this); @@ -735,7 +735,7 @@ abstract class VersioncontrolItem extends VersioncontrolEntity { drupal_write_record('versioncontrol_item_revisions', $this); - $this->backendInsert(); + $this->backendInsert($options); // Everything's done, invoke the hook. module_invoke_all('versioncontrol_entity_item_insert', $this); @@ -765,7 +765,7 @@ abstract class VersioncontrolItem extends VersioncontrolEntity { ->condition('item_revision_id', $this->item_revision_id) ->execute(); - $this->backendDelete(); + $this->backendDelete($options); module_invoke_all('versioncontrol_entity_item_delete', $this); } diff --git includes/VersioncontrolOperation.php includes/VersioncontrolOperation.php index 6396810..c514de9 100644 --- includes/VersioncontrolOperation.php +++ includes/VersioncontrolOperation.php @@ -129,7 +129,7 @@ abstract class VersioncontrolOperation extends VersioncontrolEntity { public function loadItemRevisions($ids = array(), $conditions = array(), $options = array()) { $conditions['repo_id'] = $this->repo_id; $conditions['vc_op_id'] = $this->vc_op_id; - return $items; + return $this->backend->loadEntities('item', $ids, $conditions, $options); } public function insert($options = array()) { @@ -147,7 +147,7 @@ abstract class VersioncontrolOperation extends VersioncontrolEntity { $this->insertNested($options); } - $this->backendInsert(); + $this->backendInsert($options); // Everything's done, invoke the hook. module_invoke_all('versioncontrol_entity_commit_insert', $this); @@ -175,7 +175,7 @@ abstract class VersioncontrolOperation extends VersioncontrolEntity { $this->updateNested($options); } - $this->backendUpdate(); + $this->backendUpdate($options); // Everything's done, invoke the hook. module_invoke_all('versioncontrol_entity_commit_update', $this); diff --git includes/VersioncontrolRepository.php includes/VersioncontrolRepository.php index f5f6c8f..76c1776 100644 --- includes/VersioncontrolRepository.php +++ includes/VersioncontrolRepository.php @@ -243,15 +243,13 @@ abstract class VersioncontrolRepository implements VersioncontrolEntityInterface drupal_write_record('versioncontrol_repositories', $this, 'repo_id'); - $this->backendUpdate(); + $this->backendUpdate($options); // Everything's done, let the world know about it! module_invoke_all('versioncontrol_repository_entity_update', $this); return $this; } - protected function backendUpdate($options = array()) {} - /** * Insert a repository into the database, and call the necessary hooks. * @@ -270,7 +268,7 @@ abstract class VersioncontrolRepository implements VersioncontrolEntityInterface // drupal_write_record() will fill the $repo_id property on $this. drupal_write_record('versioncontrol_repositories', $this); - $this->backendInsert(); + $this->backendInsert($options); // Everything's done, let the world know about it! module_invoke_all('versioncontrol_repository_entity_insert', $this); @@ -278,13 +276,6 @@ abstract class VersioncontrolRepository implements VersioncontrolEntityInterface } /** - * Let child backend repo classes add information that _is not_ in - * VersioncontrolRepository::data without modifying general flow if - * necessary. - */ - protected function backendInsert($options = array()) {} - - /** * Delete a repository from the database, and call the necessary hooks. * Together with the repository, all associated commits and accounts are * deleted as well. @@ -314,13 +305,11 @@ abstract class VersioncontrolRepository implements VersioncontrolEntityInterface ->condition('repo_id', $this->repo_id) ->execute(); - $this->backendDelete(); + $this->backendDelete($options); module_invoke_all('versioncontrol_entity_repository_delete', $this); } - protected function backendDelete($options = array()) {} - /** * Export a repository's authenticated accounts to the version control system's * password file format. diff --git includes/VersioncontrolTag.php includes/VersioncontrolTag.php index 70a4b1c..9d02a43 100644 --- includes/VersioncontrolTag.php +++ includes/VersioncontrolTag.php @@ -61,7 +61,7 @@ class VersioncontrolTag extends VersioncontrolEntity { drupal_write_record('versioncontrol_labels', $this, 'label_id'); // Let the backend take action. - $this->backendUpdate(); + $this->backendUpdate($options); // Everything's done, invoke the hook. module_invoke_all('versioncontrol_entity_tag_update', $this); @@ -79,7 +79,7 @@ class VersioncontrolTag extends VersioncontrolEntity { drupal_write_record('versioncontrol_labels', $this); - $this->backendInsert(); + $this->backendInsert($options); // Everything's done, invoke the hook. module_invoke_all('versioncontrol_entity_tag_insert', $this); @@ -98,7 +98,7 @@ class VersioncontrolTag extends VersioncontrolEntity { ->condition('label_id', $this->label_id) ->execute(); - $this->backendDelete(); + $this->backendDelete($options); module_invoke_all('versioncontrol_entity_tag_delete', $this); } diff --git includes/controllers.inc includes/controllers.inc index a21719f..7eacab4 100644 --- includes/controllers.inc +++ includes/controllers.inc @@ -571,26 +571,23 @@ class VersioncontrolItemController extends VersioncontrolEntityController { interface VersioncontrolEntityInterface extends ArrayAccess { public function build($args = array()); - public function save(); + public function save($options = array()); /** * Insert a new entity into the database, then invoke relevant hooks, if any. */ public function insert($options = array()); - protected function backendInsert($options = array()); /** * Update an existing entity's record in the database, then invoke relevant * hooks, if any. */ public function update($options = array()); - protected function backendUpdate($options = array()); /** * Delete an entity from the database, along with any of its dependent data. */ public function delete($options = array()); - protected function backendDelete($options = array()); } /** @@ -684,7 +681,7 @@ abstract class VersioncontrolEntity implements VersioncontrolEntityInterface { * Overridden by backends as needed to decorate the new entity insertion * process. */ - protected function backendInsert($options = array()); + protected function backendInsert($options = array()) {} /** * Default, empty implementation of backendUpdate(). diff --git versioncontrol.pages.inc versioncontrol.pages.inc index 1942cf1..6f8b0b6 100644 --- versioncontrol.pages.inc +++ versioncontrol.pages.inc @@ -509,7 +509,7 @@ function versioncontrol_account_edit_form_validate($form, &$form_state) { function versioncontrol_account_edit_form_submit($form, &$form_state) { // Reconstruct the user data from the $form_state that was passed. $uid = $form['#uid']; - $username = $form_state['values']['account_name']; + $vcs_username = $form_state['values']['account_name']; $repository = $form['#repository']; $vcs_name = $form['#vcs_name']; @@ -522,15 +522,16 @@ function versioncontrol_account_edit_form_submit($form, &$form_state) { if (empty($form['#original_username'])) { $data = array( - 'vcs_username' => $username, + 'vcs_username' => $vcs_username, 'uid' => $uid, 'repository' => $repository, ); $vcs_account = $repository->backend->buildEntity('account', $data); - $vcs_account->insert($additional_data); + $vcs_account->additional_data = $additional_data; + $vcs_account->insert(); $message = drupal_set_message(t( - 'The @vcs account %username has been registered.', - array('@vcs' => $vcs_name, '%username' => $username) + 'The @vcs account %vcs_username has been registered.', + array('@vcs' => $vcs_name, '%vcs_username' => $vcs_username) )); } else { @@ -544,14 +545,16 @@ function versioncontrol_account_edit_form_submit($form, &$form_state) { // if we got an original_username we can trust on getting a value $vcs_account = array_shift($vcs_accounts); - $vcs_account->update($username, $additional_data); + $vcs_account->vcs_username = $vcs_username; + $vcs_account->additional_data = $additional_data; + $vcs_account->update(); // Regular users cannot change the username, and will probably get // a message for each of the other actions that hook into the form. if (versioncontrol_admin_access()) { $message = drupal_set_message(t( 'The @vcs account %username has been updated successfully.', - array('@vcs' => $vcs_name, '%username' => $username) + array('@vcs' => $vcs_name, '%username' => $vcs_username) )); } } @@ -559,5 +562,5 @@ function versioncontrol_account_edit_form_submit($form, &$form_state) { $form_state['redirect'] = versioncontrol_admin_access() ? 'admin/project/versioncontrol-accounts' : 'user/'. $uid .'/edit/versioncontrol/'. $repository->repo_id - .'/'. drupal_urlencode($username); + .'/'. drupal_urlencode($vcs_username); } diff --git versioncontrol_account_status/versioncontrol_account_status.module versioncontrol_account_status/versioncontrol_account_status.module index d5aebad..7f67107 100644 --- versioncontrol_account_status/versioncontrol_account_status.module +++ versioncontrol_account_status/versioncontrol_account_status.module @@ -546,27 +546,40 @@ function versioncontrol_account_status_versioncontrol_repository_submit(&$reposi } /** - * Implementation of hook_versioncontrol_repository(): - * Act on database changes when VCS repositories are inserted, - * updated or deleted. + * Implementation of hook_versioncontrol_repository_entity_insert(). */ -function versioncontrol_account_status_versioncontrol_repository($op, $repository) { - if (!isset($repository['versioncontrol_account_status'])) { +function versioncontrol_account_status_versioncontrol_repository_entity_insert($repository) { + if (!isset($repository->versioncontrol_account_status)) { return; } $strings = $repository->versioncontrol_account_status['strings']; - switch ($op) { - case 'update': - case 'insert': - _versioncontrol_account_status_set_strings($repository->repo_id, $strings); - break; + _versioncontrol_account_status_set_strings($repository->repo_id, $strings); +} - case 'delete': - db_query('DELETE FROM {versioncontrol_account_status_strings} - WHERE repo_id = %d', $repository['repo_id']); - break; +/** + * Implementation of hook_versioncontrol_repository_entity_update(). + */ +function versioncontrol_account_status_versioncontrol_repository_entity_update($repository) { + if (!isset($repository->versioncontrol_account_status)) { + return; + } + $strings = $repository->versioncontrol_account_status['strings']; + + _versioncontrol_account_status_set_strings($repository->repo_id, $strings); +} + +/** + * Implementation of hook_versioncontrol_repository_entity_delete(). + */ +function versioncontrol_account_status_versioncontrol_repository_entity_delete($repository) { + if (!isset($repository->versioncontrol_account_status)) { + return; } + $strings = $repository->versioncontrol_account_status['strings']; + + db_query('DELETE FROM {versioncontrol_account_status_strings} + WHERE repo_id = %d', $repository->repo_id); } function _versioncontrol_account_status_get_value_names() { @@ -905,61 +918,78 @@ function versioncontrol_account_status_versioncontrol_account_submit(&$additiona } /** - * Implementation of hook_versioncontrol_account(): - * Insert the account status for accounts that are being inserted or updated, - * and delete the account status for those that are being deleted. + * Implementation of hook_versioncontrol_entity_account_insert(). */ -function versioncontrol_account_status_versioncontrol_account($op, $uid, $username, $repository, $additional_data = array()) { - switch ($op) { - case 'insert': - case 'update': - // Recap: if form_alter() wasn't applied, our array element is not set. - $additional_data = isset($additional_data['versioncontrol_account_status']) - ? $additional_data['versioncontrol_account_status'] - : NULL; - - if (!isset($additional_data)) { - // Auto-approve the account if it was inserted programmatically and - // the caller doesn't know anything about this module. - if ($op == 'insert') { - $additional_data = array( - 'status_data' => array( - 'uid' => $uid, - 'repo_id' => $repository['repo_id'], - 'motivation' => 'Inserted programmatically.', - 'status' => VERSIONCONTROL_ACCOUNT_STATUS_APPROVED, - ), - 'admin_message' => array('is_send' => FALSE, 'contents' => ''), - 'send_application_mails' => FALSE, - ); - } - // Don't change the status for programmatical updates, though. - if ($op == 'update') { - break; - } - } +function versioncontrol_account_status_versioncontrol_entity_account_insert($vcs_account) { + // Recap: if form_alter() wasn't applied, our array element is not set. + $additional_data = isset($vcs_account->additional_data['versioncontrol_account_status']) + ? $vcs_account->additional_data['versioncontrol_account_status'] + : NULL; + + if (!isset($additional_data)) { + // Auto-approve the account if it was inserted programmatically and + // the caller doesn't know anything about this module. + $additional_data = array( + 'status_data' => array( + 'uid' => $vcs_account->uid, + 'repo_id' => $vcs_account->repository->repo_id, + 'motivation' => 'Inserted programmatically.', + 'status' => VERSIONCONTROL_ACCOUNT_STATUS_APPROVED, + ), + 'admin_message' => array('is_send' => FALSE, 'contents' => ''), + 'send_application_mails' => FALSE, + ); + } - $status_data = $additional_data['status_data']; - versioncontrol_account_status_set($status_data); + $status_data = $additional_data['status_data']; + versioncontrol_account_status_set($status_data); - if ($additional_data['admin_message']['is_send']) { - $contents = $additional_data['admin_message']['contents']; - _versioncontrol_account_status_send_status_update_mails($status_data, $contents); - } - if ($additional_data['send_application_mails']) { - $is_reapplication = !empty($additional_data['is_reapplication']); - _versioncontrol_account_status_send_application_mails($status_data, $is_reapplication); - } - break; + if ($additional_data['admin_message']['is_send']) { + $contents = $additional_data['admin_message']['contents']; + _versioncontrol_account_status_send_status_update_mails($status_data, $contents); + } + if ($additional_data['send_application_mails']) { + $is_reapplication = !empty($additional_data['is_reapplication']); + _versioncontrol_account_status_send_application_mails($status_data, $is_reapplication); + } +} - case 'delete': - db_query('DELETE FROM {versioncontrol_account_status_users} - WHERE uid = %d', $uid); - break; +/** + * Implementation of hook_versioncontrol_entity_account_update(). + */ +function versioncontrol_account_status_versioncontrol_entity_account_update($vcs_account) { + // Recap: if form_alter() wasn't applied, our array element is not set. + $additional_data = isset($vcs_account->additional_data['versioncontrol_account_status']) + ? $vcs_account->additional_data['versioncontrol_account_status'] + : NULL; + + if (!isset($additional_data)) { + // Don't change the status for programmatical updates, though. + return; + } + + $status_data = $additional_data['status_data']; + versioncontrol_account_status_set($status_data); + + if ($additional_data['admin_message']['is_send']) { + $contents = $additional_data['admin_message']['contents']; + _versioncontrol_account_status_send_status_update_mails($status_data, $contents); + } + if ($additional_data['send_application_mails']) { + $is_reapplication = !empty($additional_data['is_reapplication']); + _versioncontrol_account_status_send_application_mails($status_data, $is_reapplication); } } /** + * Implementation of hook_versioncontrol_entity_account_delete(). + */ +function versioncontrol_account_status_versioncontrol_entity_account_delete($vcs_account) { + db_query('DELETE FROM {versioncontrol_account_status_users} + WHERE uid = %d', $vcs_account->uid); +} + +/** * Send out notification mails in response to an application submission. */ function _versioncontrol_account_status_send_application_mails($status_data, $is_reapplication) { diff --git versioncontrol_fakevcs/includes/classes.inc versioncontrol_fakevcs/includes/classes.inc index 1ef4e5b..4a2af5f 100644 --- versioncontrol_fakevcs/includes/classes.inc +++ versioncontrol_fakevcs/includes/classes.inc @@ -1,5 +1,6 @@