[TYPO3-core] RFC: #16315: New extension manager part 1: splitting classes

[Xavier Perseguers]

Hi Steffen,

Here's my review by reading for the first ~ 2200 lines. Both of you made 
a crazy job!

+	/**
+	 * Get extension list from cache_extensions
+	 *
+	 * @param  $repository

^^^^ missing type

+	 * @param string $andWhere
+	 * @param string $orderBy
+	 * @param string $orderDir
+	 * @param string $limit
+	 * @return array
+	 */
+	public function getExtensionListFromRepository($repository, $andWhere 
= '', $orderBy = '', $orderDir = 'ASC', $limit = '') {
+		$order = $orderBy ? $orderBy . ' ' . $orderDir : '';
+		$ret = array();
+
+		$temp = $GLOBALS['TYPO3_DB']->exec_SELECTgetRows(
+			'count(*) count',
+			'cache_extensions',
+			'repository=' . intval($repository) . $andWhere,
+			'extkey'
+		);
+		$ret['count'] = count($temp);

^^^^ It's a pity exec_SELECTcountRows cannot take a "group by" argument

+		$ret['results'] = $GLOBALS['TYPO3_DB']->exec_SELECTgetRows(
+			'*, count(*) versions, max(intversion) maxintversion',
+			'cache_extensions',
+			'repository=' . intval($repository) . $andWhere,
+			'extkey',
+			$order,
+			$limit
+		);
+		return $ret;
+	}
+

For aliases in field names, we don't have CGL but personally I prefer 
having the explicit keyword "AS"

+	/**
+	 * Get versions of extension
+	 *
+	 * @param int $repository
+	 * @param string $extKey
+	 * @retuen array $versions
+	 */
+	public function getExtensionVersionsFromRepository($repository, $extKey) {

@retuen instead of @return

+	/**
+	 * Function inserts a repository object into database.
+	 *
+	 * @access  public
+	 * @param   tx_em_Repository $repository  repository object
+	 * @return  void
+	 */
+	public function updateRepository(tx_em_Repository $repository) {

Strange description! Copy paste from next:

+	/**
+	 * Function inserts a repository object into database.
+	 *
+	 * @access  public
+	 * @param   tx_em_Repository $repository  repository object
+	 * @return  integer  UID of the newly inserted repository object
+	 */
+	public function insertRepository(tx_em_Repository $repository) {

At least for these 2 methods, you start with "Function <some verb>", 
why? Later you have multiple times "Method <some verb>"... I'd prefer 
having simply the description starts with the verb in the 3rd person 
each time.

+	function dumpTableHeader($table, $fieldKeyInfo, $dropTableIfExists = 0) {
+		$lines = array();
+		$dump = '';
+
+		// Create field definitions
+		if (is_array($fieldKeyInfo['fields'])) {
+			foreach ($fieldKeyInfo['fields'] as $fieldN => $data) {
+				$lines[] = '  ' . $fieldN . ' ' . $data;
+			}
+		}
+
+		// Create index key definitions
+		if (is_array($fieldKeyInfo['keys'])) {
+			foreach ($fieldKeyInfo['keys'] as $fieldN => $data) {
+				$lines[] = '  ' . $data;
+			}
+		}

I know it was borrowed, but I don't like having "$fieldN" as key 
variable for the loop.


+	/**
+	 * Enter description here...
+	 *
+	 * @return unknown
+	 */
+	public function getInstalledExtkeys() {

Missing description

+	/**
+	 * Keeps sponsor link of currently processed mirror.
+	 *
+	 * @var  string
+	 */
+	protected $sponsorlink = NULL;
+
+	/**
+	 * Keeps sponsor logo location of currently processed mirror.
+	 *
+	 * @var  string
+	 */
+	protected $sponsorlogo = NULL;
+
+	/**
+	 * Keeps sponsor name of currently processed mirror.
+	 *
+	 * @var  string
+	 */
+	protected $sponsorname = NULL;

What about lower camel case?

+	/**
+	 * Returns an assoziative array of all mirror properties.
+	 *
+	 * Valid array keys of returned array are:
+	 * country, host, path, sponsorlink, sponsorlogo, sponsorname, title
+	 *
+	 * @access  public
+	 * @return  array  assoziative array of a mirror's properties
+	 * @see	 $country, $host, $path, $sponsorlink, $sponsorlogo, 
$sponsorname, $title
+	 */
+	public function getAll() {

The "z" of "assoziative" sounds really german to me ;-)

+/**
+ * Importer object for extension list
+ *
+ * @author	  Marcus Krause <marcus#exp2010 at t3sec.info>
+ * @author	  Steffen Kamper <info at sk-typo3.de>
+ *
+ * @since	   2010-02-10
+ * @package	 TYPO3
+ * @subpackage  EM
+ */
+class tx_em_Import_ExtensionListImporter implements SplObserver {
+

Shouldn't be @since a TYPO3 version instead? Anyway, the date is wrong.

+	/**
+	 * Keeps fieldnames of cache_extension table.
+	 *
+	 * @var  array
+	 */
+	static protected $fieldNames = array('extkey', 'version', 
'intversion', 'alldownloadcounter', 'downloadcounter', 'title', 
'ownerusername', 'authorname', 'authoremail', 'authorcompany', 
'lastuploaddate', 't3xfilemd5', 'repository', 'state', 'reviewstate', 
'category', 'description', 'dependencies', 'uploadcomment' /*, 
'lastversion', 'lastreviewedversion'*/);
+

Could this huge list be split and stored as multiple lines?

+	/**
+	 * Keeps indexes of fields that should not be quoted.
+	 *
+	 * @var  array
+	 */
+	static protected $fieldIndicesNoQuote = array(2, 3, 4, 10, 12, 13, 14, 
15);

This typically is a list of "magic numbers"...

What about using prepared queries?


Cheers
Xavier