New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

pkp/pkp-lib#3617 Support for MS SQL #3603

Open
wants to merge 27 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@nibou230

nibou230 commented Apr 17, 2018

Here is the code relative to the integration of MS SQL database support.

@@ -421,7 +421,7 @@ function checkCSRF() {
* Get the remote IP address of the current request.
* @return string
*/
function getRemoteAddr() {
static function getRemoteAddr() {

This comment has been minimized.

@asmecher

asmecher Apr 17, 2018

Member

I know the kinds of warnings you're working to resolve with these, but longer term, our solution will be to have a $request instance available to call on (rather than static calls). So the warnings will be helpful in identifying these.

This comment has been minimized.

@nibou230

nibou230 Apr 18, 2018

On our side, we get an exception in this file when we try to upload a file somewhere in the application. Here is the exceptions I get for this event.

PHP Deprecated:  Non-static method PKPRequest::getUserVar() should not be called statically in C:\inetpub\wwwroot\ojs\lib\pkp\classes\form\Form.inc.php on line 369

PHP Deprecated:  Non-static method PKPRequest::_checkThis() should not be called statically in C:\inetpub\wwwroot\ojs\lib\pkp\classes\core\PKPRequest.inc.php on line 592

PHP Deprecated:  Methods with the same name as their class will not be constructors in a future version of PHP; Smarty_Compiler has a deprecated constructor in C:\inetpub\wwwroot\ojs\lib\pkp\lib\vendor\smarty\smarty\libs\Smarty_Compiler.class.php on line 35

Every changes we made is generally to prevent the application throwing an error and freezing at the same time at the user interface side. I know it's certainly not the right way to fix it, but we can't let the application freeze each time we try to upload a file. Can you hint us on how to resolve this problem?

This is also linked to the comment below.

This comment has been minimized.

@asmecher

asmecher Apr 18, 2018

Member

Yes, simply configuring PHP to direct warnings/errors to the log file and not the browser should resolve the UI freezing. We're working to resolve the warnings gradually but it'll take a few releases.

return $this;
} else {
} else {*/

This comment has been minimized.

@asmecher

asmecher Apr 17, 2018

Member

(This stop-gap is necessary while we refactor Request static calls into instance calls.)

@@ -444,7 +444,9 @@ static function mime_content_type($filename, $suggestedExtension = '') {
}
// Check ambiguous mimetypes against extension
$ext = array_pop(explode('.',$filename));
// The result of "explode" can't be passed directly to "array_pop" in PHP 7.

This comment has been minimized.

@asmecher

asmecher Apr 17, 2018

Member

I'm using PHP7 and haven't run into this -- are you getting a warning?

This comment has been minimized.

@nibou230

nibou230 Apr 18, 2018

We are getting this exception, which is causing the UI to freeze even if the action is completed.

PHP Notice:  Only variables should be passed by reference in C:\inetpub\wwwroot\ojs\lib\pkp\classes\core\PKPString.inc.php on line 447

This comment has been minimized.

@asmecher

asmecher Apr 18, 2018

Member

Ah, OK. Happy to have warnings like this fixed. FYI, it's not recommended to have warning/error messages displayed to the browser, and if they are, it'll probably explain why the UI is freezing when you encounter a cosmetic warning.

This comment has been minimized.

@asmecher
$errorNo = $dataSource->errorNo();
// For MS SQL (on an update), the statement query is trying to fetch a
// non existing row and this action is throwing the exception code -15.

This comment has been minimized.

@asmecher

asmecher Apr 17, 2018

Member

Very weird -- is this documented by ADODB somewhere?

This comment has been minimized.

@nibou230

nibou230 Apr 18, 2018

I did not see any documentation about this kind of problem for ADODB. It is probably how the library is used with the DAOs and SQL server. Somewhere, we try to fetch the non-existent results of the update.

This comment has been minimized.

@asmecher

asmecher Apr 18, 2018

Member

Can you let me know more about how this condition is triggered? Unfortunately I can't run SQL Server myself.

This comment has been minimized.

@nibou230

nibou230 Apr 18, 2018

It seems to be pretty frequent. On each update and in some cases in the selectLimit function. This fix is the fastest one I found to resolve the problem without investigating too deep in the libraries code.

@@ -312,7 +312,9 @@ function getObjectsByGroup($groupSymbolic, $contextId = CONTEXT_ID_NONE, $getTem
// result set that comply with the current runtime
// environment.
$matchingFilters = array();
foreach($result->GetAssoc() as $filterRow) {
$isSqlServer = Config::getVar('database', 'ms_sql');
$rows = $isSqlServer ? $result->GetRows() : $result->GetAssoc();

This comment has been minimized.

@asmecher

asmecher Apr 17, 2018

Member

Since we're not using the key in the foreach loop, I think we can just use GetRows for all drivers.

This comment has been minimized.

@nibou230

nibou230 Apr 18, 2018

So I can remove the condition and use GetRows for every databases. Can you confirm to me that this change won't cause some problems in the subsequent functions?

This comment has been minimized.

@asmecher

asmecher Apr 18, 2018

Member

Yes, in this case we're not using the returned array key at all.

@@ -146,6 +146,12 @@ static function initialize($request) {
date_default_timezone_set($timeZone);
if (Config::getVar('general', 'installed')) {
$isSqlServer = Config::getVar('database', 'ms_sql');
if ($isSqlServer) {

This comment has been minimized.

@asmecher

asmecher Apr 17, 2018

Member

For cleanliness, I'd suggest adding a new case below to deal with this. It might cost a few extra cycles in some cases, but it's more compact/consistent.

This comment has been minimized.

@nibou230

nibou230 Apr 18, 2018

All good, I am gonna add a static function to get the offset from UTC to avoid calling this portion of code each and every time with SQL server.

@@ -79,6 +79,7 @@ function __construct($request) {
'informix' => array('ifx', 'Informix'),
'sybase' => array('sybase', 'Sybase'),
'odbc' => array('odbc', 'ODBC'),
'pdo_sqlsrv' => array('pdo_sqlsrv', 'PDO SQL Server')

This comment has been minimized.

@asmecher

asmecher Apr 17, 2018

Member

If this is simply a PDO connection, and doesn't need to know anything particular about SQL Server, then why not just call it "PDO" here? It's possible that other connections could even be used. (I think this will only be useful if we can get rid of SQL Server-specific SQL formulations but so far I think that's possible.)

This comment has been minimized.

@asmecher

asmecher Apr 17, 2018

Member

(i.e. use the same driver name that ADODB expects, rather than pdo_sqlsrv)

e.event_type = ?' .
($userId ? ' AND u.user_id = ?' : ''),
e.event_type = ?'
. ($userId && !$isSqlServer ? ' AND u.user_id = ?' : ''),

This comment has been minimized.

@asmecher

asmecher Apr 17, 2018

Member

Won't this cause the function to return matches from other users? Is there a reason SQL Server won't accept the condition?

This comment has been minimized.

@nibou230

nibou230 Apr 18, 2018

You are right on this one, we should not exclude the condition. Sadly, there is a major problem we can't totally explain : the "email_log_users" table is totally empty. This said, SQL server can't return any results with the user ID filter.

The problem seems located in the method "_insertLogUserIds" when the following pattern is used :

$pattern = '/(?<=\<)[^\>]*(?=\>)/';
preg_match_all($pattern, $recipients, $matches);

Why the method is expecting emails formed this way : < "EMAIL" >? In our case, the characters < and > are not present in any conditions within the recipients.

This comment has been minimized.

@asmecher

asmecher Apr 19, 2018

Member

@nibou230, I suspect it's expecting emails to be provided in the form Screen Name <emailaddress@domain.com>.

@@ -37,7 +37,9 @@ protected function bootstrap() {
// Map valid OJS3 config options to Illuminate database drivers
$driver = strtolower(Config::getVar('database', 'driver'));
if (substr($driver, 0, 8) === 'postgres') {
if (strpos($driver, 'sqlsrv') !== false) {
$driver = 'sqlsrv';

This comment has been minimized.

@asmecher

asmecher Apr 17, 2018

Member

(Watch out for space-based indents.)

This comment has been minimized.

@nibou230

nibou230 Apr 18, 2018

Yes, I will do my best to avoid those space-based indents, I saw the documentation about them after all the modifications we made to support SQL server.

sprintf(
'INSERT INTO edit_decisions
$sqlUpdate = sprintf(
'INSERT INTO edit_decisions

This comment has been minimized.

@asmecher

asmecher Apr 17, 2018

Member

Is this change doing anything? It looks like it's just reformatting.

This comment has been minimized.

@nibou230

nibou230 Apr 18, 2018

It's actually doing nothing special. I maybe thought for a moment it was to fix another UI freezing problem. Probably forgot to change it back after some corrections.

$params
);
$isSqlServer = Config::getVar('database', 'ms_sql');
$sql = sprintf(($fileId && $isSqlServer ? 'SET IDENTITY_INSERT submission_files ON;' : '') .

This comment has been minimized.

@asmecher

asmecher Apr 17, 2018

Member

Could you point me to something describing this aspect of SQL Server? I'm not familiar.

This comment has been minimized.

@nibou230

nibou230 Apr 18, 2018

The IDs for the table are generated sequentially so SQL server doesn't accept the modifications or the insertions over on this value. That's where the IDENTITY_INSERT ON/OFF comes handy in some cases. But as you said in the comment below, if we can remove the ID from the update and do a clear update instead of an insertion when the file ID is not null, than we can make the code works for every databases.

// MS SQL doesn't accept the UPDATE on a primary key IDENTITY
$isSqlServer = Config::getVar('database', 'ms_sql');
$params = array((int)$submissionFile->getRevision(),

This comment has been minimized.

@asmecher

asmecher Apr 17, 2018

Member

So the issue is updating the file ID or revision, right? I don't think OJS ever changes these values for existing entries, so it should be possible to remove them from the UPDATE entirely for all DB types. We would of course have to test for this!

This comment has been minimized.

@nibou230

nibou230 Apr 23, 2018

I removed the file ID for the update section, but I can't make the same change for the insertion as the table is composed with a two columns primary key.

We need to insert a new revision each time in the table and SQL server doesn't really like it.

To make it functionnal within all databases without the use of "IDENTITY_INSERT ON/OFF", a refactor is needed to split the table in 2 (one with the file ID and revision ID as a foreign key and another one with the revision ID + associated data).

$result = $this->retrieve(
$this->_getSelectQuery() .
' WHERE r.submission_id = ? AND
r.reviewer_id = ?
ORDER BY r2.stage_id DESC, r2.round DESC LIMIT 1',
ORDER BY r2.stage_id DESC, r2.round DESC
' . ($isSqlServer ? 'OFFSET 0 ROWS FETCH NEXT 1 ROWS ONLY' : 'LIMIT 1'),

This comment has been minimized.

@asmecher

asmecher Apr 17, 2018

Member

I think you could use DAO::retrieveRange for all DB types, and remove this extra consideration for SQL Server.

This comment has been minimized.

@asmecher

asmecher Apr 17, 2018

Member

(rather than DAO::retrieve as it currently is written)

This comment has been minimized.

@nibou230

nibou230 Apr 18, 2018

You are right, gonna go this way instead.

$result = $this->retrieve(
'SELECT *
FROM review_rounds
WHERE submission_id = ?
' . ($stageId ? ' AND stage_id = ?' : '') . '
ORDER BY stage_id DESC, round DESC
LIMIT 1',
' . ($isSqlServer ? 'OFFSET 0 ROWS FETCH NEXT 1 ROWS ONLY' : 'LIMIT 1'),

This comment has been minimized.

@asmecher

asmecher Apr 17, 2018

Member

As above, use DAO::retrieveRange for all DB types rather than add a SQL Server specific consideration.

$reviewAssignment->setDeclined($decline);
$reviewAssignment->setDateConfirmed(Core::getCurrentDate());
$reviewAssignment->setDateConfirmed($currentDate);

This comment has been minimized.

@asmecher

asmecher Apr 17, 2018

Member

I don't think this change does anything.

This comment has been minimized.

@nibou230

nibou230 Apr 18, 2018

True, it's an artifact of the completed reviews correction. I'll revert it.

@@ -51,7 +51,8 @@ function fetch($request) {
*/
function execute() {
// Set review to next step.
$this->updateReviewStepAndSaveSubmission($this->getReviewerSubmission());
$reviewerSubmission = $this->getReviewerSubmission();

This comment has been minimized.

@asmecher

asmecher Apr 17, 2018

Member

Watch out for space-based indents.

@@ -220,7 +220,8 @@ function execute($request) {
}
// Set review to next step.
$this->updateReviewStepAndSaveSubmission($this->getReviewerSubmission());
$reviewerSubmission = $this->getReviewerSubmission();
$this->updateReviewStepAndSaveSubmission($reviewerSubmission);

This comment has been minimized.

@asmecher

asmecher Apr 17, 2018

Member

This is for a reference warning, right? Maybe better to remove the & from the function definition for updateReviewStepAndSaveSubmission. The reference isn't necessary and we're trying to remove useless references throughout (gradually, as there are lots).

@@ -13,7 +13,7 @@
"michelf/php-markdown": "1.5.0",
"slim/slim": "^3.0",
"pimple/pimple": "^3.0",
"illuminate/database": "4.1.*",
"illuminate/database": "5.6.*",

This comment has been minimized.

@asmecher

asmecher Apr 17, 2018

Member

Tagging @NateWr and @kaschioudi in case they're aware of any ramifications we should be aware of.

This comment has been minimized.

@NateWr

NateWr Apr 18, 2018

Contributor

My understanding is that Laravel 5.6 requires PHP 7+. I can't seem to find info specifically on the Illuminate database part of it, but I would guess if Laravel made the jump that it's DB layer would have as well...

This comment has been minimized.

@NateWr

NateWr Apr 18, 2018

Contributor

Yeah, it looks like it expects ^PHP 7.1.3: https://packagist.org/packages/illuminate/database

This comment has been minimized.

@asmecher

asmecher Apr 18, 2018

Member

Is this a required bump, @nibou230? We'd like to continue supporting all actively maintained releases of PHP (currently 5.6 or later) and 7.1.3+ is quite an increase.

This comment has been minimized.

@nibou230

nibou230 Apr 18, 2018

I'm affraid it's required to support SQL server. We tested 4.2.17 and the bug still remains.

You can see the problematic section at the line 56 of the file "https://github.com/illuminate/database/blob/4.2/Connectors/SqlServerConnector.php".

return "sqlsrv:Server={$host}{$port}{$dbName}";

When the host already contains the "sqlsrv:Server", the produced result looks like : "sqlsrv:Server=sqlsrv:Server" which is not expected by the connector...

Maybe there is an in between version that can fix it while still supporting PHP 5? Else, the only way to still support all versions of PHP would be to ask for a version 4.2.18 that will look at this specific exception and fix it.

This comment has been minimized.

@kaschioudi

kaschioudi Apr 23, 2018

Collaborator

@nibou230 I suggest bumping to v5.4.* which requires php: >=5.6.4 and I believe corrects the problem you are having (https://github.com/illuminate/database/blob/5.4/Connectors/SqlServerConnector.php#L41-L53)

This comment has been minimized.

@nibou230

nibou230 Apr 23, 2018

Thanks for the info @kaschioudi, I'll change the version for v5.4.* instead.

@@ -27,18 +27,20 @@ class DeleteFileLinkAction extends FileLinkAction {
function __construct($request, $submissionFile, $stageId, $localeKey = 'grid.action.delete') {
$router = $request->getRouter();
import('lib.pkp.classes.linkAction.request.RemoteActionConfirmationModal');
$modal = new RemoteActionConfirmationModal(

This comment has been minimized.

@asmecher

asmecher Apr 17, 2018

Member

Is this just a formatting change? Watch out for space-based indents, and better to exclude this kind of thing from the PR, as it's not related to the SQL Server issue.

This comment has been minimized.

@nibou230

nibou230 Apr 18, 2018

If I remember correctly, it was more to prevent the UI from freezing on another exception. Don't really have the will (time) to format the code just for the fun of it. ;)

This comment has been minimized.

@asmecher

asmecher Jun 20, 2018

Member

Resolved instead by removing the reference from the parent constructor's function definition: 4807816

@@ -96,22 +99,25 @@ function fetchTab($args, $request) {
$dispatcher = $request->getDispatcher();
import('lib.pkp.classes.linkAction.request.AjaxModal');
$submissionId = $submission->getId();
$url = $dispatcher->url(
$request, ROUTE_COMPONENT, null,

This comment has been minimized.

@asmecher

asmecher Apr 17, 2018

Member

Watch out for space-based indents.

@@ -276,7 +276,8 @@ function uploadFile($args, $request) {
// If no revised file id was given then try out whether
// the user maybe accidentally didn't identify this file as a revision.
if (!$uploadForm->getRevisedFileId()) {
$revisedFileId = $this->_checkForRevision($uploadedFile, $uploadForm->getSubmissionFiles());
$files = $uploadForm->getSubmissionFiles();

This comment has been minimized.

@asmecher

asmecher Apr 17, 2018

Member

Space-based indents.

@@ -384,7 +385,7 @@ function confirmRevision($args, $request) {
// Validate the form and revise the file.
if ($confirmationForm->validate($request)) {
if (is_a($uploadedFile = $confirmationForm->execute($request), 'SubmissionFile')) {
if (is_a($uploadedFile = $confirmationForm->execute(), 'SubmissionFile')) {

This comment has been minimized.

@asmecher

asmecher Apr 17, 2018

Member

This is for strict mode PHP warnings, right? We're probably going to have to standardize the parent Form class's implementation of various functions to have it require a $request instance, but this will take a lot of work and probably should be left out of this PR.

This comment has been minimized.

@nibou230

nibou230 Apr 18, 2018

Yes always for the annoying freezing state. I had the possibility to remove it because the request was not used in the execute method.

@@ -192,6 +192,9 @@
*/
$.pkp.controllers.modal.ModalHandler.prototype.modalOpen =
function($handledElement) {
if (!$handledElement) {

This comment has been minimized.

@asmecher

asmecher Apr 17, 2018

Member

Watch out for space-based indents. Is this to correct a bug or prevent a warning or something?

This comment has been minimized.

@nibou230

nibou230 Apr 18, 2018

Honnestly I don't know why it has been added here. I will remove it for now, but gonna keep an eye on it in case of a possible exception.

@asmecher

This comment has been minimized.

Member

asmecher commented Apr 17, 2018

@nibou230, excellent work, thanks again. I've made some general comments, and as per the OJS pull request, the less we have to specifically consider SQL Server as an exception in the code, the better -- I don't see many cases where we can't code behavior that suits all 3 DBMSs, rather than having special cases with different SQL.

I'd also suggest removing the ADODB library entirely from the repository, and adding it instead as a Composer dependency (see composer.json). That way Composer can handle versioning, now that we're jettisoning our changes to that library (which is a good thing).

$type = PKPString::mime_content_type(
$_FILES[$fileName]['tmp_name'], // Location on server
array_pop(explode('.',$_FILES[$fileName]['name'])) // Extension on client machine
array_pop($exploded) // Extension on client machine

This comment has been minimized.

@asmecher
@@ -34,7 +34,7 @@ function &getTemporaryFile($fileId, $userId) {
$returner = null;
if (isset($result) && $result->RecordCount() != 0) {
$returner =& $this->_returnTemporaryFileFromRow($result->GetRowAssoc(false));
$returner = $this->_returnTemporaryFileFromRow($result->GetRowAssoc(false));

This comment has been minimized.

@asmecher
@@ -96,7 +96,8 @@ function handleUpload($fileName, $userId) {
$temporaryFile->setUserId($userId);
$temporaryFile->setServerFileName($newFileName);
$temporaryFile->setFileType(PKPString::mime_content_type($this->getBasePath() . $newFileName, array_pop(explode('.', $_FILES[$fileName]['name']))));
$exploded = explode('.', $_FILES[$fileName]['name']);
$temporaryFile->setFileType(PKPString::mime_content_type($this->getBasePath() . $newFileName, array_pop($exploded)));

This comment has been minimized.

@asmecher

asmecher Jun 20, 2018

Member

All changes to this file merged: f75c688

@@ -312,7 +312,7 @@ function getObjectsByGroup($groupSymbolic, $contextId = CONTEXT_ID_NONE, $getTem
// result set that comply with the current runtime
// environment.
$matchingFilters = array();
foreach($result->GetAssoc() as $filterRow) {
foreach($result->GetRows() as $filterRow) {

This comment has been minimized.

@asmecher
@@ -103,7 +103,7 @@ function &configureFilter($filterNode, $persist = true) {
$similarFilterFactory = $filterDao->getObjectsByGroupAndClass($filterGroupSymbolic, $filterClassName, 0, $isTemplate);
if ($similarFilterFactory->getCount() > 0) {
// 1) Find similar filters.
$similarFilters =& $similarFilterFactory->toArray();
$similarFilters = $similarFilterFactory->toArray();

This comment has been minimized.

@asmecher
@@ -25,7 +25,7 @@ class SubmissionFileLog extends SubmissionLog {
* @param $params array optional
* @return object SubmissionLogEntry iff the event was logged
*/
static function logEvent($request, &$submissionFile, $eventType, $messageKey, $params = array()) {
static function logEvent($request, $submissionFile, $eventType, $messageKey, $params = array()) {

This comment has been minimized.

@asmecher
@@ -311,7 +311,7 @@ function insertObject($submissionFile, $sourceFile, $isUpload = false) {
// If the updated file does not have the correct target type then we'll have
// to retrieve it again from the database to cast it to the right type (downcast).
if (strtolower_codesafe(get_class($insertedFile)) != $targetImplementation) {
if ($insertedFile && strtolower_codesafe(get_class($insertedFile)) != $targetImplementation) {

This comment has been minimized.

@asmecher
$tryLocales = array(
$this->getFormLocale(), // Current form locale
AppLocale::getLocale(), // Current UI locale
$this->context->getPrimaryLocale(), // Context locale
$supportedSubmissionLocales[array_shift(array_keys($supportedSubmissionLocales))] // Fallback: first one on the list
$supportedSubmissionLocales[array_shift($keys)] // Fallback: first one on the list

This comment has been minimized.

@asmecher
$params
ORDER BY r2.stage_id DESC, r2.round DESC',
$params,
1

This comment has been minimized.

@asmecher
$params
ORDER BY stage_id DESC, round DESC',
$params,
1

This comment has been minimized.

@asmecher
@@ -99,7 +99,7 @@ function fetch($request) {
* update the given reviewer submission.
* @param $reviewerSubmission ReviewerSubmission
*/
function updateReviewStepAndSaveSubmission(&$reviewerSubmission) {
function updateReviewStepAndSaveSubmission($reviewerSubmission) {

This comment has been minimized.

@asmecher
@@ -113,7 +113,7 @@ function fetch($request) {
* @param $request PKPRequest
*/
function execute($request) {
$reviewAssignment =& $this->getReviewAssignment();
$reviewAssignment = $this->getReviewAssignment();

This comment has been minimized.

@asmecher
@@ -36,7 +36,8 @@ function getTemplateVarsFromRowColumn($row, $column) {
case 'name':
return array('label' => $userGroup->getLocalizedName());
case 'roleId':
return array('label' => __(array_shift(Application::getRoleNames(false, array($userGroup->getRoleId())))));
$roleNames = Application::getRoleNames(false, array($userGroup->getRoleId()));
return array('label' => __(array_shift($roleNames)));

This comment has been minimized.

@asmecher
@@ -70,7 +70,8 @@ function fetch($request) {
// assign the user groups options
$templateMgr->assign('userGroupOptions', $userGroupOptions);
// assigned the first element as selected
$templateMgr->assign('selectedUserGroupId', array_shift(array_keys($userGroupOptions)));
$keys = array_keys($userGroupOptions);
$templateMgr->assign('selectedUserGroupId', array_shift($keys));

This comment has been minimized.

@asmecher
@@ -201,7 +202,8 @@ protected function getFilterValues($filter) {
if (isset($filter['filterUserGroupId']) && $filter['filterUserGroupId']) {
$filterUserGroupId = $filter['filterUserGroupId'];
} else {
$filterUserGroupId = reset(array_keys($this->_userGroupOptions));
$keys = array_keys($this->_userGroupOptions);
$filterUserGroupId = reset($keys);

This comment has been minimized.

@asmecher

asmecher Jun 20, 2018

Member

Merged both cases: ee34663

$value[$locale] = array(
'name' => $temporaryFile->getOriginalFileName(),
'uploadName' => $uploadName,
'width' => $width,
'height' => $height,
'dateUploaded' => Core::getCurrentDate(),
'altText' => $imageAltText[$locale]
'altText' => $localizedImageAltText

This comment has been minimized.

@asmecher

asmecher Jun 20, 2018

Member

Resolved (inline): 8a83263

__('editor.submission.newRound'),
'modal_add_item'
),
$ajaxModal,

This comment has been minimized.

@asmecher

asmecher Jun 20, 2018

Member

I think this change is no longer needed as there's no reference in the LinkAction constructor's $actionRequest parameter.

This comment has been minimized.

@nibou230

nibou230 Jun 20, 2018

That's right, this change can be ignored and reverted on our side.

@@ -90,7 +90,7 @@ function access($args, $request) {
$workingStageId = null;
for ($workingStageId = $currentStageId; $workingStageId >= WORKFLOW_STAGE_ID_SUBMISSION; $workingStageId--) {
if (isset($accessibleWorkflowStages[$workingStageId]) && array_intersect($editorialWorkflowRoles, $accessibleWorkflowStages[$workingStageId])) {
if (array_key_exists($workingStageId, $accessibleWorkflowStages)) {

This comment has been minimized.

@asmecher

asmecher Jun 20, 2018

Member

I think this might inadvertently pass users in when isset($accessibleWorkflowStages[$workingStageId] = array(). The array_intersect will evaluate false in that case, but array_key_exists will evaluate true.

This comment has been minimized.

@nibou230

nibou230 Jun 20, 2018

Yep, I don't know why this change is still in our code. I'm going to revert it. Thanks!

@@ -198,6 +198,9 @@ function editorDecisionActions($args, $request) {
$lastReviewRound = $reviewRoundDao->getLastReviewRoundBySubmissionId($submission->getId(), $stageId);
$reviewRound = $reviewRoundDao->getById($reviewRoundId);
}
else {
$lastReviewRound = null;

This comment has been minimized.

@asmecher
@@ -238,7 +241,7 @@ function editorDecisionActions($args, $request) {
import('lib.pkp.classes.linkAction.request.AjaxModal');
$editorActions = array();
$lastRecommendation = $allRecommendations = null;
if (!empty($editorsStageAssignments) && (!$reviewRoundId || $reviewRoundId == $lastReviewRound->getId())) {
if (!empty($editorsStageAssignments) && (!$reviewRoundId || ($lastReviewRound && $reviewRoundId == $lastReviewRound->getId()))) {

This comment has been minimized.

@asmecher

This comment has been minimized.

@asmecher

asmecher Jun 20, 2018

Member

Oops, that should be a744079

$clone = $doc->importNode($representationDoc->documentElement, true);
$submissionNode->appendChild($clone);
if ($representationDoc) {
$clone = $doc->importNode($representationDoc->documentElement, true);

This comment has been minimized.

@asmecher

asmecher Jun 20, 2018

Member

There's probably some missing error handling here that's not your doing -- but I'd rather throw a PHP error than have it continue processing with missing information. I'll leave this one out.

@@ -231,7 +231,8 @@ function addAuthors($doc, $submissionNode, $submission) {
$exportFilter = array_shift($nativeExportFilters);
$exportFilter->setDeployment($this->getDeployment());
$authorsDoc = $exportFilter->execute($submission->getAuthors());
$authors = $submission->getAuthors();
$authorsDoc = $exportFilter->execute($authors);

This comment has been minimized.

@asmecher
@@ -158,7 +158,8 @@ function addUserGroups($doc, $rootNode) {
$exportFilter = array_shift($userGroupExportFilters);
$exportFilter->setDeployment($this->getDeployment());
$userGroupsDoc = $exportFilter->execute($userGroups->toArray());
$userGroups = $userGroups->toArray();
$userGroupsDoc = $exportFilter->execute($userGroups);

This comment has been minimized.

@asmecher

asmecher Jun 20, 2018

Member

Merged (slightly differently): 02f3c3e

@asmecher

This comment has been minimized.

Member

asmecher commented Jun 20, 2018

@nibou230, I've picked the trivial fixes (reference warnings etc.) from this and merged them into master, noting the commits in this PR. Rebasing your branch will should mostly result in the duplicate commits being removed from your branch. Exceptions are where our solutions were different (there are only a few of these), and where you had space-based indents and I used tabs. I'll work through the OJS one next. This should shorten your PR down considerably.

@nibou230

This comment has been minimized.

nibou230 commented Jun 20, 2018

Thanks @asmecher, that's great news to see our work is being integrated!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment