PHP notice on multiple items image field

Right. That's because ImageWidget subsclasses FileWidget, which in some cases (when cardinality > 1) uses the 'display_field' setting, which does not exist for image fields.
That's slightly shaky, but is acceptable IMO since this is just used to populate a #property that ends up not being used if what runs is an ImageWidget.

The getFieldSetting() method should be resilient on such cases, and return NULL if the setting is unknown for the field type.
Patch attached.

The getFieldSetting() method should be resilient on such cases

I don't know that I agree with that. I wonder if there's a way to handle the "subtype" case without removing the strictness otherwise.

Unless we want to throw an exception, which would be an overkill for this situation imo, the reasoning and fix makes sense :)

Edit: Crossposted with Alex, please feel free to set it to NW if you think it needs more discussion.

How about adding a $strict parameter (defaults TRUE) to getFieldSetting()? In situations where formatters/widgets don't care about whether the setting exists, they can explicitly pass FALSE.

@eff: what are your concerns about returning NULL ?

@eff: what are your concerns about returning NULL ?

The same as why PHP issues a notice when accessing any other undefined variable or array key. More strictness leads to better finding of code flaws and more maintainable code. By default, calling getFieldSetting() with a setting name not defined by the field type is nonsense, and is likely to be a result of a typo, or a code refactoring that missed some hunks, for which a notice is useful.

ImageWidget subsclasses FileWidget, which in some cases (when cardinality > 1) uses the 'display_field' setting, which does not exist for image fields. That's slightly shaky

Exactly. Ideally, we should fix that. We need to decide if ImageItem should extend FileItem. If yes, then it should also include all of the base type's settings, even if they're not used, just as PHP classes inherit the base class's public properties even if the subclass doesn't use them. If not, then I don't think it makes sense for ImageWidget to subclass FileWidget; instead maybe we need an abstract base class for each widget to subclass.

but is acceptable IMO since this is just used to populate a #property that ends up not being used

Yep, for rare cases where we know we're doing something not really right, but also not problematic, that's where I think a 2nd parameter to getFieldSetting() would be acceptable.

You're right of course, failing with a notice is better than silently returning null, that's the whole point of developing with E_ALL.
It's just that the current notice is not really telling though - maybe we should add a custom trigger_error() message here 'unknown setting' or something.

