ImageWidget::validateRequiredFields() produces an error message if an image field is hidden with hook_entity_field_access()

Same is the case with if (!in_array('file_managed_file_submit', $form_state->getTriggeringElement()['#submit'])) { as as per api, getTriggeringElement() can also return null.

Fixed the issue for me.

We should add test coverage so we don;t break this in the future.

Hello,

Any example/suggestion on how to implement this test ?

Thanks,

Thanks @joachim for the suggestions

This is a first try at the test.
For now, it passes even without the fix, so there is still some work to be done,

I can confirm that the patch from #12 addresses the problem, thanks for patching.

Stumble on this but my setup was little different. Have two image fields on entity and hiding both of them on form with form alter and ['#access'] = FALSE. On form submit one of image fields was giving above PHP warning but the other image field was fine.

While debuging i've noticed that problematic field was failing with above PHP warning while trying to validate alt field of image. This does not make any sense because alt field was not enabled for this field. Then finally noticed next two things in debugger

$form['field_card_image']['widget'][0]['#alt_field'] = false
$form['field_card_image']['widget'][0]['#alt_field_required'] = true

And when checked field setting indeed alt field was disabled but required setting was enabled. Checking required setting to disabled fixed PHP warning.

This patch will also fix this situation.

Hello,

I had time to look over this again.
I'm having trouble in choosing the assert to do.
Because the error message is not displayed in the page, only in the log, i can't check for text.
I tried to use assertErrorLogged(), but i found no example in core where this kind of test is done.

Any suggestions ?
Thanks,

I've finally managed to add a proper test for this.

I had a related issue that produced the same error, and the patch in #18 that passes testing worked for me.

Should the image field be marked non-required in configuration instead? We should definitely handle this case properly, but I'm not sure effectively eating the error message is the right way.

@catch Do you mean setting field_image for the Article content type provided by core to a be non-required field? If so, it currently isn't a required field. This error is triggered whether a field is required or not.

Yes, I've seen this too. There are valid cases for hiding image fields, e.g. to display a limited user form after a password-login.

Per #23, this also happens on non-required fields. Setting back to RTBC, will also check if the patch still applies.

Hello,

I've seen the patch from #18 still applies.
Is something else required here ?

Thanks,

Patch in #18 applies cleanly for me in D8.4.5 & the error message is gone.

I think #24 wanted to change the status as well.

#18 doesn't solve the problem for not required fields.
Step to reproduce:
1-Create a content type with an image field (not required) in a bilingual site
2-Do not set this field as to be translated
3-In "admin/config/regional/content-language", check "Hide non translatable fields on translation forms" for this content type
4-Open the edit form of a translated node (Thus, without the image field... because this field is hided as not to be translated)
5-save the form
=>You get the error

=>I got the error only once... the patch seem o,k

Note: applying #18 on 8.5, I got a 40 lines offset

patching file core/modules/image/src/Plugin/Field/FieldWidget/ImageWidget.php
Hunk #1 succeeded at 301 (offset 40 lines).
patching file core/modules/image/src/Tests/ImageFieldAccessHiddenTest.php
patching file core/modules/image/tests/modules/image_access_test_hidden/image_access_test_hidden.info.yml
patching file core/modules/image/tests/modules/image_access_test_hidden/image_access_test_hidden.module

The test and fix are looking good. Just one tricky thing to resolve.

+++ b/core/modules/image/src/Tests/ImageFieldAccessHiddenTest.php
@@ -0,0 +1,81 @@
+    $this->assertNoErrorMessage($error_warning);

This is testing for the absence of something - always very tricky and prone to false positives and breaking later due to other changes. Can we extend the test to uninstall image_access_test_hidden and check that the required message is present if you try to re-save the node.

+++ b/core/modules/image/src/Tests/ImageFieldAccessHiddenTest.php
@@ -0,0 +1,81 @@
+    $error_warning = [
+      '%type' => 'Warning',
+      '@message' => 'array_key_exists() expects parameter 2 to be array, null given',
+      '%function' => 'Drupal\error_test\Controller\ErrorTestController->generateWarnings()',
+      '%file' => drupal_get_path('module', 'error_test') . '/error_test.module',
+    ];

Also then we don't have to assert this. This should cause an error in our test suite if it occurs. And we shouldn't have to extend ErrorHandler test - which is a bit odd.

#18 doesn't solve the problem for not required fields.

I think there's some confusion about what we're fixing here.

This patch supresses a PHP notice - it's a simple type-checking patch. Want to avoid regression in the future? Type-check your inputs.
(It's not feasible to add test coverage for all possible scenarios in which we fixed PHP notices and warnings.)

This patch does not address the underlying issue, where a hidden field is required, and thus the user cannot submit the node form.

So, there's 2 paths forward in this thread, imo:
1. Fix the PHP notice; don't bother with additional test coverage; open a new issue (or find an existing one) about fixing the hidden-required field conundrum
2. Address the conundrum here, and create test coverage for it

Thoughts?

Oh wow, this was tricker than expected.

I updated the test and converted it into a phpunit test.

As suggested, I changed it to explicitly assert the success condition with/without the field visible. However, I was fighting with the fact that even if the field is hidden, there's still form-level validation of ManagedFile that is failing the validation.

I realized that there is a bug in FileWidget that doesn't trigger that validation if the cardinality is not 1 or unlimited and I'm now using that bug to be able to better test this problem here. I created #3011744: File/image widget validation is inconsistent based on cardinality/visibility to improve it further.

Hurrah Berdir for digging into this!
RTBC+1 for #33

Can this (trivial, besides the test) patch get in first, and #3011744 take responsibility for updating the test once the cardinality/validation bug is fixed?

> RTBC+1 for #33

Someone actually needs to set this to RTBC for that to count ;)

And yes, I split that other issue up so we can get this in, with the slightly weird test coverage and then clean it up in that issue.

Waiting for someone else to set RTBC since I jumped in late.

Test now cover regression as well

The method where the fix is confuses me. Here's the full code after the change...

  /**
   * Validate callback for alt and title field, if the user wants them required.
   *
   * This is separated in a validate function instead of a #required flag to
   * avoid being validated on the process callback.
   */
  public static function validateRequiredFields($element, FormStateInterface $form_state) {
    // Only do validation if the function is triggered from other places than
    // the image process form.
    $triggering_element = $form_state->getTriggeringElement();
    if (empty($triggering_element['#submit']) || !in_array('file_managed_file_submit', $triggering_element['#submit'])) {
      // If the image is not there, we do not check for empty values.
      $parents = $element['#parents'];
      $field = array_pop($parents);
      $image_field = NestedArray::getValue($form_state->getUserInput(), $parents);
      // The image field will have no values if it was hidden in the form.
      if (empty($image_field)) {
        return;
      }
      // We check for the array key, so that it can be NULL (like if the user
      // submits the form without using the "upload" button).
      if (!array_key_exists($field, $image_field)) {
        return;
      }
    }
    else {
      $form_state->setLimitValidationErrors([]);
    }
  }

Why can't it be

  /**
   * Validate callback for alt and title field, if the user wants them required.
   *
   * This is separated in a validate function instead of a #required flag to
   * avoid being validated on the process callback.
   */
  public static function validateRequiredFields($element, FormStateInterface $form_state) {
    // Only do validation if the function is triggered from other places than
    // the image process form.
    $triggering_element = $form_state->getTriggeringElement();
    if (!empty($triggering_element['#submit']) && in_array('file_managed_file_submit', $triggering_element['#submit'], TRUE)) {
      $form_state->setLimitValidationErrors([]);
    }
  }

Also the docs are wrong because we have #required on the form elements. We changed that in #1906264: Required alt tag missing on image alt tag input - actually as far as I can see I think we can completely remove validateRequiredFields since core/modules/image/tests/src/Functional/ImageFieldValidateTest.php passes just fine without it.

OK, here's the test-only from #33.
If this test passes, then we can remove validateRequiredFields

@aaronbauman the test will fail... we need to remove the calls to validateRequiredFields also in discussions in slack I think @Berdir has pointed out that we need $form_state->setLimitValidationErrors([]);... so here are the patches.

So the error that got 2715859-40-setLimitValidationErrors.patch was a known random - retesting and re-uploading so it is obvious which patch is correct. It looks like we can do that and that we have test coverage for needing $form_state->setLimitValidationErrors([]); in Drupal\Tests\image\Functional\ImageFieldDisplayTest and Drupal\Tests\node\Functional\PagePreviewTest which is great.

Works for me :)

