Default file visibility setting not respected

This affects 8.x and will have to be fixed there first. See process() in /core/modules/file/src/Plugin/Field/FieldWidget.php.

I'm not at all familiar with how this widget works, but I'm guessing that this hidden field in the else statement is a no-JS fallback to make sure that the display value is populated when the file's database record is created.

malc0mn is correct that the widget doesn't respect the field setting.

The most obvious problem is that this always-true value prevents the AJAX-rebuilt form from using the field setting.

A second problem is that if a user has JS turned off, selects a file in the widget, and submits the entity form without clicking the Upload button, then the file field is set to be displayed contrary to the field settings and without giving the user the option to display or not.

Closing as outdated. Please re-open if this is still relevant.

This appears to be a real bug, although now instead of always being checked, the 'display' checkbox is always unchecked when you upload a new file regardless of the setting.

The issue is that when you upload a file the checkbox isn't shown at all, so it's empty, but instead of being empty it should be the default value.

Attaching a patch that fixes it.

#16 Patch has been tested locally in 9.4 Version and the outcome meets as per the requirement.

Applied patch #16 on 9.4.x and it works fine.

Before patch:

After patch;

I have verified the patch #16 and tested it against the Drupal version 9.4.x-dev. The patch works fine and I have added the before and after screenshots for reference.

Tested on Drupal 9.4.5
patch applied successfully.

@Arun Velusamy #20 no reason to provide screenshots when they've been provided in an earlier comment. That is not something that will receive issue credit.

@Jaykumar95 #21 providing a screenshot of a patch applying provides no benefit. We have automated tests such as the ones that run in #16 that will fail if a patch can't be applied. This type of comment also receives no credit.

This should not be set to RTBC. Comment #16 correctly said this needs tests, so automated tests should be added to confirm this is addressed.

@catch
This issue seems to be related to claro theme.
I tried to reproduce the bug on simplytest.me for seven and stark theme.
It seems to be working fine for seven and stark theme.

When I switched the theme to claro theme, it is causing issues.

To reproduce the issue, I am uploading two test-only patches.
On local, test only patch with stark 2272637-24-test-only-stark.patch passes the test cases and same test case is failing for `claro` defaultTheme 2272637-24-test-only-claro.patch.
Not sure what's wrong with claro theme.

@bnjmnm
Would it be possible for you to double-check the behavior in seven and stark themes?

#24 Patch Applied on 9.5.x and it works fine.

I verified the patch #24 on the drupal 9.5.x version. The patch applied successfully and worked fine.

Testing Results: The state of the checkbox in the DISPLAY column: is checked.
Can be moved to RTBC

RTBC+1

Thanks for the patch, I tested #24 on 9.5.x and the following are my findings:

1. The issue summary is clear and steps to reproduce have been given
2. Was able to reproduce the issue on 9.5.x
3. The patch provided applies cleanly and seems to fix the issue. The file visibility setting seems to be consistent
4. Tests have been included and test only patch is failing
5. No issues identified with code review

Marking the issue as RTBC, not including additional screenshots, they have been provided in #26

Thanks for working on this issue.

The feedback in #24 is valid and has not been addressed. If this only fails in Claro (as per the patches provided), the test needs to use Claro, or it isn't testing anything.

I also think asking for a frontend framework manager to weigh in on why this is different between themes -- is this a bug in Claro? Would a different fix be better? Etc.

I looked at Claro and found it has a file-widget-multiple.html.twig but that doesn't look to be doing anything spurious.

However there's a lot going on in claro_preprocess_file_widget_multiple, including looping over the table rows/cells by reference.

I wonder if this bug occurs with that function commented out. If it doesn't then I think the fix should be there rather than in the FileWidget.

mohit++ for the two tests showing the bug exists in claro but not in stark.

Confirmed this is specific to Claro, and I'm 95% sure it is happening in _claro_preprocess_file_and_image_widget. It restructures the render array and I'm not sure it's accounting for this value at all, which I believe is in $element["#value"]["display"]

