[HttpFoundation] Add UploadedFile::getClientOriginalPath() to support directory uploads

[danielburger1337]

Q	A
Branch?	7.1
Bug fix?	no
New feature?	yes
Deprecations?	no
License	MIT

This removes the "hotfix" #42112 and now supports "full_path" PHP file uploads.

Since PHP 8.1 is now the lowest allowed version of PHP, this shouldnt cause any problems.

---

Although I'm quite certain that I understand the messy FileBag code, I'm very concerned for the potential impact this change might have because of the widespread use of http-foundation.

To the best of my knowledge, this won't break Symfony BC (yet) because this is all done by an optional argument in the constructor of UploadedFile (this should eventually be replaced by a mandatory argument though).

Also, I'm not quite happy with the tests. Since this is only a "passthrough" of the PHP value, this shouldnt be a huge problem.

Someone should definitly go through this change very carefully. The tests pass and I also threw some "real world" application tests at it and everything seems to work just fine. But because of the nature of HttpFoundation and its intangled mess, I can't guarantee that I accounted for every possible or even simple edge case.

---

Though it would be nice to still see this in 7.0, I know that it will probably be pushed back to 7.1.

[nicolas-grekas]

That's a new feature for sure so for 7.1
What about also adding support for the webkitdirectory attribute in the Form component?

[danielburger1337]

What about also adding support for the webkitdirectory attribute in the Form component?

I haven't contributed to the form component yet, so I will check it out. Should be fairly simple though.

[danielburger1337]

I just realized that merging this should break many legacy test cases where a file upload is manually submitted.
See FormTypeTest as an example. These submit the mock file upload without the "full_path" option.

Even though it is more work than simply making this option optional in FileBag, these tests SOULD be fixed because they do not adhere to the default PHP behaviour and in my opinion the FileBag should not account for that. It would also require to rework the entire logic of the FileBag.

[alexislefebvre]

That's a new feature for sure so for 7.1 What about also adding support for the webkitdirectory attribute in the Form component?

See this answer from stof: #52528 (comment)

[nicolas-grekas]

Thanks for working on this. I played a bit with the code+feature, here are my comments.

[nicolas-grekas]

I think the failure on PsrHttpFactoryTest::testUploadErrNoFile is related, maybe the ones of Form also.
Can you please have a look?

[danielburger1337]

Yeah, this failure is related. Like I mentioned here, pretty much every test that submits a file upload manually will fail now and will have to be migrated because they don't include the full_path key.

This is standard PHP behaviour since 8.1 and thats also the minimum required PHP version now. So I don't really see the "point" in supporting these legacy cases and they should be updated to include full_path.

If it is the teams desire to keep this legacy behaviour, the logic of FileBag has to be heavily modified because it requires that every possible "file key" in $_FILES is included.

IMO fixing all of these test cases should be a separate PR which I'm willing to create after this is merged.

[stof]

I think we could support it more gracefully by using $file['full_path'] ?? $file['name'] so that passing a file without a full_path key still works like before.

However, it might be great to update the tests emulating file uploads to include this full_path key.

[danielburger1337]

I had a quick look at where files are submitted manually and what tests will have to be fixed:

• PsrHttpMessage (Bridge)
• Form

It looks like these aren't that many and we can literally just copy the value name and set it to full_path.
I changed the tests I found locally and the tests now pass. Should I add them to this PR?

BrowserKit/DomCrawler also submits files but their logic seem to be independant.

---

I also discovered NativeRequestHandler in the Form component includes basically just a copy of FileBag and probably should be updated to support directory uploads as well. Add to this PR or new?

[stof]

I think updating NativeRequestHandler in this PR makes sense as well.

[danielburger1337]

I think we could support it more gracefully by using $file['full_path'] ?? $file['name'] so that passing a file without a full_path key still works like before.

Adding something like the following in FileBag should in theory work:

protected function convertFileInformation(array|UploadedFile $file): array|UploadedFile|null
{
    if ($file instanceof UploadedFile) {
        return $file;
    }

    if (!\array_key_exists('full_path', $file) && \array_key_exists('name', $file)) {
        $file['full_path'] = $file['name'];
    }
        
    // ...
}

This should then both work with "fixed file arrays and "normal" ones.

[nicolas-grekas]

I pushed an alternative implementation that should fix BC issues.

[fabpot]

Thank you @danielburger1337.