Layout editor file download error

[aleskl]

Hi,
we're using OJS 2.4.1 and we just encountered a nasty bug regarding Layout editor pages. When the Layout editor is logged in and tries to download any file from the Editing step (Layout version, Supplementary files) the download fails with server error 500. The server log contains these two lines for each download event:

Code: Select all

NOTICE: Undefined variable: submission (/var/www/html/ojs/pages/layoutEditor/SubmissionLayoutHandler.inc.php:477), referer: http://www.ias-iss.org/ojs/IAS/layoutEditor/submission/926

PHP Fatal error:  Call to a member function getId() on a non-object in /var/www/html/ojs/classes/submission/layoutEditor/LayoutEditorAction.inc.php on line 356, referer: http://www.ias-iss.org/ojs/IAS/layoutEditor/submission/926


It's probably interesting to know that when Editor is logged in and tries to download the same files, it works perfectly. I'm going to look at the above two files to see if I can compare them with editor-associated ones.

cheers,
Ales

[aleskl]

Ok, I did some crude copy&pasting and now the download works for the Layout Editor! Let me see if I can summarize what I did (sorry for the backward slashes):

1. \pages\layoutEditor\SubmissionLayoutHandler.inc.php
replaced lines 437-479

Code: Select all

      if($this->validate($request, $articleId)) {
         $journal =& $request->getJournal();
         $submission =& $this->submission;
      }
      if (!LayoutEditorAction::downloadFile($submission, $fileId, $revision)) {
         $request->redirect(null, null, 'submission', $articleId);
      }


with these lines, essentially copied from \pages\sectionEditor\SubmissionEditHandler.inc.php

Code: Select all

      $this->validate($articleId);
      if (!LayoutEditorAction::downloadFile($articleId, $fileId, $revision)) {
         $request->redirect(null, null, 'submission', $articleId);
      }


2. \classes\submission\layoutEditor\LayoutEditorAction.inc.php
replaced the function downloadFile (lines 349-374)

Code: Select all

   function downloadFile($article, $fileId, $revision = null) {
      $canDownload = false;

      $galleyDao =& DAORegistry::getDAO('ArticleGalleyDAO');
      $signoffDao =& DAORegistry::getDAO('SignoffDAO');
      $suppDao =& DAORegistry::getDAO('SuppFileDAO');

      $layoutSignoff = $signoffDao->build('SIGNOFF_LAYOUT', ASSOC_TYPE_ARTICLE, $article->getId());
      if ($layoutSignoff->getFileId() == $fileId) {
         $canDownload = true;
      } else if($galleyDao->galleyExistsByFileId($article->getId(), $fileId)) {
         $canDownload = true;
      } else if($suppDao->suppFileExistsByFileId($article->getId(), $fileId)) {
         $canDownload = true;
      }

      $result = false;
      if (!HookRegistry::call('LayoutEditorAction::downloadFile', array(&$article, &$fileId, &$revision, &$canDownload, &$result))) {
         if ($canDownload) {
            return parent::downloadFile($article->getId(), $fileId, $revision);
         } else {
            return false;
         }
      }
      return $result;
   }


with this much leaner version from the \classes\submission\common\Action.inc.php

Code: Select all

   function downloadFile($articleId, $fileId, $revision = null) {
      import('classes.file.ArticleFileManager');
      $articleFileManager = new ArticleFileManager($articleId);
      return $articleFileManager->downloadFile($fileId, $revision);
   }


The above function is called via EditorAction.inc.php that calls SectionEditorAction.inc.php that calls \classes\submission\common\Action.inc.php. I copied the things from the Editor's section, because there everything worked.

However, I did not do anything to the viewFile function that has the same structure as downloadFile, this was just a quick fix for our needs. It looks to me that the Layout editor pages were not upgraded in the same way as the Editor and Section editor pages.

Cheers,
Ales

[jmacgreg]

Hi Ales,

Thanks for this report and the fix! I've replicated and have posted a bug report here: http://pkp.sfu.ca/bugzilla/show_bug.cgi?id=7978. Please feel free to CC yourself to that report for future udpates.

Cheers,
James

[aleskl]

Thanks!

[JasonNugent]

Hi Ales,

There's a small issue with your fix. While it may solve the issue you've found, the validate() function does require two parameters, the first being a $request object. Calling it with just an articleId shifts the argument stack to the left. There are lots of other Handler functions that call validate with two (or even three) arguments, so I'll have a look and see what I can determine from the code.

Cheers,
Jason

[JasonNugent]

The problem seems to be trivial -- the validate() function in LayoutEditorHandler will perform a redirect if the request for the file is not valid, but it does not return true if the request IS valid. It correctly sets the submission, but since there is no explicit return (like there is in the corresponding SectionEditorHandler's validate() method) the calling context has no submission object.

Ales, if you can just add a 'return true;' to the bottom of the validate() method in LayoutEditorHandler, after the submission is assigned, and see if that fixes your issue I will commit the fix.

Regards,
Jason

[aleskl]

How can I test a case when the request for the file is not valid? The fix I did seems to work, files can be downloaded by clicking their links and no errors are recorded in the server log.

I'll add the 'return true;' later and see if it works. As I said before, the fix was just done without any consideration of the big picture, I feel I was just lucky to get the download to work the viewFile function needs to be updated, too, and possibly other functions also - can they be somehow shared, as it is the case for Editor and Section editor functions?

cheers,
Ales

[JasonNugent]

Ales,

The easiest way to check for a non-valid case is to log in as a layout editor, get the URL to the file, make sure it works when you are logged in, and then copy the URL and save it. Then log out or log in as a user without layout editor privileges and see if it fails. (It should).

Regards,
Jason

[aleskl]

I added the 'return true;' to the end of the validate() function in LayoutEditorHandler.inc.php and the file downloading works as it should.

I have checked how the download link copied from the Layout editor works if no user or other user is logged in. It redirects correctly to the login page if nobody is logged in or the user home page if someone else is logged in. But the funny thing is that it also worked before, without the 'return true;' line...

I'm sorry that I can't provide a more complete fix, but all this is too complex for me. I think that the Layout editor pages need to be rewritten in the style of Section editor pages, eg. the validate() function looks different and there are other differences in similar functions between the two user modes.

cheers,
Ales

[JasonNugent]

Ales, the reason why your original fix worked is because your modified call to validate() didn't include the request object. Because of that, the argument stack gets pushed to the left, and validate() then assumes that the articleId, which is supposed to be the second parameter, is null. A whole lot of code gets skipped in that case, including a redirect to the submissions page if the person is not validated. Your other modifications masked these new problems.

Anyway, I am glad that the fix worked. I'll commit a patch to James' bug.

Cheers,
Jason

[aleskl]

Well, this is really strange

I thought that the fix with 'return true;' to the end of the validate() function in LayoutEditorHandler.inc.php should be added in addition to the copying mambo-jumbo I did with the other files, but this is actually the only thing that needs to be done! I have just reverted all other files I have changed (see the posts above) to the stock versions and just implemented the fix from http://pkp.sfu.ca/bugzilla/show_bug.cgi?id=7978. And the download works for the Layout editor! Who would have thought that the fix is so simple?

Thank you very much for your help!
Ales