Based on feedback from #32 and #33 removing the change from file module and moving fix the claro theme.

For now, I have added test case into file module itself.

Oops, forgot to upload interdiff.
Re-uploading patch along with interdiff.

Hiding the patch in #34 and #24 as both of them are not relevant.

Attempting the test case failures and doing code change accordingly.
Both the failing tests are passing on local now.

Looking good, we can remove the frontend framework manager review tag now we've isolated this to Claro.

Just some minor nits on the test

1. +++ b/core/modules/file/tests/src/FunctionalJavascript/FileFieldWidgetClaroThemeTest.php
   @@ -0,0 +1,95 @@
   + * We are intentionally testing it with claro default theme to test the few
   + * changes added in _claro_preprocess_file_and_image_widget().
   

   nit `Claro as the` (capital C and added as the), probably can remove the word 'few' too

2. +++ b/core/modules/file/tests/src/FunctionalJavascript/FileFieldWidgetClaroThemeTest.php
   @@ -0,0 +1,95 @@
   +  protected function setUp(): void {
   +    parent::setUp();
   +  }
   ...
   +  public function testWidgetDefaultVisibilitySettings() {
   

   Not needed anymore?

3. +++ b/core/modules/file/tests/src/FunctionalJavascript/FileFieldWidgetClaroThemeTest.php
   @@ -0,0 +1,95 @@
   +    $title = $this->randomString();
   ...
   +    $title = $this->randomString();
   

   Let's use randomMachineName instead, string includes things like &, > and < and can therefore cause some random failures with CSS selectors (not relevant now, but could become later)

4. +++ b/core/modules/file/tests/src/FunctionalJavascript/FileFieldWidgetClaroThemeTest.php
   @@ -0,0 +1,95 @@
   +    $type = $page->find('css', "input[name='{$field_name}[0][display]']")->getAttribute('type');
   

   Can we use $this->assertSession->fieldExists() instead

5. +++ b/core/modules/file/tests/src/FunctionalJavascript/FileFieldWidgetClaroThemeTest.php
   @@ -0,0 +1,95 @@
   +    $assert_session->checkboxChecked("{$field_name}[0][display]");
   ...
   +    $assert_session->checkboxNotChecked("{$field_name}[0][display]");
   ...
   +    $assert_session->checkboxChecked("{$field_name}[0][display]");
   

   Would be good to also assert the data is stored correctly in the field on the $node, in addition to asserting it shows correctly in the form

6. +++ b/core/modules/file/tests/src/FunctionalJavascript/FileFieldWidgetClaroThemeTest.php
   @@ -0,0 +1,95 @@
   +    // Disable the field settings and ensure that settings are updated on form.
   

   Missing a word here, perhaps on 'the' form?

7. +++ b/core/modules/file/tests/src/FunctionalJavascript/FileFieldWidgetClaroThemeTest.php
   @@ -0,0 +1,95 @@
   +    $node = $this->drupalGetNodeByTitle($title);
   

   I don't think you need to call this again as its the same node?

Addressed the comment #38-point 1 and 6
Keeping it in Needs Work to address the point 2,3,4,5 and 7
Attached patch and interdiff for #38

Addressed the suggestions made in #38.

1. Comment updated.
2. Removed the setUp()
3. Changed to randomMachineName().
4. Replaced with $this->assertSession->fieldExists().
5. Added assert to $node->get($field_name)->value.
6. Comment updated.
7. No code change was done to address this point in the patch. I believe we need to call $node = $this->drupalGetNodeByTitle($title); again after every form submit, to get the latest data stored in the DB for the node.

Please review.
Thanks!

Fixed the phpunit test errors in #40, thanks!

#16 the patch is work for me, and the patch which modify the claro.theme is wrong, if user unchecked the Include file in display, the checkbox will always checked.
and the #16 patch is also work on drupal 10, I think the root cause is display not set default value, not in claro theme.

- Refactoring the implementation little bit so we can easily extend for other themes if required.
- Now we are calculating the values in field widget and setting in claro theme.

Patch in #43 had a few failures.
Addressed those as well.

$node = $this->drupalGetNodeByTitle($title);
We need to call it with each submit with second argument as `TRUE`, so it pulls latest values from the database rather than cache.

Tests are passing on local now.

Fixing the test case failures.
Tests are passing on local now.

Could the issue summary be updated to include proposed solution too.

Per #48

Verified the issue following the steps in the issue summary.
Applied patch #47
Verified that the Include file in display is now defaulted to check.

Thanks everyone for working on this. It's close!

@smustgrave, to get credit for manual testing, you should post before and after screenshots as described in the contributor task doc. Once the patch is updated with my feedback, that will be a good time for someone to provide screenshots, since there have been eight different patch versions and nine months since the most recent screenshots.

1. +++ b/core/modules/file/src/Plugin/Field/FieldWidget/FileWidget.php
   @@ -321,7 +321,12 @@ public static function value($element, $input, FormStateInterface $form_state) {
   +        if (empty($input['fids'])) {
   +          $input['display'] = $element['#display_default'];
   +        }
   +        else {
   +          $input['display'] = $element['#display_field'] ? 0 : 1;
   +        }
   

   I think we need to expand the inline documentation here a little, since the previous comment clearly wasn't enough to write the right code. One inline comment for the if and one for the else, I expect.

2. +++ b/core/modules/file/tests/src/FunctionalJavascript/FileFieldWidgetClaroThemeTest.php
   @@ -0,0 +1,100 @@
   + * We are intentionally testing it with Claro as the default theme to test the
   + * changes added in _claro_preprocess_file_and_image_widget().
   

   This is really nitpicky, but we usually shouldn't refer to "we" (or "you"). It's better to make parts of Drupal the subjects of sentences, or simple passive voice as a last resort.

   Here, we can say:

   The widget is tested with Claro as the default theme to test the [...etc.]

   Also, we should add an @see to that underscore-namespaced preprocess further down in the docblock.

3. +++ b/core/modules/file/tests/src/FunctionalJavascript/FileFieldWidgetClaroThemeTest.php
   @@ -0,0 +1,100 @@
   +   * Tests that visibility settings from field widget are followed on the form.
   

   "Followed" confused me here. "Respected" maybe?

   Also, it's missing the word "the". I understand we're pushing 80 chars, but we can rearrange the sentence a little:

   Tests that the field widget visibility settings are respected on the form.

4. +++ b/core/modules/file/tests/src/FunctionalJavascript/FileFieldWidgetClaroThemeTest.php
   @@ -0,0 +1,100 @@
   +    $type_name = 'article';
   +    $field_name = 'test_file_field_1';
   +    $field_storage_settings = [
   +      'display_field' => TRUE,
   +      'display_default' => TRUE,
   +    ];
   +    $field_settings = [];
   +    $widget_settings = [];
   +    $field_storage = $this->createFileField($field_name, 'node', $type_name, $field_storage_settings, $field_settings, $widget_settings);
   +
   +    $page = $this->getSession()->getPage();
   +    $assert_session = $this->assertSession();
   +
   +    $test_file = current($this->getTestFiles('text'));
   +    $test_file_path = \Drupal::service('file_system')->realpath($test_file->uri);
   +
   +    $this->drupalGet("node/add/$type_name");
   

   Not a hard requirement, just a suggestion: A couple inline comments for each "paragraph" of code here would make it easier for folks to parse the code in their minds.

5. +++ b/core/modules/file/tests/src/FunctionalJavascript/FileFieldWidgetClaroThemeTest.php
   @@ -0,0 +1,100 @@
   +    $title = $this->randomMachineName();
   ...
   +    $title = $this->randomMachineName();
   

   Can we use a meaningful test title in each case rather than randomMachineName()? If we want to test specific characters, we should add tests for those characters. In this case, however, we just want a title, and it can describe the purpose of the item under test. (@larowlan is correct that randomString() here would be very bad, but a string literal is better and more readable than randomMachineName() in this context as well.)

6. +++ b/core/modules/file/tests/src/FunctionalJavascript/FileFieldWidgetClaroThemeTest.php
   @@ -0,0 +1,100 @@
   +    // Now submit the same form and ensure that value is retained.
   

   Very-ultra-nit: There should be a comma after "Now".

7. +++ b/core/themes/claro/claro.theme
   @@ -1382,6 +1382,11 @@ function _claro_preprocess_file_and_image_widget(array &$variables) {
   +  // Handle the default checkbox display after file is uploaded.
   

   Nit: "...after the file is uploaded."

I've just start looking into it, I should be able to provide some feedback to comment #53 soon

Added screenshots before and after and addressed items on #53 comment

Addressed feedback from #53, attached patch with interdiff. please review

Fixed CC failure issue

Thanks @Gauravvvv for applying the interdiff file 47-55 and recreate the patch for me

@Gauravvvv this was tagged for novice/new users. Based on your git posts that would not include you :). Please leave novice issues alone for new users.

But what done is done.

Took the screenshots from #55 and added to the IS.

1. #53.1 was not addressed, oops!
    

2. +++ b/core/modules/file/tests/src/FunctionalJavascript/FileFieldWidgetClaroThemeTest.php
   @@ -7,9 +7,11 @@
   + * The widget is intentionally tested with Claro as the default theme to test the
   

   This is 81 characters. :) Drupal has a character limit of 80 characters on code comments, so this comment now needs to be rewrapped.

3. +++ b/core/modules/file/tests/src/FunctionalJavascript/FileFieldWidgetClaroThemeTest.php
   @@ -20,9 +22,10 @@ class FileFieldWidgetClaroThemeTest extends FileFieldWidgetTest {
   +    // Given the initial data below
   
   @@ -39,8 +42,9 @@ public function testWidgetDefaultVisibilitySettings(): void {
   +    // Filling the form accordingly
   

   Drupal inline comments have to be complete sentences (with periods at the end). So for the first comment, I would change it to describe what's important about the data. Something like:

   Set up an article node with whatever whatever about the display settings.

   The second can be:

   Fill out the form accordingly.

Adressed comment #60

Fixing the patch in a few moments

The patch failed to apply due to trailing whitespace, but since I didn't find any whitespace locally I suspected of an EOL issue. So I just converted CR LF to LF

Looking much better, good catch with the line endings

works for me (Drupal 10.1.5 + Gin Admin Theme)

Saving issue credits. @smustgrave is credited for mentoring (the small issue management tasks themselves weren't quite to creditable, but the mentoring is). @bnjmnm, @larowlan, and myself are credited for code reviews etc.

@ameymudras' earlier RTBC comment is kind of borderline -- I'm going to credit it here since it helped sum up some things that were lost in the noise back in January, but in the future I'd recommend doing your own code review and documenting more specific thoughts or steps you had during the code review. (Also, mentioning that you tested with the steps in the IS and exactly what changed from before to after, in words, is helpful even if the screenshots are already provided -- since no one had actually really said that yet when uploading all the duplicate screenshots.)

@pooja saraah, it looks like you've been able to contribute to numerous core issues already -- nice work! Going forward, it would be good to see you addressing the full review (rather than just the easiest points from it) in order to receive issue credit.

@Gauravvvv, you mostly duplicated @ashley_herodev's work in #55. Normally, I would not add credit for this; however, since @ashley_herodev was having some issues with patch encoding, I'll credit it this once. In the future, duplicating an existing patch or small changes like fixing a reroll will not receive credit since you've already made extensive contributions and aren't really a novice anymore. :)

@Arun Velusamy, @Jaykumar95, @Arti Anil Pattewar, @prasanth_kp, and @Bushra Shaikh do not receive credit since they either duplicated previous manual testing or did not provide sufficient explanation and illustration of their manual testing.

Committed to 11.x, 10.2.x, and 10.1.x as a patch-safe, non-disruptive bugfix. Thanks everyone!

Fixed on commit: some super-small nitpicks that I missed in my previous reviews.

diff --git a/core/modules/file/src/Plugin/Field/FieldWidget/FileWidget.php b/core/modules/file/src/Plugin/Field/FieldWidget/FileWidget.php
index 29abb041432..c97a4d20b38 100644
--- a/core/modules/file/src/Plugin/Field/FieldWidget/FileWidget.php
+++ b/core/modules/file/src/Plugin/Field/FieldWidget/FileWidget.php
@@ -325,7 +325,7 @@ public static function value($element, $input, FormStateInterface $form_state) {
           $input['display'] = $element['#display_default'];
         }
         // Checkboxes lose their value when empty.
-        // If the display field is present make sure its unchecked value is
+        // If the display field is present, make sure its unchecked value is
         // saved.
         else {
           $input['display'] = $element['#display_field'] ? 0 : 1;
diff --git a/core/modules/file/tests/src/FunctionalJavascript/FileFieldWidgetClaroThemeTest.php b/core/modules/file/tests/src/FunctionalJavascript/FileFieldWidgetClaroThemeTest.php
index af2cd7ec6ce..69bbaf3bf1e 100644
--- a/core/modules/file/tests/src/FunctionalJavascript/FileFieldWidgetClaroThemeTest.php
+++ b/core/modules/file/tests/src/FunctionalJavascript/FileFieldWidgetClaroThemeTest.php
@@ -5,7 +5,7 @@
 use Drupal\Core\Url;
 
 /**
- * Tests the widget visibility settings for Claro theme.
+ * Tests the widget visibility settings for the Claro theme.
  *
  * The widget is intentionally tested with Claro as the default theme to test
  * the changes added in _claro_preprocess_file_and_image_widget().
@@ -25,7 +25,7 @@ class FileFieldWidgetClaroThemeTest extends FileFieldWidgetTest {
    * Tests that the field widget visibility settings are respected on the form.
    */
   public function testWidgetDefaultVisibilitySettings(): void {
-    // Set up an article node with all field storage settings as true.
+    // Set up an article node with all field storage settings set to TRUE.
     $type_name = 'article';
     $field_name = 'test_file_field_1';
     $field_storage_settings = [
@@ -69,7 +69,7 @@ public function testWidgetDefaultVisibilitySettings(): void {
     $this->drupalGet(Url::fromRoute('entity.node.edit_form', ['node' => $node->id()]));
     $assert_session->checkboxNotChecked("{$field_name}[0][display]");
 
-    // Disable the field settings and ensure that settings are updated on form.
+    // Disable the field settings and ensure that the form is updated.
     $field_storage->setSetting('display_default', FALSE);
     $field_storage->save();
     $page = $this->getSession()->getPage();
@@ -84,14 +84,14 @@ public function testWidgetDefaultVisibilitySettings(): void {
     $this->assertEquals($type, 'checkbox');
     $assert_session->checkboxNotChecked("{$field_name}[0][display]");
 
-    // Now submit the same form and ensure that value is retained.
+    // Submit the same form and ensure that value is retained.
     $this->submitForm([], 'Save');
     $node = $this->drupalGetNodeByTitle($title, TRUE);
     $this->assertEquals(0, $node->get($field_name)->getValue()[0]['display'], 'The checkbox is disabled.');
     $this->drupalGet(Url::fromRoute('entity.node.edit_form', ['node' => $node->id()]));
     $assert_session->checkboxNotChecked("{$field_name}[0][display]");
 
-    // Now check the checkbox and ensure that it is submitted properly.
+    // Check the checkbox and ensure that it is submitted properly.
     $this->submitForm([
       "{$field_name}[0][display]" => TRUE,
     ], 'Save');

The Needs Review Queue Bot tested this issue.

While you are making the above changes, we recommend that you convert this patch to a merge request. Merge requests are preferred over patches. Be sure to hide the old patch files as well. (Converting an issue to a merge request without other contributions to the issue will not receive credit.)