Submission file with orphaned uploader user ID causes fatal error on submission list loading

[Vitaliy-1]

Describe the bug
As a part of the migration process, in submission_files table, uploader_user_id is set to NULL if it's pointing to a non-existing user. This causes a fatal error, see: Vitaliy-1/JATSParserPlugin#80

The problem occurs when the submission file schema tries to get a user object from the uploader user ID:

pkp-lib/classes/submissionFile/maps/Schema.php

Line 179 in 3a4dce7

$user = Repo::user()->get($submissionFile->getData('uploaderUserId'));

What application are you using?
OJS main branch

[Vitaliy-1]

PR: #8501
tests: pkp/ojs#3671

[Vitaliy-1]

@asmecher, can you review this PR?

[asmecher]

@Vitaliy-1, this is happening because we're getting stricter about type hints (on the PHP side) and null data (in the SQL side), where we previously were very vague on both -- a 0 ID and an empty string $someId variable and a null were all considered equivalent. Now that we're getting stricter we'll probably have to solve this in a lot of places, so I'm looking for a more succinct syntax than this PR proposes.

The Repo::submission()->get(...) function already is a nullable return, which happens if the supplied ID doesn't exist, so calling code is already responsible for error checking. With that in mind, how do you feel about just using a hard (int) cast? For example:

$user = Repo::user()->get((int) $submissionFile->getData('uploaderUserId'));

If $submissionFile->getData('uploaderUserId') is null, then it'll get cast to a 0, which we know will not exist in the database -- thus resulting in the expected null return from get(...).

An alternative would be to change the get function in the Repo classes so that they accept a nullable ID. I kind of like this because calling code to get(...) functions already needs to handle null returns for cases where the specified ID doesn't exist. It is also compatible with Eloquent's find, which does not specify a strict key type. But it is a little weird to accept a null ID.

Neither of these ideas will solve our likeliest use case for the general problem, where the ID is being derived from a user-specified string ($request->getUserVar('someId')) which may well be a non-numeric string. Casting e.g. userSpecifiedNonNumericString to an int will result in 1, which is a valid but wrong database ID. For that use case, I'd suggest we look at adopting filter_var with FILTER_VALIDATE_INT. But that should be done at the middleware/framework level. I suggest we continue to move towards better Laravel adoption, which will give us access to https://laravel.com/docs/9.x/validation#rule-integer. But that is outside the problem you're solving here.

(See also: https://wiki.php.net/rfc/nullable-casting)

[asmecher]

@Vitaliy-1, assigning you since this is already underway!

[Vitaliy-1]

@asmecher, are you ok with the solution proposed above? I've also updated the submission file schema JSON to make uploaderUserId nullable, forget to do this initially.

[asmecher]

are you ok with the solution proposed above?

Yes, please go ahead!