ImageWidget::validateRequiredFields() produces a PHP Warning message if triggering element is a non-button

Here is an alternative version where I am just adjusting the condition. I have verified that this works at the project I am working at, where there is a form with an image field plus a custom field with an AJAX handler. It is when I trigger the custom field's AJAX request that this error happens.

I will now see if I can reproduce this in a test.

Here are two patches. One is #9 with a test and the other is the test that proves the bug.

Removing Need Tests tag.

Is there anything missing at #10 that I could improve?

This patch works great for me (also applies in 8.2!), but given the feedback in #15, I'm setting this to Needs Work.

@mariacha1, I think that what @mcdruid means at #15 is that his patch makes a small adjustment. We believe that there is nothing left to do but waiting for core reviewers to chime in. Thanks for testing the patch though!

Patch in #15 works great, applied on 8.2.x site.

Tested patch #15 works fine. Fixed issues with Address module when using on same form with imagefield.

Cross referencing: Adding related issue: https://www.drupal.org/node/2346893. That issue's patch conflicts with this one in /core/modules/image/src/Tests/ImageFieldValidateTest.php.

Minor issues:

1. +++ b/core/modules/image/src/Tests/ImageFieldValidateTest.php
   @@ -156,4 +159,43 @@ protected function getFieldSettings($min_resolution, $max_resolution) {
   +    // Add a custom field to the Article content type that contains
   +    // an AJAX handler on a select field.
   

   Inline comments should try to get to 80chars limit as close as possible.

2. +++ b/core/modules/image/src/Tests/ImageFieldValidateTest.php
   @@ -156,4 +159,43 @@ protected function getFieldSettings($min_resolution, $max_resolution) {
   +    entity_get_form_display('node', 'article', 'default')
   

   entity_get_form_display() is deprecated. Its docblock gives hints on what to replace it with.

3. +++ b/core/modules/image/tests/modules/image_module_test/src/Plugin/Field/FieldWidget/DummyAjaxWidget.php
   @@ -0,0 +1,57 @@
   + * Default widget for Zeus videos.
   ...
   +   * Ajax callback to update the video widget fields when select is changed.
   ...
   +   *   Ajax response with updated widget for video.
   

   These need to be updated as they refer to a video widget.

The problem addressed in this issue is occurring for me on a client site running drupal-8.3.7 and address-8.x-1.1 — selecting a country from the select list in the first step of an address widget fails to AJAX-load the rest of the address widget fields because the validation for an image widget much lower down on the node/add page breaks validation for the address widget.

The patch in #15 fixes the issue; but Composer can't apply it automatically because the patch context doesn't match exactly (Hunk #1 succeeded at 256 (offset 3 lines) while patching file core/modules/image/src/Plugin/Field/FieldWidget/ImageWidget.php), so I'm uploading a copy of the patch in #15, but generated against drupal-8.3.x to make Composer happy.

Since I've confirmed this is a bug in 8.3.x., and I daresay these are not "disruptive changes", I'm adding the "Needs backport to 8.3.x" tag. Feel free to remove if necessary. There doesn't seem to be a "Needs backport to 8.4.x" tag yet.

Updating issue summary (especially, Remaining tasks).

Since I've confirmed this is a bug in 8.3.x., and I daresay these are not "disruptive changes", I'm adding the "Needs backport to 8.3.x" tag. Feel free to remove if necessary. There doesn't seem to be a "Needs backport to 8.4.x" tag yet.

There won't be any more patch releases for 8.3.x so it's unlikely - I would say impossible - that this patch will ever see 8.3.x. Even more 8.2.x which is now close.
So removing both from remaining tasks and removing the Needs backport 8.3.x tag.

Then I'm not 100% sure what 'Backport to 8.4.x' would mean, as we backport between major versions only AFAIK (i.e. D8->D7). Patches are cherry-picked between minor versions by committers and normally it's at their discretion. As this patch is a non-disruptive bugfix has all the chances to get into 8.4.x.
See https://www.drupal.org/core/backport-policy and https://www.drupal.org/core/d8-allowed-changes .

The patch in #15 fixes the issue; but Composer can't apply it automatically because the patch context doesn't match exactly

I see what you mean, however I'm using #15 with composer against 8.3.7 and applies nicely.

Addressing minor issues in #26 and some code style fixes in touched files.

Just a rebase

Hi #30 and #31. Thanks for your work! Much appreciated.
However your patches are changing pieces of code out of this issue's scope. I can see all of them are trying to fix Coding Standards issues, but they will need to be fixed in a separated issue.

Can you re-roll patch on #15 and apply feedback from #26?

Adding re-roll tag + setting the current patch back to #15.
Also adding Novice tag as both re-rolling task and comments on #26 are not that complicated. (Global sprint weekend 2018 is coming after all, and we need Novice tasks!)

Thanks a lot to everybody!

Re-rolled and created the patch. Please review.

Addressed points in #26.

Sorry, only saw this was tagged as Novice after I'd created the patch. :#

Looks great now.
+1 for RTBC.

Thank you for the hard work guys!

#26.3 is still outstanding.

Looks ok to me +1 for RTBC.

Looks good to me. The only things I'm not sure is if we should re-write the test to use phpunit instead of simpletest (and so postpone this issue until #2863626: Convert web tests to browser tests for image module is in).
To me it makes sense to add it in there and migrate then all test class at once in its own issue, but not sure if committers will agree.
Adding the related issue and RTBCing.

Thanks everyone for the hard work!

Committed/pushed to 8.6.x and cherry-picked to 8.5.x. Thanks!

It's OK to add the new method here and convert all the tests in one go together.