The steps to reproduce in the IS aren't correct, because the article image field in Standard profile is cardinality 1 and not required. In 8.8.x HEAD, having a custom module that forbids access does not result in errors when creating articles.

Even when I change it to Required and cardinality=2, I still get no errors.

The test in #43 does fail locally for me if I remove the fix to ImageWidget.php. Any thoughts as to why it does, but why I can't replicate that same failure outside of the test?

Tagging for IS update for steps to reproduce outside of a test.

I've done the steps in the IS and they work... I've changed them so no custom modules are necessary.

I found why I wasn't seeing the error. It's because by default, sites are installed with Error messages to display set to None. Updating the IS to add the extra step of changing that setting.

Given that the IS has reproduction steps that do not require setting the field to required or its cardinality to something other than 1, why does the test in #43 do that? Seems to me we should have a test for the more common situation of it being not required and single-valued. If we want to additionally test multi-valued and/or required image fields, that's great, but I don't think we should do that without testing the not-required, single-valued case. Setting to Needs work for that, but feel free to reset it to NR or RTBC if there's a good explanation for why the test doesn't match the IS.

Comments 13 and 33 explain why the test included making the field required, and multiple cardinality. I can't make a judgement on whether that means work is needed to change the patch, or whether the test is valid and the manual steps to reproduce in the IS just needs updating.

Yeah, I'm confused about why we have this test in this thread.
We're including a test to recreate the bug in #3011744.
But the patch doesn't solve the cause of that bug, it only addresses a symptom.

Maybe it makes more sense to just solve 3011744?

Yes, as commented in #33, the test relies on those specific settings to be easily reproducible and it makes sense to me to include that here and commit it now. I've seen other cases too where it happens in more complex scenarios and it's actually a fairly common problem on sites and fills up logs.

Afterwards, we can adjust it as necessary in the follow-up, which might or might not happen any time soon.

The problem is that what this issue is surfacing is that we don't have test coverage for submitting empty values to an image widget. Both for the case where edit access is allowed, and for the case where it's forbidden. And if we know there are different code paths for optional vs. required and for single/limited/unlimited cardinality, then we need tests for all of those cases.

Here's a patch that adds those tests. It removes the separate test class that's in #43 and instead adds the test method to ImageFieldValidateTest, since that's where we're testing other validation.

This is just the test-only patch to prove that it fails (triggers the warnings described in the issue title/summary).

And here it is with the fix to ImageWidget.php.

lgtm

Thanks for all the collaboration on figuring out the bug, the fix, and the tests. Pushed to 8.8.x and 8.7.x.

BTW, #55 applies cleanly on 8.6.x.