[API] Add dicom upload
Brief summary of changes
This is a port of #6161, which contains a description of the changes. Since the old PR has diverged quite a bit from the main branch, and that its author no longer works here, I found it easier to copy the changes on the current main branch and open a new PR rather than to rebase the PR. While porting, I only rewrote the old PR code whenever static analysis or practical stests showed that it was no longer in sync with the main branch (or when my IDE removed trailing spaces).
Testing instructions (if applicable)
- Test the four new API routes, described in section 5 of the documentation of the API v0.0.4.
Notes for existing projects
This PR requires to run the following SQL patch SQL/New_patches/2024-03_19-MRIUploadServerProcessRel.sql.
I must say I am not sure what I am supposed to do regarding the failed tests on files that I did not modify in the PR. Any direction @driusan (or someone else) 🤔.
Line php/libraries/FilesDownloadHandler.php
71 Parameter #1 $value of function strval expects
bool|float|int|resource|string|null, mixed given.
Line php/libraries/SiteIDGenerator.php
81 Negated boolean expression is always false.
💡 Because the type is coming from a PHPDoc, you can turn off this
check by setting treatPhpDocTypesAsCertain: false in your
./test/phpstan-loris.neon.
297 Parameter #1 $value of function strval expects
bool|float|int|resource|string|null, mixed given.
377 Parameter #1 $value of function strval expects
bool|float|int|resource|string|null, array<int,
int|string>|string|null given.
387 Parameter #1 $value of function strval expects
bool|float|int|resource|string|null, array<int, int|string>|string
given.
400 Parameter #1 $value of function strval expects
bool|float|int|resource|string|null, array<int, int|string>|string
given.
Hum, yeah, I already saw that.
I was just suprised to see that the errors are marked as originating from FilesDownloadHandler.php and SiteIDGenerator.php, which are not modified by this PR. I guess I must call these files from elsewhere and that the location reporting is just imprecise.
I'll tackle this tomorrow probably.
I've looked at the static errors in more details, it appears to be mainly due to an upgrade of phpstan, which changes the typing of string conversions with strval and (string) from mixed -> string to (bool|float|int|resource|string|null) -> string. I still don't know why this PR triggers the error and not other ones (it did trigger on my main branch though), but at least the errors are clear.
However, I find the code that triggers the errors to be quite cryptic, with some lines that seem obviously wrong (at least according to typing). If someone who knows this code better than me could look into it, it would be best. In the meantime, I'm adding this to the next meeting.
I still don't know why this PR triggers the error and not other ones (it did trigger on my
mainbranch though), but at least the errors are clear.
Probably because composer update was run, which triggered an update of the static checker.
Ok it seems the linting errors are ~~fixed~~, silenced. @laemtl had noticed that the errors were due to a composer update on my end. The errors should still appear in any PR that updates composer and should be fixed eventually. However, this can have its own issue and be another PR.
@ridz1208 Ok I finally finished updating and re-testing this PR. Generally it seems to work. However, if the pipeline fails during the latter stages, the DICOM archive previously inserted does not have its session initialized. This should be fixed with the new DICOM archive PR, but I don't expect it to be merged before at least a few weeks.
Please remove all the unrelated changes (whitespace, variable name changes, etc). If necessary you can send those in another PR. It is not feasible to review this PR with so many unrelated changed mixed together.
I replied to all the comments and applied the suggested changes.
I reverted the breaking changes to the API including floats / strings and naming. Back when I wrote them, I thought it was part of the original PR and not something that was already in LORIS.
I made the terminology more consistent by using $VisitLabel everywhere in the DICOM part (section 5) of the API documentation.
I also disabled my trailing spaces auto-removal and removed the associated changes.
Ready for review @cmadjar @driusan