Default file visibility setting not respected

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!

The last submitted patch, 40: 2272637-40.patch, failed testing. View results →

Fixed the phpunit test errors in #40, thanks!

The last submitted patch, 42: 2272637-42.patch, failed testing. View results →

#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.

- 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.

The last submitted patch, 45: 2272637-45.patch, failed testing. View results →

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 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. (@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 @@
   - * We are intentionally testing it with Claro as the default theme to test the
   + * The widget is intentionally tested with Claro as the default theme to test the
     * changes added in _claro_preprocess_file_and_image_widget().
   

   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 and @larowlan 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.)

Automatically closed - issue fixed for 2 weeks with no activity.