Required flag on file forms breaks on validation

Backlink to open flexinode issue: http://drupal.org/node/42762

(21:46:24) vertice:  the problem is that during the upload process, the stuff gets removed OUT of the form system
(21:46:28) vertice:  and gets stuffed in the session
(21:46:36) vertice:  and every file api module does this on it's own

This is not the place neither the time to fix file API. Everybody on its own. I am outta here. If the change gets in, I will doc the change.

I am not too happy about it, but unless somethign better comes up, I will commit it.

Maybe we should simply document that you can't use required with file fields. Also, Ber coul dpass an attribute to his home-brew validation routine.

Since this issue does not affect core, I'd like to not patch core, but to simply update the docs. Opinions?

Like the previous posters, I'm not crazy about the idea of introducing such a hack into core.

I may have found a potential workaround, at least as far as flexinode goes (but should be generally applicable).

Can you guys take a look at http://drupal.org/node/42762, #12. It seems to work fine... Am I missing something that makes this a bad approach?

It might be better to ensure that that type of approach is supported via forms system, rather than just bypass required checking for file fields.

What do you think?

I'm OK with this patch for 4.7.0. We can make it work properly for 4.7.1 IMO

We agreed that the solution is that I am doccing right now that #type file can't be required.

I marked http://drupal.org/node/64984 as a duplicate. There is a patch on that issue.

Patch does not apply and a conversion of the patch does not seem to work.

