Skip to content

Commit

Permalink
#6057 Address code review for submission files
Browse files Browse the repository at this point in the history
- Update DataObject::setAllData() to not use a reference
- Use setAllData() when creating new objects in the API
- Get submission file locale in fetch query instead of
	separate query
- Use flysystem stream when downloading files
- Don't allow a submission file's createdAt property to
  be edited
- Restore deprecated log constants to preserve historical
  data
- Add missing locale keys
- Add references to several hooks
- Update license to GNU GPL v3 where it was still v2.
  • Loading branch information
NateWr committed Oct 27, 2020
1 parent 8c87989 commit 3735157
Show file tree
Hide file tree
Showing 39 changed files with 161 additions and 157 deletions.
2 changes: 1 addition & 1 deletion api/v1/_submissions/PKPBackendSubmissionsHandler.inc.php
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,7 @@ public function getMany($slimRequest, $response, $args) {
case 'status':
case 'stageIds':
case 'assignedTo':
if (is_string($val) && strpos($val, ',') > -1) {
if (is_string($val)) {
$val = explode(',', $val);
} elseif (!is_array($val)) {
$val = array($val);
Expand Down
8 changes: 4 additions & 4 deletions api/v1/announcements/PKPAnnouncementHandler.inc.php
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,9 @@
/**
* @file api/v1/announcements/PKPAnnouncementHandler.inc.php
*
* Copyright (c) 2014-2019 Simon Fraser University
* Copyright (c) 2014-2020 Simon Fraser University
* Copyright (c) 2003-2019 John Willinsky
* Distributed under the GNU GPL v2. For full terms see the file docs/COPYING.
* Distributed under the GNU GPL v3. For full terms see the file docs/COPYING.
*
* @class PKPAnnouncementHandler
* @ingroup api_v1_announcement
Expand Down Expand Up @@ -131,7 +131,7 @@ public function getMany($slimRequest, $response, $args) {
switch ($param) {
case 'contextIds':
case 'typeIds':
if (is_string($val) && strpos($val, ',') > -1) {
if (is_string($val)) {
$val = explode(',', $val);
} elseif (!is_array($val)) {
$val = [$val];
Expand Down Expand Up @@ -198,7 +198,7 @@ public function add($slimRequest, $response, $args) {
}

$announcement = DAORegistry::getDao('AnnouncementDAO')->newDataObject();
$announcement->_data = $params;
$announcement->setAllData($params);
$announcement = Services::get('announcement')->add($announcement, $request);
$announcementProps = Services::get('announcement')->getFullProperties($announcement, [
'request' => $request,
Expand Down
2 changes: 1 addition & 1 deletion api/v1/contexts/PKPContextHandler.inc.php
Original file line number Diff line number Diff line change
Expand Up @@ -293,7 +293,7 @@ public function add($slimRequest, $response, $args) {
}

$context = Application::getContextDAO()->newDataObject();
$context->_data = $params;
$context->setAllData($params);
$context = $contextService->add($context, $request);
$contextProps = $contextService->getFullProperties($context, array(
'request' => $request,
Expand Down
4 changes: 2 additions & 2 deletions api/v1/emailTemplates/PKPEmailTemplateHandler.inc.php
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ public function getMany($slimRequest, $response, $args) {
case 'fromRoleIds':
case 'toRoleIds':
case 'stageIds':
if (is_string($val) && strpos($val, ',') > -1) {
if (is_string($val)) {
$val = explode(',', $val);
} elseif (!is_array($val)) {
$val = array($val);
Expand Down Expand Up @@ -199,7 +199,7 @@ public function add($slimRequest, $response, $args) {
}

$emailTemplate = Application::getContextDAO()->newDataObject();
$emailTemplate->_data = $params;
$emailTemplate->setAllData($params);
$emailTemplate = Services::get('emailTemplate')->add($emailTemplate, $request);

$data = Services::get('emailTemplate')->getFullProperties($emailTemplate, [
Expand Down
8 changes: 3 additions & 5 deletions api/v1/submissions/PKPSubmissionFileHandler.inc.php
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,7 @@ public function getMany($slimRequest, $response, $args) {
switch ($param) {
case 'fileStages':
case 'reviewRoundIds':
if (is_string($val) && strpos($val, ',') > -1) {
if (is_string($val)) {
$val = explode(',', $val);
} elseif (!is_array($val)) {
$val = array($val);
Expand Down Expand Up @@ -324,7 +324,7 @@ public function add($slimRequest, $response, $args) {
}

$submissionFile = DAORegistry::getDao('SubmissionFileDAO')->newDataObject();
$submissionFile->_data = $params;
$submissionFile->setAllData($params);

$submissionFile = Services::get('submissionFile')->add($submissionFile, $request);

Expand Down Expand Up @@ -353,9 +353,7 @@ public function edit($slimRequest, $response, $args) {
$params = $this->convertStringsToSchema(SCHEMA_SUBMISSION_FILE, $slimRequest->getParsedBody());

// Don't allow these properties to be modified
unset($params['submissionId']);
unset($params['fileId']);
unset($params['uploaderUserId']);
unset($params['submissionId'], $params['fileId'], $params['uploaderUserId']);

if (empty($params) && empty($_FILES['file'])) {
return $response->withStatus(400)->withJsonError('api.submissions.files.400.noParams');
Expand Down
6 changes: 3 additions & 3 deletions api/v1/submissions/PKPSubmissionHandler.inc.php
Original file line number Diff line number Diff line change
Expand Up @@ -209,7 +209,7 @@ public function getMany($slimRequest, $response, $args) {
case 'status':
case 'stageIds':
case 'assignedTo':
if (is_string($val) && strpos($val, ',') > -1) {
if (is_string($val)) {
$val = explode(',', $val);
} elseif (!is_array($val)) {
$val = array($val);
Expand Down Expand Up @@ -332,7 +332,7 @@ public function add($slimRequest, $response, $args) {

$submissionDao = DAORegistry::getDAO('SubmissionDAO'); /* @var $submissionDao SubmissionDAO */
$submission = $submissionDao->newDataObject();
$submission->_data = $params;
$submission->setAllData($params);
$submission = Services::get('submission')->add($submission, $request);
$userGroupDao = DAORegistry::getDAO('UserGroupDAO'); /* @var $userGroupDao UserGroupDAO */

Expand Down Expand Up @@ -585,7 +585,7 @@ public function addPublication($slimRequest, $response, $args) {
$publicationDao = DAORegistry::getDAO('PublicationDAO'); /* @var $publicationDao PublicationDAO */
$userGroupDao = DAORegistry::getDAO('UserGroupDAO'); /* @var $userGroupDao UserGroupDAO */
$publication = $publicationDao->newDataObject();
$publication->_data = $params;
$publication->setAllData($params);
$publication = Services::get('publication')->add($publication, $request);
$publicationProps = Services::get('publication')->getFullProperties(
$publication,
Expand Down
2 changes: 1 addition & 1 deletion api/v1/users/PKPUserHandler.inc.php
Original file line number Diff line number Diff line change
Expand Up @@ -235,7 +235,7 @@ private function _processAllowedparams($params, $allowedKeys) {

// Always convert roleIds to array
case 'roleIds':
if (is_string($val) && strpos($val, ',') > -1) {
if (is_string($val)) {
$val = explode(',', $val);
} elseif (!is_array($val)) {
$val = [$val];
Expand Down
4 changes: 2 additions & 2 deletions classes/core/DataObject.inc.php
Original file line number Diff line number Diff line change
Expand Up @@ -177,8 +177,8 @@ public function &getAllData() {
* Set all data variables at once.
* @param $data array
*/
public function setAllData(&$data) {
$this->_data =& $data;
public function setAllData($data) {
$this->_data = $data;
}

/**
Expand Down
4 changes: 4 additions & 0 deletions classes/log/PKPSubmissionEventLogEntry.inc.php
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,10 @@
// Production events
define('SUBMISSION_LOG_PROOFS_APPROVED', 0x50000008);

// Deprecated events. Preserved for historical data.
define('SUBMISSION_LOG_LAST_REVISION_DELETED', 0x50000003); // uses submission.event.lastRevisionDeleted


class PKPSubmissionEventLogEntry extends EventLogEntry {

//
Expand Down
5 changes: 4 additions & 1 deletion classes/log/SubmissionFileEventLogEntry.inc.php
Original file line number Diff line number Diff line change
Expand Up @@ -21,14 +21,17 @@
define('SUBMISSION_LOG_FILE_UPLOAD', 0x50000001);
define('SUBMISSION_LOG_FILE_DELETE', 0x50000002);
define('SUBMISSION_LOG_FILE_REVISION_UPLOAD', 0x50000008);
define('SUBMISSION_LOG_FILE_EDIT', 0x50000009);
define('SUBMISSION_LOG_FILE_EDIT', 0x50000010);

// Audit events
define('SUBMISSION_LOG_FILE_AUDITOR_ASSIGN', 0x50000004);
define('SUBMISSION_LOG_FILE_AUDITOR_CLEAR', 0x50000005);
define('SUBMISSION_LOG_FILE_AUDIT_UPLOAD', 0x50000006);
define('SUBMISSION_LOG_FILE_SIGNOFF_SIGNOFF', 0x50000007);

// Deprecated events. Preserve for historical logs
define('SUBMISSION_LOG_FILE_REVISION_DELETE', 0x50000009); // uses submission.event.revisionDeleted

class SubmissionFileEventLogEntry extends EventLogEntry {
}

Expand Down
16 changes: 8 additions & 8 deletions classes/services/PKPAnnouncementService.inc.php
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,9 @@
/**
* @file classes/services/PKPAnnouncementService.php
*
* Copyright (c) 2014-2019 Simon Fraser University
* Copyright (c) 2000-2019 John Willinsky
* Distributed under the GNU GPL v2. For full terms see the file docs/COPYING.
* Copyright (c) 2014-2020 Simon Fraser University
* Copyright (c) 2000-2020 John Willinsky
* Distributed under the GNU GPL v3. For full terms see the file docs/COPYING.
*
* @class PKPAnnouncementService
* @ingroup services
Expand Down Expand Up @@ -104,7 +104,7 @@ public function getQueryBuilder($args = []) {
$announcementQB->filterByTypeIds($args['typeIds']);
}

\HookRegistry::call('Announcement::getMany::queryBuilder', [$announcementQB, $args]);
\HookRegistry::call('Announcement::getMany::queryBuilder', [&$announcementQB, $args]);

return $announcementQB;
}
Expand Down Expand Up @@ -209,7 +209,7 @@ public function validate($action, $props, $allowedLocales, $primaryLocale) {
public function add($announcement, $request) {
$announcement->setData('datePosted', Core::getCurrentDate());
DAORegistry::getDao('AnnouncementDAO')->insertObject($announcement);
\HookRegistry::call('Announcement::add', [$announcement, $request]);
\HookRegistry::call('Announcement::add', [&$announcement, $request]);

return $announcement;
}
Expand All @@ -222,7 +222,7 @@ public function edit($announcement, $params, $request) {
$newAnnouncement = DAORegistry::getDAO('AnnouncementDAO')->newDataObject();
$newAnnouncement->_data = array_merge($announcement->_data, $params);

\HookRegistry::call('Announcement::edit', array($newAnnouncement, $announcement, $params, $request));
\HookRegistry::call('Announcement::edit', array(&$newAnnouncement, $announcement, $params, $request));

DAORegistry::getDAO('AnnouncementDAO')->updateObject($newAnnouncement);
$newAnnouncement = $this->get($newAnnouncement->getId());
Expand All @@ -234,8 +234,8 @@ public function edit($announcement, $params, $request) {
* @copydoc \PKP\Services\EntityProperties\EntityWriteInterface::delete()
*/
public function delete($announcement) {
\HookRegistry::call('Announcement::delete::before', array($announcement));
\HookRegistry::call('Announcement::delete::before', array(&$announcement));
DAORegistry::getDao('AnnouncementDAO')->deleteObject($announcement);
\HookRegistry::call('Announcement::delete', array($announcement));
\HookRegistry::call('Announcement::delete', array(&$announcement));
}
}
10 changes: 5 additions & 5 deletions classes/services/PKPAuthorService.inc.php
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ public function getQueryBuilder($args = []) {
$authorQB->filterByAffiliation($args['affiliation']);
}

\HookRegistry::call('Author::getMany::queryBuilder', array($authorQB, $args));
\HookRegistry::call('Author::getMany::queryBuilder', array(&$authorQB, $args));

return $authorQB;
}
Expand Down Expand Up @@ -211,7 +211,7 @@ public function add($author, $request) {
$authorId = $authorDao->insertObject($author);
$author = $this->get($authorId);

\HookRegistry::call('Author::add', array($author, $request));
\HookRegistry::call('Author::add', array(&$author, $request));

return $author;
}
Expand All @@ -225,7 +225,7 @@ public function edit($author, $params, $request) {
$newAuthor = $authorDao->newDataObject();
$newAuthor->_data = array_merge($author->_data, $params);

\HookRegistry::call('Author::edit', array($newAuthor, $author, $params, $request));
\HookRegistry::call('Author::edit', array(&$newAuthor, $author, $params, $request));

$authorDao->updateObject($newAuthor);
$newAuthor = $this->get($newAuthor->getId());
Expand All @@ -237,9 +237,9 @@ public function edit($author, $params, $request) {
* @copydoc \PKP\Services\EntityProperties\EntityWriteInterface::delete()
*/
public function delete($author) {
\HookRegistry::call('Author::delete::before', [$author]);
\HookRegistry::call('Author::delete::before', [&$author]);
$authorDao = DAORegistry::getDAO('AuthorDAO'); /* @var $authorDao AuthorDAO */
$authorDao->deleteObject($author);
\HookRegistry::call('Author::delete', [$author]);
\HookRegistry::call('Author::delete', [&$author]);
}
}
10 changes: 5 additions & 5 deletions classes/services/PKPContextService.inc.php
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,7 @@ public function getQueryBuilder($args = array()) {
->filterByUserId($args['userId'])
->searchPhrase($args['searchPhrase']);

\HookRegistry::call('Context::getMany::queryBuilder', array($contextListQB, $args));
\HookRegistry::call('Context::getMany::queryBuilder', array(&$contextListQB, $args));

return $contextListQB;
}
Expand Down Expand Up @@ -407,7 +407,7 @@ public function add($context, $request) {
// Load all plugins so they can hook in and add their installation settings
\PluginRegistry::loadAllPlugins();

\HookRegistry::call('Context::add', array($context, $request));
\HookRegistry::call('Context::add', array(&$context, $request));

return $context;
}
Expand Down Expand Up @@ -440,7 +440,7 @@ public function edit($context, $params, $request) {
$newContext = $contextDao->newDataObject();
$newContext->_data = array_merge($context->_data, $params);

\HookRegistry::call('Context::edit', array($newContext, $context, $params, $request));
\HookRegistry::call('Context::edit', array(&$newContext, $context, $params, $request));

$contextDao->updateObject($newContext);
$newContext = $this->get($newContext->getId());
Expand All @@ -452,7 +452,7 @@ public function edit($context, $params, $request) {
* @copydoc \PKP\Services\EntityProperties\EntityWriteInterface::delete()
*/
public function delete($context) {
\HookRegistry::call('Context::delete::before', array($context));
\HookRegistry::call('Context::delete::before', array(&$context));

$contextDao = Application::getContextDao();
$contextDao->deleteObject($context);
Expand Down Expand Up @@ -489,7 +489,7 @@ public function delete($context) {
$contextPath = \Config::getVar('files', 'files_dir') . '/' . $this->contextsFileDirName . '/' . $context->getId();
$fileManager->rmtree($contextPath);

\HookRegistry::call('Context::delete', array($context));
\HookRegistry::call('Context::delete', array(&$context));
}

/**
Expand Down
10 changes: 5 additions & 5 deletions classes/services/PKPEmailTemplateService.inc.php
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,7 @@ public function getQueryBuilder($args = []) {
->filterByStageIds($args['stageIds'])
->searchPhrase($args['searchPhrase']);

HookRegistry::call('EmailTemplate::getMany::queryBuilder', array($emailTemplateQB, $args));
HookRegistry::call('EmailTemplate::getMany::queryBuilder', array(&$emailTemplateQB, $args));

return $emailTemplateQB;
}
Expand Down Expand Up @@ -277,7 +277,7 @@ public function add($emailTemplate, $request) {
$emailTemplateDao->insertObject($emailTemplate);
$emailTemplate = $this->getByKey($contextId, $emailTemplate->getData('key'));

HookRegistry::call('EmailTemplate::add', array($emailTemplate, $request));
HookRegistry::call('EmailTemplate::add', array(&$emailTemplate, $request));

return $emailTemplate;
}
Expand All @@ -290,7 +290,7 @@ public function edit($emailTemplate, $params, $request) {
$newEmailTemplate = $emailTemplateDao->newDataObject();
$newEmailTemplate->_data = array_merge($emailTemplate->_data, $params);

HookRegistry::call('EmailTemplate::edit', array($newEmailTemplate, $emailTemplate, $params, $request));
HookRegistry::call('EmailTemplate::edit', array(&$newEmailTemplate, $emailTemplate, $params, $request));

$emailTemplateKey = $emailTemplate->getData('key');

Expand Down Expand Up @@ -318,10 +318,10 @@ public function edit($emailTemplate, $params, $request) {
* @copydoc \PKP\Services\EntityProperties\EntityWriteInterface::delete()
*/
public function delete($emailTemplate) {
HookRegistry::call('EmailTemplate::delete::before', array($emailTemplate));
HookRegistry::call('EmailTemplate::delete::before', array(&$emailTemplate));
$emailTemplateDao = DAORegistry::getDAO('EmailTemplateDAO'); /* @var $emailTemplateDao EmailTemplateDAO */
$emailTemplateDao->deleteObject($emailTemplate);
HookRegistry::call('EmailTemplate::delete', array($emailTemplate));
HookRegistry::call('EmailTemplate::delete', array(&$emailTemplate));
}

/**
Expand Down
Loading

0 comments on commit 3735157

Please sign in to comment.