JSON:API Cannot upload files to public file root (Gets 422 Unprocessable Entity)

This happens because following line in core/modules/jsonapi/src/Controller/TemporaryJsonapiFileFieldUploader.php

$file_uri = "{$destination}/{$prepared_filename}";

On public root directory, this value will be public:///test.jpg which parse_url will return FALSE, and will give violations when validated.

I created an MR that remove that leading slash on public root directory so it will be public://test.jpg.

Wouldn't a ltrim('/') make more sense? Or perhaps implode('/', [$destination, $prepared_filesname]);?

Also this needs a test.

I don't think ltrim or implode is neccessary, because the slash only become a problem for root directory only.

Another approach to avoid parse_url that I can think of is to move the responsibility of handling the slash to getUploadLocation, so in handleFileUploadForField can be like just:

    $file_uri = "{$destination}{$prepared_filename}";

Thanks for all the work you are putting in :)

How about checking if the destination is empty since that is the exact usecase we are fixing here? I really dislike the logic in this code right now:

$file_uri = parse_url($destination) ? "{$destination}/{$prepared_filename}" : "{$destination}{$prepared_filename}";

This reads like: "If the destination cannot be parsed as a destination for whatever reason, just append it in from of the filename". This feels like its way to open to bugsa where the destination or something that parse_url doesn't like.

Perhaps an explicit check on this usecase where destination is empty is best.

No, the problem isn't an empty $destination, destination will never be empty. There are two possible cases:
1. $destination will have value like: public://subdirectory, and the $file_uri will became public://subdirectory/file.ext which is valid.
2. $destination will have a value: public://, and the $file_uri will became public:///file.ext which is invalid, and here is the problem

You are correct, sorry, i'm going to get me some coffee!

This is looking good, thanks for you patience :)

No problem :)

Think that statuschange was unintended ;)

I have similar misgivings as bbrala about the use of parse_url, it feels a bit risky.

Left a suggestion on the MR

Changed as @larowlan suggestion. I think that's much better

Thanks @el7cosmos, that looks way better.

Left one question on the MR

When you look at the tests in that file the rebuild is done every time a setting is changed, so he just followed the code as presented in the tests. I would also assume that it is needed to rebuild then.

Just to see what happens i've removed the rebuild routes from the test. I still think though that since testFileUploadNoExtensionSetting also does this it might be needed. We'll see :)

If this test does not fail, we should make a followup to check if cleanup of the rebuild routes call is possible to optimize the testload a little more.

Is seems you are right. I'll push the minor change and set to RTBC again so you can review @larowlan. I'll also created a follow up issue #3236255: Remove rebuildAll from FileUploadtest where possible.

@alexpott appending trailing slash for $destination will bring us back again to the bug, if the $destination is root directory: public:///file.ext will be invalid, while the valid one should be public://file.ext

@el7cosmos no alex is asking for the converse. I think it will work - if there's no trailing slash add it - but if there is, don't.

That should prevent the /// issue

@larowlan yeah, sorry I missed that part. It will work

Changes are as requested by @alexpott. Thanks for the help. :)

Committed and pushed a8d3f780cc to 9.3.x and 7d047b209a to 9.2.x. Thanks!

Backported to 9.2.x as this is a low risk bug fix.