Is a patch applied for 4.7? It seems that is should be, as flexinode is still better at handling files and many sites are using it. This seems like a sad oversight... apply a patch to core that fixes this, please (easy for me to say, since I'm more of a bug hunter than a bug fixer). Just throwing my support at this as loudly as possible.

This is still an issue in 5.x and, presumably, HEAD.

This prevents programmatically creating node content with drupal_execute on imagefields, for example, when the field's #required property is set to TRUE.

...still an issue.

I'm sure it goes without saying, but stating that the #required setting is not allowable for file types is a pretty lousy solution.

it may be a lousy option to say #required is not allowable for file types, but at the moment #required doesn't work for them and I've just spent hours trying to figure out where in my script I was going wrong not getting it working. I had already done my own workaround (also suggested above) of having my own #validate function and doing stuff in there, and not having the field #required, but the fact that the docs say #required is allowed is very confusing. For now at least, the docs must be changed to make #required not an option for 'file' form fields, otherwise other coders will face the same headache in figuring out where they've gone wrong in implementing it.

@jakeg: Please update the docs. Anyone with a CVS account may do so. The file's in contributions/docs/developer/forms_api_reference.html, I believe.

@webchick: thanks, I didn't realise/think I had access. Have updated the docs:
http://cvs.drupal.org/viewvc.py/drupal/contributions/docs/developer/topi...

Subscribing

Patch from post #13 appears to be working in my 5.7 instance.

Ha, this was "fixed" 3 years(!) ago in #64984-4: File Uploads fail when field is set to "required", but it never made it into core. Sad, sad. Here's a reroll of my old patch, not yet tested, but just putting this on the radar.

Updated patch that has been tested.

@jakeg: thanks for fixing up the documentation, it was really important IMHO and I have experienced this headache so I totally agree with #18

subscribing

seems like we could use a unit test for this...

Fixes a bug that has pestered since FAPI 1.0. A test would be nice but squashing the bug is more important.

i'd disagree since the whole point of the test is making sure it says unbroken.

Wow!! Great to see this bug fixed! But I agree that a test is mandatory.

Also, is there by chance a nicer way to get this value than $elements['#parents'][0]? that's not really at all intuitive.

On testing, the previous patch went too far and didn't throw an error when a required file field was actually missing a file. Updated patch, including a test.

@Scott - the bot tested today and this patch will not apply cleanly :(

Updated patch for most recent 7.x-dev release.

Is this possible in 6.x?

Increase priority. Patch looks good.
Don't get why it still not fixed for years

Here's the patch re-roled against the newest drupal 7 dev.

This is going to need to be fixed in Drupal 8 first.

I re-roled the patch off of drupal 8 and hopefully fixed the simple test issue.

So we have a test for this already in file.test for the file module so I actually assume this is fixed in core....as a result closing as this is fixed.

@marcingy: how about D6 ?

Moving version then.

untagging for backport to D7.

No, this is fixed for file_managed fields, but not for #type => 'file'. This is still a valid bug in D8 and below.

Yeah the #type = managed_file element has mitigated this problem significantly, since most uploads would want to use the more advanced managed_file element anyway (who wouldn't want an AJAX-upload and progress bar?) I personally don't think this is a "major" issue any more and it's not affecting nearly as many sites now since File module is available in core to provide the majority use-case. This seems like just an inconvenience now.

For some uses managed_file is not appropriate, but yeah I agree with the bump down to normal.

#39: file-field-required-59750-39.patch queued for re-testing.

#42: file-field-required-59750-42.patch queued for re-testing.

This patch doesn't keep in mind #name changes. In a file input it's very useful to be able to name it to "files[photos][]" so you can upload multiple. (Yes, HTML has been doing that for a couple of years now.) It also doesn't handle files in a fieldset with #tree => TRUE.

I've solved this differently: http://drupalcode.org/project/value_is.git/blob/refs/heads/7.x-1.x:/valu...

I just got the exact same error in 7.43 as well.

This needs a re-roll for 8.9.x.

A file form, that is required will always fail.

Does this mean a form with a file field that is required?

Discussed this with @quietone

\Drupal\file_test\Form\FileTestForm provides a sample form and \Drupal\Tests\file\Functional\SaveUploadTest tests it.

If we make the element there #required we should yield the bug.

I think we can solve this differently now that we have plugins for elements.

We can put a valueCallback on the element and populate it with the temporary file name from the request's file bag if a file with that name has been uploaded.

Thanks to @larowlan for proving the sample code for this in #bugsmash. With that I have made a rough patch as well as a fail patch. There is no interdiff because this is a different approach.

This makes the file field required for all the tests using FileTestForm. I haven't looked yet but is there a still a test without the field being required?

Ah, this should be on 9.3.x

+++ b/core/lib/Drupal/Core/Render/Element/File.php
@@ -72,4 +75,23 @@ public static function preRenderFile($element) {
+   * @param array $element
+   * @param mixed $input
+   * @param \Drupal\Core\Form\FormStateInterface $form_state
+   *
+   * @return mixed|null

Should be @inheritdoc

Yeah an probably better

+++ b/core/modules/file/tests/file_test/src/Form/FileTestForm.php
@@ -24,6 +24,7 @@ public function getFormId() {
       '#type' => 'file',
+      '#required' => TRUE,
       '#title' => t('Upload a file'),

Yeah and probably better/easier to split the required field off into its own form that you can use just for testing that

OK.

I removed the addition of required in FileTestForm and added a new test form. Instead of making a new test file I added, testRequired, to the existing SaveUploadTest.php although I am not sure if that is the best way to do this.

Not sure if there is a better way, this now checks that the files variable is of the UploadedFile class before treating it like an object.

Restore the test that $all_files is not empty. Don't know why I removed it. Added comments to the test.

Changing to use NULL coalesce on the file variable.

Test looks good and the problem is fixed.
I'm adding in the IS the steps to reproduce.

I'm adding credit to larowlan who provided the solution, and support, in bugsmash.

+    $image2 = current($this->drupalGetTestFiles('image'));
+    $edit = ['files[file_test_upload]' => \Drupal::service('file_system')->realpath($image2->uri)];

Nit, should this be image instead of images?

The new test uses code from SaveUploadTest::testNormal and SaveUploadTest::testDuplicate and both of those use $image2 for the second image that is uploaded. For example, https://git.drupalcode.org/project/drupal/-/blob/9.3.x/core/modules/file.... I thought it best to kept to the existing style in the file.

Happy to change it though, shall I?

I think with it as image2 our future selves might wonder what happened to image 1, so its probably worth fixing for future comprehension sake

can be fixed on commit though

I'm going to ask chx on slack if he can have a look at this to make sure we're not doing anything wrong in the value callback

Discussed this with @chx on slack and he pointed out a few minor code-style changes

But we thought that using #value for this might be risky, because file elements have never had a value.

So I've come up with a different approach, patch to follow

Something like this perhaps

phpcs

So I've thought about this some more and I think the approach of unsetting #required is wrong - as the tests show - because it means that the required state is not available to the theme system

So I think we should go back to the approach from #83, here's that patch plus some minor changes that chx identified on slack

Because this will mean that file elements now have a #value, we'll need a change record.

Updated the issue summary while here.

Thinking about this some more, should we put the UploadedFile object in the value instead of just the file name?

Ok, those fails have convinced me, because with #multiple this needs to be an array.

So I'll make the #value an array of UploadedFile objects

Fix unused import.

Ok, that fail seems to point to somewhere we're using #value for a file element, in theme settings. So this is good, we might have found a possible regression

So theme settings form has a #maxlength on a file upload field, which obviously makes no sense.

Removing it.

Added a change record.

"Why is this in my dashboard? I don't remember running into this recently that's just the way things work. ... Oh, I commented _15 years ago_!"

This isn't so much a review of the patch but an observation about the form api.

+  public static function valueCallback(&$element, $input, FormStateInterface $form_state) {
+    if ($input === FALSE) {
+      return NULL;
+    }
+    $element_name = implode('_', $element['#parents']);
+    $uploaded_files = \Drupal::request()->files->get('files', []);
+    $uploaded_file = $uploaded_files[$element_name] ?? NULL;

This smells bad. Why do we need to look at the request? Makes something already difficult to test(I'm waiting on a test run for something I'm trying to write while writing this) and likely makes it harder. I know the answer is not the fault of this patch its just the only option. Should the FormState give us access to files the same way it does values? Probably. Is that likely to be hacky and ugly which is why it doesn't exist? Again probably but worth the effort in another patch/follow up I think.

$element_name = implode('_', $element['#parents']);

This part was also confusing until I confirmed it matches logic duplicated in a couple places in file.module. Again not a problem with this patch but highlighting maybe some debt in FormApi's file handling that could be addressed. Again in formstate's interface?

This isn't going to be backported to 6 or 7 at this point I wager so a little tag cleanup as well.

Patch looks solid and skim of tests looks good. Wager this is good to go but smell makes me feel like it needs more manually testing than I can do in this review so leaving an rtbc to someone else or another review.

Thanks for the change record @quietone - Looks good. I'll remove the needs CR tag now.
I'm going to have a crack at a manual test, but it's been a while since I've done this. Wish me luck ;-)

Reading the issue thread here is like a who's who of Drupal!
It would be amazing to get this one smashed. :)

Add manual testing to the tasks.

Not that anyone asked for this... But I'm working on a D7 project now that needs this, so here's a reroll of #42.

Oops...

Restoring "Needs review", since my D7 testing bumped it back to "Needs work"...

Adding tag.

I have an issue with a part of the patch :
implode('_', $element['#parents'])
This part does not work for me because i use sub index in my form for file type (#tree = TRUE).
If i replace this by :
array_shift($element['#parents'])
It works for me. What do you think about this?
https://www.drupal.org/files/issues/2021-11-30/59750-112.patch

delete coment

Add @Mir4 purposed patch in comment #114.

The patch in #116 still applies to 9.5.x.

Adding interdiff.

+++ b/core/lib/Drupal/Core/Render/Element/File.php
@@ -72,4 +79,23 @@ public static function preRenderFile($element) {
+    $uploaded_files = \Drupal::request()->files->get('files', []);

Do we have an issue to move files to form state instead of calling this?

I think this needs a test-only patch.

Added a test-only patch.

Updating for 10.0.x and adding interdiffs. Interdiff.txt is between the success and fail patch. The other interdiff is highlight the changed suggested in #114 since there was not an interdiff in #116.

The patches

1. `59750-123.patch` #123
2. `file-field-required-59750-114.patch` #114
3. `59750-101.patch` #111

could be applied via composer but the feature still not work
Attached the image for your reference

@mrddthi, thank you for your interest in this issue. What patch did you test? Your comment lists three patches.

I decided to retest with #123 on Drupal 10. It works fine for me, I even managed to make a screencast.

Patch applies to 9.5.x

Tested using the form api sub module from the examples module

Added the file type to a simple form confirmed the bug.

Applied the patch and the issue was resolved.

On Drupal 9.5.x

Test failure in unrelated file

1) Drupal\KernelTests\Core\Entity\ConfigEntityQueryTest::testCaseSensitivity
Failed asserting that actual size 4 matches expected size 3.

/var/www/html/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:121
/var/www/html/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:55
/var/www/html/core/tests/Drupal/KernelTests/Core/Entity/ConfigEntityQueryTest.php:734
/var/www/html/core/tests/Drupal/KernelTests/Core/Entity/ConfigEntityQueryTest.php:658
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestResult.php:726

then I found #3101130: ConfigEntityQueryTest::testCaseSensitivity can randomly fail. Maybe related?

Starting tests again.

#123 seemed to pass and test-only patch failed.

Moving back as it seemed to be a random failure.

Committed and pushed a5b44e8f74 to 10.1.x and 964a3f3aeb to 10.0.x and e3930ea53e to 9.5.x. Thanks!

Congrats to bugsmash for resurrecting and fixing this one. Always fun to commit a 5 digit NID :D!

Maybe I missed this, but did the D7 patch in #111 also get committed?

@PapaGrande the process for backporting this to D7 would be to open another issue against Drupal 7 for this. I think this change is unlikely to make it D7 at this point but that is for a D7 maintainer to decide on the new issue. Also the patch in #111 is a very different solution from the final patch.

Wow!!! Amazing. Thanks everyone :)

This was tested manually, removing tag.

Just a note that this commit actually introduces a pretty big bug when the File form element has #multiple set to false.

Attempting to cast a complex object like this:

$uploaded_file = new \Symfony\Component\HttpFoundation\File\UploadedFile('/real/path.txt', 'path.txt', 'text/plain');
var_dump((array) $uploaded_file);

Will result in just the UploadedFile properties being outputed as the output array (https://stackoverflow.com/a/4345609).

The actual return value should be something along the lines of:

---    return (array) $uploaded_file;
+++    return is_array($uploaded_file) ? $uploaded_file : [$uploaded_file];

And only obvious when trying to read the file value from the form state.

yes, having some issues.

I have to set #multiple set to false, otherwise, I will get the error below

Exception: Serialization of 'Symfony\Component\HttpFoundation\File\UploadedFile' is not allowed in serialize() (line 157 of /app/docroot/core/lib/Drupal/Core/Batch/BatchStorage.php).

@codebymikey did you happen to open a new issue for that regression?

@larowlan yes, the regression issue has already referenced this issue, but I'll add a direct relation now.

thanks 🙌