When working with PHP arrays you typically use isset() when unsure - so maybe we should have hasFieldSetting() ?
Then it's up to FileWidget to know it's reused by ImageWidget (we're core...), on fields which might not have the 'display_field' setting ?

We need to decide if ImageItem should extend FileItem.

I think that's the plan, and thats what #2015697: Convert field type to typed data plugin for file and image modules is doing.

If yes, then it should also include all of the base type's settings, even if they're not used, just as PHP classes inherit the base class's public properties even if the subclass doesn't use them. If not, then I don't think it makes sense for ImageWidget to subclass FileWidget; instead maybe we need an abstract base class for each widget to subclass.

Hm, interesting / tricky.
The file field type does have a couple 'settings' (both field and instance level) that the image field type doesn't have.
Same for 'properties', BTW.

I'm not sure that reasoning holds. Class inheritance is purely a contract on members.
If it applies to "field and instance settings", I guess it also applies to "properties" (they are "almost" members, luckily they happen to be magic and exposed through a method).
There's no guarantee that an Item class has the same settings or TypedData properties as a parent, it's only about code reuse IMO.

Well, one quick workaround is to use $this->getFieldSettings(), and work on the array.
Turns out, FileWidget::formElement() does exactly this already.

#8 solves the OP, but not the broader scope in the issue title change from #1, so restoring original title.

maybe we should have hasFieldSetting() ?

That seems good to me. Want to file a separate issue for that?

There's no guarantee that an Item class has the same settings or TypedData properties as a parent, it's only about code reuse IMO.

This could use some more discussion. I'll take a look at #2015697: Convert field type to typed data plugin for file and image modules and will add some thoughts there if relevant, or open a separate issue if needed.

Oh wait, the fact that bot didn't catch this bug to begin with indicates we're missing some test coverage for editing a multivalued image field.

I'll write a test later this evening - unless someone beats me to it.

fail patch will trigger the notice, pass will be green. I've kept it really easy as we do have tests for uploading multiple files already, so I think we're ok.

tests are added.

thanks ! Back to RTBC then ?

We have test coverage, the notice is fixed, the same code already exists in a different place, @effulgentsia was ok with the fix too. I'd say yes :)

+++ b/core/modules/image/lib/Drupal/image/Tests/ImageFieldDisplayTest.phpundefined
@@ -205,6 +205,10 @@ function testImageFieldSettings() {
+    // Set cardinality to unlimited.
+    $this->drupalPost('admin/structure/types/manage/article/fields/node.article.' . $field_name . '/field', array('field[cardinality]' => FIELD_CARDINALITY_UNLIMITED), t('Save field settings'));

There's no assert here - this at least needs a comment to explain that it's testing the absence of a notice, but a positive test would be better.

The change looks not ideal, but it's an improvement on #1 and I couldn't really find a better place to fix this..

Right, makes sense, while adding asserts, I actually got new notices. Also tagging so I can follow it on our board.

More comments and positive asserts + an extra upload as well.

Looks good.

+++ b/core/modules/image/lib/Drupal/image/Tests/ImageFieldDisplayTest.php
@@ -223,6 +223,19 @@ function testImageFieldSettings() {
+    $this->drupalPost('admin/structure/types/manage/article/fields/node.article.' . $field_name . '/field', array('field[cardinality]' => FIELD_CARDINALITY_UNLIMITED), t('Save field settings'));
...
+    $this->drupalPost('node/' . $nid . '/edit', $edit, t('Save and keep published'));

#2074037: Add drupalPostUrl() — drupalPost()/drupalPostAjax() are for forms only, D8 JS performs non-form HTTP requests has changed drupalPost to drupalPostForm

So this should work then ?

#21: unknown_field_setting-2050113-21-pass.patch queued for re-testing.

+++ b/core/modules/image/lib/Drupal/image/Tests/ImageFieldDisplayTest.php
@@ -223,6 +223,19 @@ function testImageFieldSettings() {
+    $edit['files[' . $field_name . '_' . Language::LANGCODE_NOT_SPECIFIED . '_1][]'] = drupal_realpath($test_image->uri);

Now that #2083811: Remove langcode nesting for fields in entity $form structures got in, this should be 'files[' . $field_name . '_1]' :-)

I know, but now I'm getting notices again, it's freaky ...

Finally have this. This also contains a fix that you can only reproduce on PHP 5.4.4 and our testbot doesn't catch on 5.3 ...

+++ b/core/modules/image/lib/Drupal/image/Plugin/field/widget/ImageWidget.php
@@ -94,7 +94,12 @@ protected function formMultipleElements(FieldInterface $items, array &$form, arr
-      $elements['#file_upload_description'] = drupal_render($file_upload_help);

I suppose there is some code above that sets the $file_upload_help variable, is it still used anywhere else or can it be removed?

Yep, it seems ImageWidget::formMultipleElements() needs some additional cleanup :-)

Okay something like this? Cleand up the multiple case and removed $file_upload_help variable.

An other version, keeping the $file_upload_variable to avoid duplicated code, suggested by yched.

Cancelled the test for #29.

#30 looks good if green - thanks !

We need those drupal_render() functions around $file_upload_help.

Rerolled the patch. This should pass.

Wait, I thought we needed to remove those drupal_render() calls ?
(fwiw, they were removed in FileWidget already)

We actually can only drop one drupal_render() ..

Just stumbled over #1430934: Notice: Undefined index: display_field in file_field_widget_value() (line 582 of /module/file/file.field.inc)., is that a duplicate?

Hmm might be, although should start testing. I do know we don't have this issue in D7 IIRC - it would have popped up much more I guess.
This patch also fixes a stupid php 5.4 bug.

Manually tested this. I changed the image field in the core Article content type to allow multiple fields. Before patch I get the notice, after the patch, no more notice. Tested locally under PHP 5.4.4.

Reroll of #35, since the field widget classes were moved to a new namespace.

Forgot that the FIELD_CARDINALITY_UNLIMITED has moved as well.

41: 2050113-multiple-image-field-warning-41.patch queued for re-testing.

...doing your job for you, testbot.

Should apply now.

Yep, thanks

Committed 185c50b and pushed to 8.x. Thanks!

Maybe related: #2164151: Regression: Cannot create field with unlimited values

ridiculous quick follow up at #2639796: file widget duplicate code