Bug #13582 ยป em_review.diff
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"> </span>';
|
||
... | ... | |
$newIcon = '<span class="t3-icon t3-icon-actions t3-icon-actions-system t3-icon-system-extension-import"> </span>';
|
||
$okIcon = '<span class="t3-icon t3-icon-status t3-icon-status-status t3-icon-status-checked"> </span>';
|
||
$errorIcon = '<span class="t3-icon t3-icon-status t3-icon-status-status t3-icon-status-permission-denied"> </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',
|