Permalink
Browse files

class constants and static members

Starting with sdboyer comment:
"Things like the static $successor_action_priority array in
VersioncontrolItem::getItemHistory() should be declared as class constants."

I found and review some of the static stuff in all vc api and now it's coherent.

BTW I found in some places cache is not used correctly(initialize before use),
that will be another commit on stable branch.
  • Loading branch information...
1 parent a366634 commit a6d445519e2605e53f3c7fc39a979008b99a2206 @marvil07 committed Jul 10, 2009
Showing with 51 additions and 36 deletions.
  1. +0 −1 OOP_TODO
  2. +16 −12 includes/VersioncontrolItem.php
  3. +27 −16 includes/VersioncontrolOperation.php
  4. +3 −3 includes/VersioncontrolRepository.php
  5. +5 −4 versioncontrol.module
View
@@ -3,7 +3,6 @@ on code
- tests for Version Control core
- follow sdboyer suggestions (http://groups.drupal.org/node/23015#comment-79517)
- - Things like the static $successor_action_priority array in VersioncontrolItem::getItemHistory() should be declared as class constants.
- I don't think including the @access tag is necessary by drupal coding standards. If it helps your doxygen generation, tho, all good.
- There are prolly other ones, but one coding convention we have is to get rid of the prepended underscore on function names when they become methods, and just make em protected. f.e. VersioncontrolItem::_badItemWarning.
- I would have figured, based on last week's conversation, that it's not just VersioncontrolBackend that's abstract, but also VersioncontrolOperation, VersioncontrolItem, VersioncontrolRepository, etc; that these, too, will be extended by the backends as needed. Although that doesn't require them to be abstract, of course - just that they hold all the shared code that's common to all the vcses.
@@ -104,6 +104,20 @@ class VersioncontrolItem implements ArrayAccess {
public $selected_label;
public $commit_operation;
+ /**
+ * FIXME: ?
+ */
+ private static $successor_action_priority = array(
+ VERSIONCONTROL_ACTION_MOVED => 10,
+ VERSIONCONTROL_ACTION_MODIFIED => 10,
+ VERSIONCONTROL_ACTION_COPIED => 8,
+ VERSIONCONTROL_ACTION_MERGED => 9,
+ VERSIONCONTROL_ACTION_OTHER => 1,
+ VERSIONCONTROL_ACTION_DELETED => 1,
+ VERSIONCONTROL_ACTION_ADDED => 0, // does not happen, guard nonetheless
+ VERSIONCONTROL_ACTION_REPLACED => 0, // does not happen, guard nonetheless
+ );
+
// Associations
// Operations
/**
@@ -311,16 +325,6 @@ public function getItemHistory($repository, &$item, $successor_item_limit = NULL
// Find (recursively) all successor items within the successor item limit.
$history_successor_items = array();
$source_item = $item;
- static $successor_action_priority = array(
- VERSIONCONTROL_ACTION_MOVED => 10,
- VERSIONCONTROL_ACTION_MODIFIED => 10,
- VERSIONCONTROL_ACTION_COPIED => 8,
- VERSIONCONTROL_ACTION_MERGED => 9,
- VERSIONCONTROL_ACTION_OTHER => 1,
- VERSIONCONTROL_ACTION_DELETED => 1,
- VERSIONCONTROL_ACTION_ADDED => 0, // does not happen, guard nonetheless
- VERSIONCONTROL_ACTION_REPLACED => 0, // does not happen, guard nonetheless
- );
while ((!isset($successor_item_limit) || ($successor_item_limit > 0))) {
$source_items = array($source_item['path'] => $source_item);
@@ -337,9 +341,9 @@ public function getItemHistory($repository, &$item, $successor_item_limit = NULL
$highest_priority_so_far = 0;
foreach ($source_item['successor_items'] as $path => $succ_item) {
if (!isset($successor_item)
- || $successor_action_priority[$succ_item['action']] > $highest_priority_so_far) {
+ || self::$successor_action_priority[$succ_item['action']] > $highest_priority_so_far) {
$successor_item = $succ_item;
- $highest_priority_so_far = $successor_action_priority[$succ_item['action']];
+ $highest_priority_so_far = self::$successor_action_priority[$succ_item['action']];
}
}
$history_successor_items[$successor_item['revision']] = $successor_item;
@@ -131,6 +131,21 @@ class VersioncontrolOperation implements ArrayAccess {
*/
public $labels;
+ /**
+ * All possible operation constraints.
+ * Each constraint is identified by its key which denotes the array key within
+ * the $constraints parameter that is given to self::getOperations().
+ * The array value of each element is a description array containing the
+ * elements 'callback' and 'cardinality'.
+ *
+ */
+ private static $constraint_info = array();
+
+ /**
+ * FIXME: ?
+ */
+ private static $error_messages = array();
+
/**
* Constructor
*/
@@ -812,25 +827,23 @@ private static function _constructQuery(&$constraints, &$tables) {
* @static
*/
private static function _constraintInfo() {
- static $constraint_info = array();
-
- if (empty($constraint_info)) {
+ if (empty(self::$constraint_info)) {
foreach (module_implements('versioncontrol_operation_constraint_info') as $module) {
$function = $module .'_versioncontrol_operation_constraint_info';
$constraints = $function();
foreach ($constraints as $key => $info) {
- $constraint_info[$key] = $info;
+ self::$constraint_info[$key] = $info;
if (!isset($info['callback'])) {
- $constraint_info[$key]['callback'] = $module .'_operation_constraint_'. $key;
+ self::$constraint_info[$key]['callback'] = $module .'_operation_constraint_'. $key;
}
if (!isset($info['cardinality'])) {
- $constraint_info[$key]['cardinality'] = VERSIONCONTROL_CONSTRAINT_MULTIPLE;
+ self::$constraint_info[$key]['cardinality'] = VERSIONCONTROL_CONSTRAINT_MULTIPLE;
}
}
}
}
- return $constraint_info;
+ return self::$constraint_info;
}
/**
@@ -876,12 +889,10 @@ private function _fill($include_unauthorized = FALSE) {
* @access private
*/
private function _accessErrors($new_messages = NULL) {
- static $error_messages = array();
-
if (isset($new_messages)) {
- $error_messages = $new_messages;
+ self::$error_messages = $new_messages;
}
- return $error_messages;
+ return self::$error_messages;
}
/**
@@ -932,7 +943,7 @@ private function _insert_operation_item($item, $type) {
* @access protected
*/
protected function getAccessErrors() {
- return _versioncontrol_access_errors();
+ return $this->_accessErrors();
}
@@ -1017,7 +1028,7 @@ protected function hasWriteAccess($operation, $operation_items) {
$type = t('tag');
break;
}
- _versioncontrol_access_errors(array(t(
+ $this->_accessErrors(array(t(
'** ERROR: Version Control API cannot determine a repository
** for the !commit-branch-or-tag information given by the VCS backend.',
array('!commit-branch-or-tag' => $type)
@@ -1031,7 +1042,7 @@ protected function hasWriteAccess($operation, $operation_items) {
if (!$repo_data['allow_unauthorized_access']) {
if (!versioncontrol_is_account_authorized($operation['repository'], $operation['uid'])) {
- _versioncontrol_access_errors(array(t(
+ $this->_accessErrors(array(t(
'** ERROR: !user does not have commit access to this repository.',
array('!user' => $operation['username'])
)));
@@ -1041,7 +1052,7 @@ protected function hasWriteAccess($operation, $operation_items) {
// Don't let people do empty log messages, that's as evil as it gets.
if (isset($operation['message']) && empty($operation['message'])) {
- _versioncontrol_access_errors(array(
+ $this->_accessErrors(array(
t('** ERROR: You have to provide a log message.'),
));
return FALSE;
@@ -1066,7 +1077,7 @@ protected function hasWriteAccess($operation, $operation_items) {
// Let the operation fail if there's more than zero error messages.
if (!empty($error_messages)) {
- _versioncontrol_access_errors($error_messages);
+ $this->_accessErrors($error_messages);
return FALSE;
}
return TRUE;
@@ -235,8 +235,8 @@ public static function getRepositories($constraints = array()) {
}
$constraints_serialized = serialize($constraints);
- if (isset($repository_cache[$constraints_serialized])) {
- return $repository_cache[$constraints_serialized];
+ if (isset(self::$repository_cache[$constraints_serialized])) {
+ return self::$repository_cache[$constraints_serialized];
}
list($and_constraints, $params) =
@@ -283,7 +283,7 @@ public static function getRepositories($constraints = array()) {
}
}
- $repository_cache[$constraints_serialized] = $result_repositories; // cache the results
+ self::$repository_cache[$constraints_serialized] = $result_repositories; // cache the results
return $result_repositories;
}
View
@@ -471,9 +471,10 @@ function versioncontrol_get_backends() {
/**
* Convenience function, retrieving the backend information array for a
* single repository. So, the result is one of the elements in the result array
- * of versioncontrol_get_backends(). As versioncontrol_get_repositories() only
- * returns repositories for backends that actually exist, this function can be
- * trusted to always return a valid backend array.
+ * of versioncontrol_get_backends(). As
+ * VersioncontrolRepository::getRepositories() only returns repositories for
+ * backends that actually exist, this function can be trusted to always return
+ * a valid backend array.
*/
function versioncontrol_get_backend($repository) {
$backends = versioncontrol_get_backends();
@@ -1470,7 +1471,7 @@ function versioncontrol_get_url_tracker($repository, $issue_id) {
* - 'tracker': The issue/bug/case URL of the associated issue tracker.
*/
function _versioncontrol_get_repository_urls($repository) {
- static $urls_by_repository = array();
+ static $urls_by_repository;
if (!isset($urls_by_repository[$repository->repo_id])) {
$result = db_query('SELECT * FROM {versioncontrol_repository_urls}

0 comments on commit a6d4455

Please sign in to comment.