-
Notifications
You must be signed in to change notification settings - Fork 172
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
[imaging browser] Fix download of JSON, Bval and Bvec files when they are on s3 #8354
[imaging browser] Fix download of JSON, Bval and Bvec files when they are on s3 #8354
Conversation
modules/api/php/endpoints/candidate/visit/image/format.class.inc
Outdated
Show resolved
Hide resolved
modules/api/php/endpoints/candidate/visit/image/format/downloadfile.class.inc
Outdated
Show resolved
Hide resolved
@@ -90,7 +90,11 @@ class Format extends Endpoint | |||
$handler = new Format\Brainbrowser($this->_image); | |||
break; | |||
case 'thumbnail': | |||
$handler = new Format\Thumbnail($this->_image); | |||
case 'nifti': |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
aren't thumbnails intended to be displayed inline?
(It looks like the code already had an attachment Content-Disposition, so I guess it doesn't matter..)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, the thumbnail is mainly used to be displayed in the imaging browser but it was already available for download in the API and that is the file I used to base all the other classes so I kept it. @driusan does it answer your question?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the $_image
instance variable is not needed anymore. All the other places are using $this->_info
modules/api/php/endpoints/candidate/visit/image/format/downloadfile.class.inc
Outdated
Show resolved
Hide resolved
modules/api/php/endpoints/candidate/visit/image/format/downloadfile.class.inc
Outdated
Show resolved
Hide resolved
modules/api/php/endpoints/candidate/visit/image/format/downloadfile.class.inc
Outdated
Show resolved
Hide resolved
modules/api/php/endpoints/candidate/visit/image/format/downloadfile.class.inc
Outdated
Show resolved
Hide resolved
modules/api/php/endpoints/candidate/visit/image/format/downloadfile.class.inc
Outdated
Show resolved
Hide resolved
modules/api/php/endpoints/candidate/visit/image/format/downloadfile.class.inc
Outdated
Show resolved
Hide resolved
modules/api/php/endpoints/candidate/visit/image/format/downloadfile.class.inc
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Xavier is happy as well
I'm so happy to hear that @xlecours! |
if (!$info->isFile()) { | ||
error_log('Thumbnail not found'); | ||
if (!$this->_info->isFile()) { | ||
error_log("$this->_file_type file not found"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be using the PSR logger, not error_log
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@driusan could you elaborate on how to do that? Sorry I am not very versed in the PSR realm apparently...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NDB_Page includes a $this->logger which implements PSR-3 (https://www.php-fig.org/psr/psr-3/) which honours the config settings from the LORIS config. I think this endpoint bypasses NDB_Page, so you might need to copy the initialization from the NDB_Page constructor and then use ie $this->logger->notice("something")
(or the appropriate log level)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would involve changing src/Http/Endpoint to use Psr\Log\LoggerAwareTrait;
and add somehting like this in process.
$loris = $request->getAttribute('loris');
$loglevel = $loris->getConfiguration()
->getLogSettings()
->getRequestLogLevel();
$this->logger = new \LORIS\Log\ErrorLogLogger($loglevel);
if (!$info->isReadable()) { | ||
error_log('Thumbnail exists but is not readable by webserver'); | ||
if (!$this->_info->isReadable()) { | ||
error_log( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same
if (!$info->isReadable()) { | ||
error_log('Thumbnail exists but is not readable by webserver'); | ||
if (!$this->_info->isReadable()) { | ||
$this->logger->notice( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should probably be logged at the level of "error" log level.
See the different log level definitions in PSR-3. https://www.php-fig.org/psr/psr-3/
The database referencing a file that the server can't read seems more severe than a notice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed to error
I am even more happy now. :) |
… are on s3 (aces#8354) This fixes the error 404 users had when they tried to download a JSON, Bval or Bvec file hosted on S3 from the imaging browser. Fixing this bug also resolved aces/HBCD#227 as new API endpoints needed to be created for those files.
Brief summary of changes
This fixes the error 404 users had when they tried to download a JSON, Bval or Bvec file hosted on S3 from the imaging browser.
Fixing this bug also resolved https://github.com/aces/HBCD/issues/227 as new API endpoints needed to be created for those files.