diff --git a/typo3/sysext/em/classes/class.tx_em_extensionmanager.php b/typo3/sysext/em/classes/class.tx_em_extensionmanager.php index 4543e74..042ba55 100644 --- a/typo3/sysext/em/classes/class.tx_em_extensionmanager.php +++ b/typo3/sysext/em/classes/class.tx_em_extensionmanager.php @@ -261,6 +261,7 @@ class tx_em_ExtensionManager { foreach ($GLOBALS['TYPO3_CONF_VARS']['SC_OPTIONS']['em/classes/class.tx_em_extensionamager.php']['renderHook'] as $classRef) { $hookObject = t3lib_div::getUserObj($classRef); if(!($hookObject instanceof tx_em_renderHook)) { + // @review There is no such interface as tx_em_renderHook. So the hook can never be executed. throw new UnexpectedValueException('$hookObject must implement interface tx_em_renderHook', 1298121373); } $hookObject->render( diff --git a/typo3/sysext/em/classes/connection/class.tx_em_connection_extdirectserver.php b/typo3/sysext/em/classes/connection/class.tx_em_connection_extdirectserver.php index 75b8c97..772407e 100644 --- a/typo3/sysext/em/classes/connection/class.tx_em_connection_extdirectserver.php +++ b/typo3/sysext/em/classes/connection/class.tx_em_connection_extdirectserver.php @@ -90,6 +90,8 @@ class tx_em_Connection_ExtDirectServer { */ protected function getSettingsObject() { if (!is_object(self::$objSettings) && !(self::$objSettings instanceof tx_em_Settings)) { + // @review This should be a ||, not a && (or we just trust the correct type and do an !== NULL check here + // (and add NULL as default value for the static field). self::$objSettings = t3lib_div::makeInstance('tx_em_Settings'); } return self::$objSettings; @@ -105,6 +107,8 @@ class tx_em_Connection_ExtDirectServer { * Render local extension list * * @return string $content + * + * The return type is wrong - it actually should be array */ public function getExtensionList() { /** @var $list tx_em_Extensions_List */ @@ -140,6 +144,8 @@ class tx_em_Connection_ExtDirectServer { * Render extensionlist for languages * * @return unknown + * + * No, not unknown, but array. */ public function getInstalledExtkeys() { $list = $this->getExtensionList(); @@ -158,11 +164,15 @@ class tx_em_Connection_ExtDirectServer { ); $keys[$i]['lang'] = array(); if (count($selectedLanguages)) { + // @review You don't need this if because a foreach loop for an empty array + // is just a no-op. foreach ($selectedLanguages as $language) { $keys[$i]['lang'][] = $GLOBALS['LANG']->sL('LLL:EXT:setup/mod/locallang.xml:lang_' . $language); } } $i++; + // @review Is this intentional that non-installed extensions then leave gaps in the array + // keys of $keys? } } @@ -178,6 +188,8 @@ class tx_em_Connection_ExtDirectServer { * @return string $content */ public function getExtensionDetails() { + // @review The function name does not match the function content + // (which is the same as of the function getExtensionList). /** @var $list tx_em_Extensions_List */ $list = t3lib_div::makeInstance('tx_em_Extensions_List'); $extList = $list->getInstalledExtensions(TRUE); @@ -198,11 +210,13 @@ class tx_em_Connection_ExtDirectServer { */ public function getExtensionUpdate($extKey) { if (isset($GLOBALS['TYPO3_LOADED_EXT'][$extKey])) { + // @review This should be a call to t3lib_extMgm::isLoaded() instead. /** @var $install tx_em_Install */ $install = t3lib_div::makeInstance('tx_em_Install'); /** @var $extension tx_em_Extensions_List */ $extension = t3lib_div::makeInstance('tx_em_Extensions_List'); - + // @review The variable name is misleading: This is not an extension, but a utility class instance + // for managing/querying the list of extensions. $extPath = t3lib_extMgm::extPath($extKey); $type = tx_em_Tools::getExtTypeFromPath($extPath); @@ -211,15 +225,22 @@ class tx_em_Connection_ExtDirectServer { $extension->singleExtInfo($extKey, $typePath, $extInfo); $extInfo = $extInfo[0]; + // @review Please use two separate variables for this instead of overwriting $extInfo. $extInfo['type'] = $extInfo['typeShort']; $update = $install->checkDBupdates($extKey, $extInfo); if ($update) { $update = $GLOBALS['LANG']->getLL('ext_details_new_tables_fields_select') . + // @review $update serves two completely different purposes. Please + // use separate variables for this. $update . '
'; + // @review You can just use the second parameter of getLL to hsc the return value. + // @review However, then you should do this on each and every call to getLL instead of only on "selected, very special places". } return $update ? $update : $GLOBALS['LANG']->sL('LLL:EXT:em/language/locallang.xml:ext_details_dbUpToDate'); + // @review This will break when the ext_details_new_tables_fields_select localllang string start with something that + // will cast to FALSE, e.g. a zero. } else { return sprintf($GLOBALS['LANG']->sL('LLL:EXT:em/language/locallang.xml:msg_extNotInstalled') ,htmlspecialchars($extKey)); } @@ -236,17 +257,22 @@ class tx_em_Connection_ExtDirectServer { /** @var $extensionList tx_em_Extensions_List */ $extensionList = t3lib_div::makeInstance('tx_em_Extensions_List', $this); list($list,) = $extensionList->getInstalledExtensions(); + // @review array_shift would be more readable. /** @var $install tx_em_Install */ $install = t3lib_div::makeInstance('tx_em_Install'); $install->setSilentMode(TRUE); $form = $install->updatesForm($extKey, $list[$extKey], 1, '', '', FALSE, TRUE); + // @review The third parameter of updatesForm is a boolean. So it should be TRUE, not 1. if (!$form) { + // @review Basic rule: Never use a boolean operator on a string. return '

' . $GLOBALS['LANG']->sL('LLL:EXT:em/language/locallang.xml:msg_extNoConfiguration') . '

'; } else { //$form = preg_replace('/]+)>/', '', $form); //$form = str_replace('', '', $form); + // @review Let's remove any deleted code. return $form; + // @review Switching the if/else would make this readable because it would remove the negation. } } @@ -254,7 +280,11 @@ class tx_em_Connection_ExtDirectServer { * Save extension configuration * * @formHandler + * @review Say what? @formHandler? * @param array $parameter + * @review Type-hint me ($parameter) + * @review Please rename and document $parameter (the name could mean anything). + * * @return array */ public function saveExtensionConfiguration($parameter) { @@ -262,31 +292,43 @@ class tx_em_Connection_ExtDirectServer { $extKey = $parameter['extkey']; $extType = $parameter['exttype']; $noSave = $parameter['noSave']; + // @review What does "noSave" mean? + // @review I propose to not use non-negated variable names in order to not + // to little decrease the readability of the code. $absPath = tx_em_Tools::getExtPath($extKey, $extType); + // @review Why not use t3lib_emtMgm::extPath here? $relPath = tx_em_Tools::typeRelPath($extType) . $extKey . '/'; + // @review Relative to what? Why doesn't the absolute path to the extension suffice? /** @var $extensionList tx_em_Extensions_List */ $extensionList = t3lib_div::makeInstance('tx_em_Extensions_List', $this); list($list,) = $extensionList->getInstalledExtensions(); - + // @review I've seen this not-so-readable piece of code before. Please + // refactor it to a nice little function and use it in all places where copies of this code lives. + // The way I read this code, $list is always used as $list[$extKey]. So a function for getting + // the configuration for a single extension would be the way to go. $arr = unserialize($GLOBALS['TYPO3_CONF_VARS']['EXT']['extConf'][$extKey]); $arr = is_array($arr) ? $arr : array(); + // @review Please rename "arr" so the name makes clear what the variable contains. /** @var $tsStyleConfig t3lib_tsStyleConfig */ $tsStyleConfig = t3lib_div::makeInstance('t3lib_tsStyleConfig'); $tsStyleConfig->doNotSortCategoriesBeforeMakingForm = TRUE; + // @review Please use getters and setters instead of public properties. $theConstants = $tsStyleConfig->ext_initTSstyleConfig( t3lib_div::getUrl($absPath . 'ext_conf_template.txt'), + // @review Please do not use getUrl to read a local file. $relPath, $absPath, $GLOBALS['BACK_PATH'] ); $tsStyleConfig->ext_procesInput($parameter, array(), $theConstants, array()); + // @review That class does not have function ext_procesInput (event without the misspelling). $arr = $tsStyleConfig->ext_mergeIncomingWithExisting($arr); - + // @review Please use two separate variables for the two different "arrays of things". /** @var $install tx_em_Install */ $install = t3lib_div::makeInstance('tx_em_Install'); @@ -297,8 +339,11 @@ class tx_em_Connection_ExtDirectServer { $html = ''; if ($noSave) { $html = $install->updatesForm($extKey, $list[$extKey], 1); + // @review The third parameter of updatesForm is a boolean. So it should be TRUE, not 1. } else { $install->writeTsStyleConfig($extKey, $arr); + // @review Why is the output ($html) empty when $noSave it false? + // Is this intentional? } return array( @@ -311,6 +356,8 @@ class tx_em_Connection_ExtDirectServer { /** * Cleans EMConf of extension * + * @review Please document whether this function also directly saves the configuration. + * * @param string $extKey * @return array */ @@ -354,6 +401,8 @@ class tx_em_Connection_ExtDirectServer { $success = TRUE; if ($res) { + // @review You need to do a !== FALSE here because $res might also be a string that casts to FALSE. + // You'll need to htmlspecialchars $res first because it can contain the extension key. $res = nl2br($res); $error = sprintf($GLOBALS['LANG']->getLL('extDelete_remove_dir_failed'), $absPath); $success = FALSE; @@ -371,20 +420,27 @@ class tx_em_Connection_ExtDirectServer { * * @param object $parameter * @return array + * + * @review Please specificy the exact class of $parameter, add type hinting + * and rename it. */ public function getExtFileTree($parameter) { $type = $parameter->typeShort; + // @review $type is never read and can be removed. $node = substr($parameter->node, 0, 6) !== 'xnode-' ? $parameter->node : $parameter->baseNode; $path = PATH_site . $node; + // @review Is $node guaranteed to be safe? $fileArray = array(); $dirs = t3lib_div::get_dirs($path); + // @review $dirs will be an error string in case of an error. You need to check for that case. $files = t3lib_div::getFilesInDir($path, '', FALSE, '', ''); if (!is_array($dirs) && !is_array($files)) { + // @review The check for !is_array($dirs) needs to happen before the getFilesInDir call. return array(); } @@ -401,11 +457,13 @@ class tx_em_Connection_ExtDirectServer { foreach ($files as $key => $file) { + // @review $key is never read and can be removed. $fileInfo = $this->getFileInfo($file); $fileArray[] = array( 'id' => $node . '/' . $file, 'text' => $fileInfo[0], + // @review Why is $dir (see above) htmlspecialchared and $fileInfo[0] isn't? 'leaf' => true, 'qtip' => $fileInfo[1], 'iconCls' => $fileInfo[4], @@ -421,23 +479,45 @@ class tx_em_Connection_ExtDirectServer { /** * Read extension file and send content * + * @review Actually, the function is not specific to *extension* files at all. + * So the name and the documentation in this regard are incorrect. + * + * @review Thsi function does not *send* the content; it just returns it. + * So either the documentation or the function content are incorrect. + * * @param string $path + * + * @review Please document that $path must be relative to PATH_site. + * * @return string file content */ public function readExtFile($path) { $path = PATH_site . $path; if (@file_exists($path)) { return t3lib_div::getURL($path); + // @review Please do not use getURL for reading local files. } return ''; + // @review This does not allow the caller to differentiate between empty files and non-readable files. + // I propose to use exceptions for error cases. } /** * Save extension file * + * @review The documentation is incorrect. This function does not just save a file - + * it writes $content to $file. + * + * @review Actually, the function is not specific to *extension* files at all. + * So the name and the documentation in this regard are incorrect. + * * @param string $file * @param string $content + * @review Please document that $path must be relative to PATH_site. + * * @return boolean success + * + * @review No, this function returns an array, not a boolean. */ public function saveExtFile($file, $content) { $path = PATH_site . $file; @@ -456,6 +536,7 @@ class tx_em_Connection_ExtDirectServer { : $GLOBALS['LANG']->sL('LLL:EXT:em/language/locallang.xml:msg_fileWriteProtected', TRUE) ) : $GLOBALS['LANG']->sL('LLL:EXT:em/language/locallang.xml:ext_details_saving_disabled', TRUE); + // @review Please don't use two ternary operators in one statement. } if ($success) { @@ -466,6 +547,8 @@ class tx_em_Connection_ExtDirectServer { 'path' => $path, 'file' => basename($path), 'content' => $content, + // @review Why return the content if it has been passed as a parameter anyway, + // i.e., the caller already has it? 'error' => $error ); } @@ -495,8 +578,11 @@ class tx_em_Connection_ExtDirectServer { $fileInfo = $this->getFileInfo($result[1]); $node = array( 'id' => substr($fileInfo[0], strlen(PATH_site)), + // @review Why are only some parts htmlspecialchard, e.g., + // only in the isFolder case? 'text' => basename($fileInfo[0]), 'leaf' => !$isFolder, + // @review Here, !$isFolder always evaluates to TRUE. 'qtip' => $fileInfo[1], 'iconCls' => $fileInfo[4], 'fileType' => $fileInfo[3], @@ -521,6 +607,7 @@ class tx_em_Connection_ExtDirectServer { * @return array result */ public function renameFile($file, $newName, $isFolder) { + // @review The parameter $isFolder is never read and can be removed. $src = basename($file); $newFile = substr($file, 0, -1 * strlen($src)) . $newName; @@ -531,6 +618,7 @@ class tx_em_Connection_ExtDirectServer { 'oldFile' => $file, 'newFile' => $newFile, 'newFilename' => basename($newFile), + // @review The names "newFile" and "newFileName" do not make the differences between the two very clear. ); } @@ -544,6 +632,7 @@ class tx_em_Connection_ExtDirectServer { * @return array */ public function moveFile($file, $destination, $isFolder) { + // @review This function is a no-op, i.e., it does *not* move a file. return array( 'success' => TRUE, 'file' => $file, @@ -560,15 +649,21 @@ class tx_em_Connection_ExtDirectServer { * @return array */ public function deleteFile($file, $isFolder) { + // @review $isFolder is not used (except in the return array). So it + // probably can be dropped. $file = str_replace('//', '/', PATH_site . $file); + // @review Why this replacement? Where does the "bad" data come from, and + // cannot we fix the broken caller instead? $command['delete'][] = array( 'data' => $file ); + // @review $command needs to be declared as an array first. $result = $this->fileOperation($command); return array( 'success' => TRUE, + // @review And what if the delete operation has failed? 'file' => $file, 'isFolder' => $isFolder, 'command' => $command, @@ -587,6 +682,7 @@ class tx_em_Connection_ExtDirectServer { $diff = t3lib_div::makeInstance('t3lib_diff'); $result = $diff->makeDiffDisplay($original, $content); //debug(array($original, $content, $result)); + // @review Please drop the commented-out debug line. return array( 'success' => TRUE, 'diff' => '
' . $result . '
' @@ -600,6 +696,7 @@ class tx_em_Connection_ExtDirectServer { * @return array */ public function loadUploadExtToTer() { + // @review Actually, this function does not load any form. $settings = $this->getSettings(); return array( 'success' => TRUE, @@ -616,6 +713,10 @@ class tx_em_Connection_ExtDirectServer { * @formHandler * * @param string $parameter + * + * @review Please add type hinting to all array function parameters in all functions. + * @review Please rename and properly document all $parameter parameters. + * * @return array */ public function uploadExtToTer($parameter) { @@ -626,6 +727,9 @@ class tx_em_Connection_ExtDirectServer { $parameter['user']['fe_p'] = $parameter['fe_p']; $parameter['upload']['mode'] = $parameter['newversion']; $parameter['upload']['comment'] = $parameter['uploadcomment']; + // @review You're using $parameter both as the function parameter as well as + // a non-documented variable container for all kinds of purposes. Please use + // separate, well-named variables instead. /** @var $extensionList tx_em_Extensions_List */ $extensionList = t3lib_div::makeInstance('tx_em_Extensions_List', $this); @@ -638,6 +742,7 @@ class tx_em_Connection_ExtDirectServer { $terConnection->wsdlURL = $wsdlURL; $parameter['extInfo'] = $list[$parameter['extKey']]; + // @review This should be a call to get the information for a single extension instead. $response = $terConnection->uploadToTER($parameter); if (!is_array($response)) { @@ -648,10 +753,12 @@ class tx_em_Connection_ExtDirectServer { ); } if ($response['resultCode'] == 10504) { //success + // @review Please use the constant instead of the magic number. $parameter['extInfo']['EM_CONF']['version'] = $response['version']; $response['resultMessages'][] = sprintf( $GLOBALS['LANG']->sL('LLL:EXT:em/language/locallang.xml:terCommunication_ext_version'), $response['version'] + // @review a hsc is missing here. ); $response['resultMessages'][] = $this->extensionDetails->updateLocalEM_CONF($parameter['extKey'], $parameter['extInfo']); } @@ -668,6 +775,8 @@ class tx_em_Connection_ExtDirectServer { * * @param string $parameter * @return string + * + * @review This function returns an array, not a string. */ public function getExtensionDevelopInfo($extKey) { /** @var $extensionList tx_em_Extensions_List*/ @@ -680,11 +789,13 @@ class tx_em_Connection_ExtDirectServer { } -/** + /** * Prints backupdelete * * @param string $parameter * @return string + * + * @review This function returns an array, not a string. */ public function getExtensionBackupDelete($extKey) { $content=''; @@ -770,6 +881,8 @@ class tx_em_Connection_ExtDirectServer { // mod menu for singleDetails + // @review Instead of having these comments before code blocks, I propose + // moving these code blocks to well-named, smaller functions. $modMenu = $GLOBALS['TBE_MODULES_EXT']['tools_em']['MOD_MENU']['singleDetails']; if (isset($modMenu) && is_array($modMenu)) { $lines[] = '' . @@ -784,7 +897,8 @@ class tx_em_Connection_ExtDirectServer { } $content = '' . implode('', $lines) . '
'; - + // @review If $lines is imploded without any glue anyway, it does not need to be an array, + // but can be a plain string. Or (for better HTML readability), you use LF as glue. return $this->replaceLinks($content); @@ -794,11 +908,17 @@ class tx_em_Connection_ExtDirectServer { /** * Execute update script * + * @review The function name (*get*...) and its actual working (*execute* the script) + * do not match. This is really misleading because one would expect this function + * to only return the script, but not actually execute anything. Please rename it. + * * @param $extkey * @return array */ public function getExtensionUpdateScript($extkey) { $updateScript = t3lib_extMgm::extPath($extkey) . 'class.ext_update.php'; + // @review There should be a file_exists before the require_once so the caller + // does not have to care about this. require_once($updateScript); $updateObj = new ext_update; $access = FALSE; @@ -820,7 +940,13 @@ class tx_em_Connection_ExtDirectServer { * Render remote extension list * * @param object $parameters + * + * @review Please type-hint and document the exact type of $parameters (and + * rename that variable). + * * @return string $content + * + * @review This function returns an array, not a string. */ public function getRemoteExtensionList($parameters) { $repositoryId = $parameters->repository; @@ -833,6 +959,7 @@ class tx_em_Connection_ExtDirectServer { $limit = htmlspecialchars($parameters->start . ', ' . $parameters->limit); $orderBy = htmlspecialchars($parameters->sort); $orderDir = htmlspecialchars($parameters->dir); + // @review You need fullQuoteStr, not htmlspecialchars for these variables. if ($orderBy == '') { $orderBy = 'relevance'; $orderDir = 'ASC'; @@ -844,17 +971,22 @@ class tx_em_Connection_ExtDirectServer { } else { $orderBy = 'cache_extensions.' . $orderBy . ' ' . $orderDir; } + // @review This if/if/elseif/if chain reads like it should rather be a switch/case. $installedOnly = $parameters->installedOnly; + // @review Please use a getter and make that field non-public. $where = $addFields = ''; + // @review Please do not put two assignments in one line. if ($search === '' && !$installedOnly) { return array( 'length' => 0, 'data' => array(), ); + // @review You can move this check far up in the function and make it a guard clause, + // thus getting rid of the non-guard-clause early return. } elseif ($search === '*') { - + // @review Why this empty elseif? } else { $quotedSearch = $GLOBALS['TYPO3_DB']->escapeStrForLike( $GLOBALS['TYPO3_DB']->quoteStr($search, 'cache_extensions'), @@ -866,8 +998,11 @@ class tx_em_Connection_ExtDirectServer { (CASE WHEN cache_extensions.extkey LIKE \'%' . $quotedSearch . '%\' THEN 60 ELSE 5 END) + (CASE WHEN cache_extensions.title LIKE \'%' . $quotedSearch . '%\' THEN 40 ELSE 5 END) AS relevance'; + // @review Please don't use these magic numbers. Please use class constants etc. instead. + // @review Please use fullQuoteStr instead of adding the quotes yourself. if (t3lib_extMgm::isLoaded('dbal')) { + // @review Then you don't need to fill $addFields in the first place, and use an if/else construct instead. // as dbal can't use the sum, make it more easy for dbal $addFields = 'CASE WHEN cache_extensions.extkey = \'' . $search . '\' THEN 100 ELSE 10 END AS relevance'; } @@ -876,6 +1011,8 @@ class tx_em_Connection_ExtDirectServer { } // check for filter $where .= $this->makeFilterQuery(get_object_vars($parameters)); + // @review Please don't use get_object_vars here, but instead make it explicit + // what you are doing here. if ($installedOnly) { $temp = array(); @@ -884,11 +1021,15 @@ class tx_em_Connection_ExtDirectServer { $temp[] = '"' . $key . '"'; } } + // @review Please use more meaningful variable names than $temp and $value here. $where .= ' AND cache_extensions.extkey IN(' . implode(',', $temp) . ')'; + // @review This SQL will be invalid when $temp is empty. $limit = ''; } + // @review This place looks like a good place to split this big function into + // two smaller, well-named functions. $list = tx_em_Database::getExtensionListFromRepository( $repositoryId, $addFields, @@ -896,24 +1037,33 @@ class tx_em_Connection_ExtDirectServer { $orderBy, $limit ); + // @review You only use $list['results']. So please put it in a local variable + // to simplify the following code. $updateKeys = array(); // transform array + // @review What's the purpose of this transformation? From what format + // is the data transformed to what format? This needs to be well-documented. + // I generally would find it more readable to have two arrays - one with the + // old data and one with the new data. foreach ($list['results'] as $key => $value) { $list['results'][$key]['dependencies'] = unserialize($value['dependencies']); $extPath = t3lib_div::strtolower($value['extkey']); + // @review The lowercase extension key is not the extension path. The variable needs to be renamed. $list['results'][$key]['statevalue'] = $value['state']; $list['results'][$key]['state'] = tx_em_Tools::getDefaultState(intval($value['state'])); $list['results'][$key]['stateCls'] = 'state-' . $list['results'][$key]['state']; $list['results'][$key]['version'] = tx_em_Tools::versionFromInt($value['maxintversion']); $list['results'][$key]['icon'] = ''; + // @review The version needs to be htmlspecialchared. $list['results'][$key]['exists'] = 0; $list['results'][$key]['installed'] = 0; $list['results'][$key]['versionislower'] = 0; $list['results'][$key]['existingVersion'] = ''; if (isset($localList[$value['extkey']])) { + // @review What does this if (above) mean (semantically)? $isUpdatable = ($localList[$value['extkey']]['intversion'] < $value['maxintversion']); $list['results'][$key]['exists'] = 1; $list['results'][$key]['installed'] = $localList[$value['extkey']]['installed']; @@ -926,7 +1076,10 @@ class tx_em_Connection_ExtDirectServer { } // updatable only if ($installedOnly == 2) { + // @review The variable name $installedOnly sounds like a boolean. + // A (magic) number of 2 for it does not make sende. $temp = array(); + // @review Please rename $temp to something more meaningful. if (count($updateKeys)) { foreach ($updateKeys as $key) { $temp[]= $list['results'][$key]; @@ -938,6 +1091,8 @@ class tx_em_Connection_ExtDirectServer { return array( 'length' => $list['count'], + // @review Please use a local variable $count instead of abusing $list as a + // "general-purpose uncommented variable container". 'data' => $list['results'], ); @@ -986,8 +1141,10 @@ class tx_em_Connection_ExtDirectServer { $objRepository = t3lib_div::makeInstance('tx_em_Repository', $parameter->repository); if ($objRepository->getMirrorListUrl()) { + // @review Please do not use boolean operators on strings. Please use !== '' instead. $objRepositoryUtility = t3lib_div::makeInstance('tx_em_Repository_Utility', $objRepository); $mirrors = $objRepositoryUtility->getMirrors(TRUE)->getMirrors(); + // @review Why get mirrors on the mirrors? This call chain does not make sense. if (count($mirrors)) { @@ -1011,6 +1168,7 @@ class tx_em_Connection_ExtDirectServer { 'sponsor' => $mirror['sponsorname'], 'link' => $mirror['sponsorlink'], 'logo' => $mirror['sponsorlogo'], + // @review All this data needs to be hsced. Please fixe this for all places where the ExtDirect functions return data. ); } } @@ -1075,6 +1233,8 @@ class tx_em_Connection_ExtDirectServer { */ public function deleteRepository($uid) { if (intval($uid) < 2) { + // @review Please replace the magic number with a named constant - or with + // a function "isMainRepositoryUid". return array( 'success' => FALSE, 'error' => $GLOBALS['LANG']->sL('LLL:EXT:em/language/locallang.xml:repository_main_nodelete') @@ -1150,15 +1310,18 @@ class tx_em_Connection_ExtDirectServer { //drop default array_shift($theLanguages); $lang = $meta = array(); + // @review Please do not put two assignments in one statement. foreach ($theLanguages as $language) { $label = htmlspecialchars($GLOBALS['LANG']->sL('LLL:EXT:setup/mod/locallang.xml:lang_' . $language)); $lang[] = array( 'label' => $label, 'lang' => $language, 'selected' => is_array($selected) && in_array($language, $selected) ? 1 : 0 + // @review $selected will always be an array because trimExplode always returns an array. ); $meta[] = array( 'hidden' => is_array($selected) && in_array($language, $selected) ? 'false' : 'true', + // @review $selected will always be an array because trimExplode always returns an array. 'header' => $language, 'dataIndex' => $language, 'width' => '100', @@ -1187,6 +1350,7 @@ class tx_em_Connection_ExtDirectServer { $selected = t3lib_div::trimExplode(',', $this->globalSettings['selectedLanguages'], TRUE); $dir = count($parameter) - count($selected); + // @review $dir is a misleading variable for an integer. $diff = $dir < 0 ? array_diff($selected, $parameter) : array_diff($parameter, $selected); $type = tx_em_Tools::getExtTypeFromPath(t3lib_extMgm::extPath('em')); @@ -1198,6 +1362,7 @@ class tx_em_Connection_ExtDirectServer { ) ); $this->saveExtensionConfiguration($params); + // @review Please do not name a variable "params" if there also is a variable "parameters" in the same scope. return array( 'success' => TRUE, @@ -1218,9 +1383,11 @@ class tx_em_Connection_ExtDirectServer { public function fetchTranslations($extkey, $type, $selection) { $result = array(); if (is_array($selection) && count($selection)) { + // @review If you use type hinting for $selection, it will always be an array. $terConnection = t3lib_div::makeInstance('tx_em_Connection_Ter', $this); $this->xmlHandler = t3lib_div::makeInstance('tx_em_Tools_XmlHandler'); $this->xmlHandler->emObj = $this; + // @review Please use a setter and make the field non-public. $mirrorURL = $this->getSettingsObject()->getMirrorURL(); $infoIcon = ' '; @@ -1228,6 +1395,7 @@ class tx_em_Connection_ExtDirectServer { $newIcon = ' '; $okIcon = ' '; $errorIcon = ' '; + // @review Can't you use the functions that automatically build these spans? foreach ($selection as $lang) { $fetch = $terConnection->fetchTranslationStatus($extkey, $mirrorURL); @@ -1243,12 +1411,15 @@ class tx_em_Connection_ExtDirectServer { } if ($localmd5 !== $fetch[$lang]['md5']) { if ($type) { + // @review Please do not use boolean operators on strings. //fetch translation $ret = $terConnection->updateTranslation($extkey, $lang, $mirrorURL); $result[$lang] = $ret + // @return Please use $ret !== TRUE as you cannot be sure that the error string returned from updateTranslation casts to FALSE. ? $okIcon . $GLOBALS['LANG']->sL('LLL:EXT:em/language/locallang.xml:msg_updated') : $errorIcon . $GLOBALS['LANG']->sL('LLL:EXT:em/language/locallang.xml:msg_failed'); + // @reviw You need to do a hsc on all these labels. } else { //translation status $result[$lang] = $localmd5 !== '' @@ -1275,6 +1446,10 @@ class tx_em_Connection_ExtDirectServer { /** * Returns settings object. * + * @review The comment and the @return are incorrect as this functions returns the settings, not the settings object. + * Generally, I find it confusing to have some settings (instance) which can return some settings (array). + * Instead, I'd recommend to add a function get($key) to the settings class. + * * @access public * @return tx_em_Settings instance of settings object */ @@ -1297,6 +1472,9 @@ class tx_em_Connection_ExtDirectServer { /** * Load form values for settings form * + * @review Bug: Data in the ExtJS forms in the EM gets escaped too much: If a single or + * double quote is entered in a field, it gets displayed with a backslash before it. + * * @return array FormValues */ public function settingsFormLoad() { @@ -1392,6 +1570,9 @@ class tx_em_Connection_ExtDirectServer { list($installedList,) = $this->extensionList->getInstalledExtensions(); $newExtensionList = $this->extensionList->addExtToList($extensionKey, $installedList); + // @review Actually, calling add() on the extension list should update the extension list. + // Having an extension list object that returns an exension list array is very error prone. + // I recommend working with the object only. $install->writeNewExtensionList($newExtensionList); tx_em_Tools::refreshGlobalExtList(); @@ -1401,6 +1582,8 @@ class tx_em_Connection_ExtDirectServer { /** * Reset all states for current user * + * @review Please document to what kind of states this refers. + * * @return void */ public function resetStates() { @@ -1422,6 +1605,7 @@ class tx_em_Connection_ExtDirectServer { /** @var $objRepositoryUtility tx_em_Repository_Utility */ $objRepositoryUtility = t3lib_div::makeInstance('tx_em_Repository_Utility', $objRepository); $mirrors = $objRepositoryUtility->getMirrors(TRUE)->getMirrors(); + // @review Why call getMirrors on the mirrors? if ($settings['selectedMirror'] == '') { @@ -1430,7 +1614,9 @@ class tx_em_Connection_ExtDirectServer { } else { foreach($mirrors as $mirror) { if ($mirror['host'] == $settings['selectedMirror']) { + // @review If you identify the mirros by host, you'll need to ensure that the hosts in the mirrors are unique. $mirrorUrl = $mirror['host'] . $mirror['path']; + // @review The variable name is incorrect as the URL also includes the schema. break; } } @@ -1448,23 +1634,32 @@ class tx_em_Connection_ExtDirectServer { protected function makeFilterQuery($parameter) { $where = ''; $filter = $found = array(); + // @review Please do not use two assignments in one statement. foreach ($parameter as $key => $value) { if (substr($key, 0, 6) === 'filter') { eval('$' . $key . ' = \'' . $value . '\';'); + // @review You do not need eval here. Actually, it is quite unsafe to use it in this way as + // this would allow execution of arbitrary PHP code. + // In addition, this will break if $value contains a single quote. + // You could just use $$key = $value. } } if (count($filter)) { + // @review You do not need the count here as foreach with an empty array is a no-op. foreach ($filter as $value) { switch ($value['data']['type']) { case 'list': if ($value['field'] === 'statevalue') { $where .= ' AND cache_extensions.state IN(' . htmlspecialchars($value['data']['value']) . ')'; + // @review You need to use quoteStr, not htmlspecialchars (both times). + // @review This query will be invalid SQL if $value['data']['value'] is empty. } if ($value['field'] === 'category') { $where .= ' AND cache_extensions.category IN(' . htmlspecialchars($value['data']['value']) . ')'; + // @review This query will be invalid SQL if $value['data']['value'] is empty. } break; default: @@ -1473,6 +1668,7 @@ class tx_em_Connection_ExtDirectServer { 'cache_extensions' ); $where .= ' AND cache_extensions.' . htmlspecialchars($value['field']) . ' LIKE "%' . $quotedSearch . '%"'; + // @review Please use blacklisting for the field name. Otherwise, this would allow execution of arbitrary SQL. } } } @@ -1496,6 +1692,9 @@ class tx_em_Connection_ExtDirectServer { /** * Get the selected repository * + * @review If this function returns a repository (according to the documentation), + * it should actually return a repository instance, not an array. + * * @return array */ protected function getSelectedRepository() { @@ -1579,6 +1778,9 @@ class tx_em_Connection_ExtDirectServer { * File operations like delete, copy, move * @param $file commandMap, @see * @return + * + * @review Please provide this function with a decent, detailed documentation about + * what it does and what its limits are. */ protected function fileOperation($file) { $mount = array(0 => array( @@ -1600,6 +1802,9 @@ class tx_em_Connection_ExtDirectServer { $httpHost = t3lib_div::getIndpEnv('TYPO3_HOST_ONLY'); if ($httpHost != $refInfo['host'] && $this->vC != $GLOBALS['BE_USER']->veriCode() + // @review $this->vC is never declared or written. + // @review You'll need to do strict comparisons in order to avoid problems with strings + // that get cast e.g., to integers. && !$GLOBALS['TYPO3_CONF_VARS']['SYS']['doNotCheckReferer'] && $GLOBALS['CLIENT']['BROWSER'] != 'flash') { $fileProcessor->writeLog(0, 2, 1, 'Referer host "%s" and server host "%s" did not match!', array($refInfo['host'], $httpHost)); diff --git a/typo3/sysext/em/classes/index.php b/typo3/sysext/em/classes/index.php index 7129cf6..7333155 100644 --- a/typo3/sysext/em/classes/index.php +++ b/typo3/sysext/em/classes/index.php @@ -152,6 +152,11 @@ class SC_mod_tools_em_index extends t3lib_SCbase { * Class for new ExtJs Extension Manager * * @var tx_em_ExtensionManager + * + * The type hinting here is not quite correct: This field may either hold a + * tx_em_ExtensionManager instance or an SC_mod_tools_em_index instance. + * As the only function called on this object is render(), an interface would be + * the correct way to use here. */ public $extensionmanager; diff --git a/typo3/sysext/em/classes/install/class.tx_em_install.php b/typo3/sysext/em/classes/install/class.tx_em_install.php index 76f5f6a..e4e0fce 100644 --- a/typo3/sysext/em/classes/install/class.tx_em_install.php +++ b/typo3/sysext/em/classes/install/class.tx_em_install.php @@ -88,6 +88,7 @@ class tx_em_Install { */ public function setSilentMode($silentMode) { $this->silentMode = $silentMode ? TRUE : FALSE; + // @review If $silentMode is a boolean anyway, you don't need the ternary operator here. } /** @@ -728,6 +729,9 @@ class tx_em_Install { * @param string Extension directory to remove (with trailing slash) * @param boolean If set, will leave the extension directory * @return boolean False on success, otherwise error string. + * + * @review Returning FALSE for the success case is highly unreadable + * (as FALSE usually indicates an error). */ function removeExtDirectory($removePath, $removeContentOnly = 0) { $errors = array(); diff --git a/typo3/sysext/em/ext_autoload.php b/typo3/sysext/em/ext_autoload.php index 93dcb17..c690506 100644 --- a/typo3/sysext/em/ext_autoload.php +++ b/typo3/sysext/em/ext_autoload.php @@ -5,6 +5,7 @@ $emInterfacesPath = $extensionPath . '/interfaces/'; return array( 'tx_em_index_checkdatabaseupdateshook' => $emInterfacesPath . 'interface.tx_em_index_checkdatabaseupdateshook.php', 'tx_em_renderhook' => $emInterfacesPath . 'interface.tx_em_renderhook.php', + // @review The tx_em_renderhook interface does not exist. 'sc_mod_tools_em_index' => $emClassesPath . 'index.php', 'tx_em_api' => $emClassesPath . 'class.tx_em_api.php',