Project

General

Profile

Bug #13582 ยป em_review.diff

Oliver Klee, 2011-03-03 14:39

View differences:

typo3/sysext/em/classes/class.tx_em_extensionmanager.php
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(
typo3/sysext/em/classes/connection/class.tx_em_connection_extdirectserver.php
*/
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;
......
* 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 */
......
* Render extensionlist for languages
*
* @return unknown
*
* No, not unknown, but array.
*/
public function getInstalledExtkeys() {
$list = $this->getExtensionList();
......
);
$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?
}
}
......
* @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);
......
*/
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);
......
$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 .
'<br /><input type="submit" name="write" id="update-submit-' . htmlspecialchars($extKey) . '" value="' .
htmlspecialchars($GLOBALS['LANG']->getLL('updatesForm_make_updates')) . '" />';
// @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));
}
......
/** @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 '<p>' . $GLOBALS['LANG']->sL('LLL:EXT:em/language/locallang.xml:msg_extNoConfiguration') . '</p>';
} else {
//$form = preg_replace('/<form([^>]+)>/', '', $form);
//$form = str_replace('</form>', '', $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.
}
}
......
* 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) {
......
$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');
......
$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(
......
/**
* Cleans EMConf of extension
*
* @review Please document whether this function also directly saves the configuration.
*
* @param string $extKey
* @return array
*/
......
$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;
......
*
* @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();
}
......
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],
......
/**
* 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;
......
: $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) {
......
'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
);
}
......
$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],
......
* @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;
......
'oldFile' => $file,
'newFile' => $newFile,
'newFilename' => basename($newFile),
// @review The names "newFile" and "newFileName" do not make the differences between the two very clear.
);
}
......
* @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,
......
* @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,
......
$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' => '<pre>' . $result . '</pre>'
......
* @return array
*/
public function loadUploadExtToTer() {
// @review Actually, this function does not load any form.
$settings = $this->getSettings();
return array(
'success' => TRUE,
......
* @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) {
......
$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);
......
$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)) {
......
);
}
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']);
}
......
*
* @param string $parameter
* @return string
*
* @review This function returns an array, not a string.
*/
public function getExtensionDevelopInfo($extKey) {
/** @var $extensionList tx_em_Extensions_List*/
......
}
/**
/**
* Prints backupdelete
*
* @param string $parameter
* @return string
*
* @review This function returns an array, not a string.
*/
public function getExtensionBackupDelete($extKey) {
$content='';
......
// 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[] = '<tr class="t3-row-header"><td colspan="2">' .
......
}
$content = '<table border="0" cellpadding="2" cellspacing="2">' . implode('', $lines) . '</table>';
// @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);
......
/**
* 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;
......
* 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;
......
$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';
......
} 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'),
......
(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';
}
......
}
// 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();
......
$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,
......
$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'] = '<img alt="" src="' . $mirrorUrl . $extPath{0} . '/' . $extPath{1} . '/' . $extPath . '_' . $list['results'][$key]['version'] . '.gif" />';
// @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'];
......
}
// 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];
......
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'],
);
......
$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)) {
......
'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.
);
}
}
......
*/
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')
......
//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',
......
$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'));
......
)
);
$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,
......
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 = '<span class="t3-icon t3-icon-actions t3-icon-actions-document t3-icon-document-info">&nbsp;</span>';
......
$newIcon = '<span class="t3-icon t3-icon-actions t3-icon-actions-system t3-icon-system-extension-import">&nbsp;</span>';
$okIcon = '<span class="t3-icon t3-icon-status t3-icon-status-status t3-icon-status-checked">&nbsp;</span>';
$errorIcon = '<span class="t3-icon t3-icon-status t3-icon-status-status t3-icon-status-permission-denied">&nbsp;</span>';
// @review Can't you use the functions that automatically build these spans?
foreach ($selection as $lang) {
$fetch = $terConnection->fetchTranslationStatus($extkey, $mirrorURL);
......
}
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 !== ''
......
/**
* 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
*/
......
/**
* 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() {
......
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();
......
/**
* Reset all states for current user
*
* @review Please document to what kind of states this refers.
*
* @return void
*/
public function resetStates() {
......
/** @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'] == '') {
......
} 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;
}
}
......
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:
......
'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.
}
}
}
......
/**
* 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() {
......
* 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(
......
$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));
typo3/sysext/em/classes/index.php
* 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;
typo3/sysext/em/classes/install/class.tx_em_install.php
*/
public function setSilentMode($silentMode) {
$this->silentMode = $silentMode ? TRUE : FALSE;
// @review If $silentMode is a boolean anyway, you don't need the ternary operator here.
}
/**
......
* @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();
typo3/sysext/em/ext_autoload.php
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',
    (1-1/1)