Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Steps:
- On a fresh D8 install, Standard Profile, change the
field_image
cardinality from 1 to Unlimited - Add a new Article node. You'll see a PHP notice popping up. See attached screenshot.
Comment | File | Size | Author |
---|---|---|---|
#46 | multiple-image-field-warning-2050113-46.patch | 3.86 KB | marthinal |
#41 | interdiff.txt | 1.35 KB | longwave |
#41 | 2050113-multiple-image-field-warning-41.patch | 3.91 KB | longwave |
article-create.jpg | 430.82 KB | claudiu.cristea |
Comments
Comment #1
yched CreditAttribution: yched commentedRight. 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.
Comment #2
effulgentsia CreditAttribution: effulgentsia commentedI 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.
Comment #3
amateescu CreditAttribution: amateescu commentedUnless 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.
Comment #4
effulgentsia CreditAttribution: effulgentsia commentedHow 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.
Comment #5
yched CreditAttribution: yched commented@eff: what are your concerns about returning NULL ?
Comment #6
effulgentsia CreditAttribution: effulgentsia commentedThe 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.
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.
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.
Comment #7
yched CreditAttribution: yched commentedYou'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 ?
I think that's the plan, and thats what #2015697: Convert field type to typed data plugin for file and image modules is doing.
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.
Comment #8
yched CreditAttribution: yched commentedWell, one quick workaround is to use $this->getFieldSettings(), and work on the array.
Turns out, FileWidget::formElement() does exactly this already.
Comment #9
effulgentsia CreditAttribution: effulgentsia commented#8 solves the OP, but not the broader scope in the issue title change from #1, so restoring original title.
That seems good to me. Want to file a separate issue for that?
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.
Comment #10
effulgentsia CreditAttribution: effulgentsia commentedOh 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.
Comment #11
swentel CreditAttribution: swentel commentedI'll write a test later this evening - unless someone beats me to it.
Comment #12
swentel CreditAttribution: swentel commentedfail 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.
Comment #13
swentel CreditAttribution: swentel commentedtests are added.
Comment #14
yched CreditAttribution: yched commentedthanks ! Back to RTBC then ?
Comment #15
BerdirWe 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 :)
Comment #16
catchThere'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..
Comment #17
swentel CreditAttribution: swentel commentedRight, makes sense, while adding asserts, I actually got new notices. Also tagging so I can follow it on our board.
Comment #18
swentel CreditAttribution: swentel commentedMore comments and positive asserts + an extra upload as well.
Comment #19
yched CreditAttribution: yched commentedLooks good.
Comment #20
alexpott#2074037: Add drupalPostUrl() — drupalPost()/drupalPostAjax() are for forms only, D8 JS performs non-form HTTP requests has changed drupalPost to drupalPostForm
Comment #21
swentel CreditAttribution: swentel commentedSo this should work then ?
Comment #22
swentel CreditAttribution: swentel commented#21: unknown_field_setting-2050113-21-pass.patch queued for re-testing.
Comment #24
yched CreditAttribution: yched commentedNow that #2083811: Remove langcode nesting for fields in entity $form structures got in, this should be 'files[' . $field_name . '_1]' :-)
Comment #25
swentel CreditAttribution: swentel commentedI know, but now I'm getting notices again, it's freaky ...
Comment #26
swentel CreditAttribution: swentel commentedFinally 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 ...
Comment #27
amateescu CreditAttribution: amateescu commentedI suppose there is some code above that sets the $file_upload_help variable, is it still used anywhere else or can it be removed?
Comment #28
yched CreditAttribution: yched commentedYep, it seems ImageWidget::formMultipleElements() needs some additional cleanup :-)
Comment #29
Hydra CreditAttribution: Hydra commentedOkay something like this? Cleand up the multiple case and removed $file_upload_help variable.
Comment #30
Hydra CreditAttribution: Hydra commentedAn other version, keeping the $file_upload_variable to avoid duplicated code, suggested by yched.
Comment #31
yched CreditAttribution: yched commentedCancelled the test for #29.
#30 looks good if green - thanks !
Comment #33
netsensei CreditAttribution: netsensei commentedWe need those drupal_render() functions around $file_upload_help.
Rerolled the patch. This should pass.
Comment #34
yched CreditAttribution: yched commentedWait, I thought we needed to remove those drupal_render() calls ?
(fwiw, they were removed in FileWidget already)
Comment #35
swentel CreditAttribution: swentel commentedWe actually can only drop one drupal_render() ..
Comment #36
BerdirJust 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?
Comment #37
swentel CreditAttribution: swentel commentedHmm 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.
Comment #38
yoroy CreditAttribution: yoroy commentedManually 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.
Comment #39
longwaveReroll of #35, since the field widget classes were moved to a new namespace.
Comment #41
longwaveForgot that the FIELD_CARDINALITY_UNLIMITED has moved as well.
Comment #42
swentel CreditAttribution: swentel commentedComment #43
webchick41: 2050113-multiple-image-field-warning-41.patch queued for re-testing.
Comment #45
webchick...doing your job for you, testbot.
Comment #46
marthinal CreditAttribution: marthinal commentedShould apply now.
Comment #47
marthinal CreditAttribution: marthinal commentedComment #48
swentel CreditAttribution: swentel commentedYep, thanks
Comment #49
alexpottCommitted 185c50b and pushed to 8.x. Thanks!
Comment #50
penyaskitoMaybe related: #2164151: Regression: Cannot create field with unlimited values
Comment #52
swentel CreditAttribution: swentel as a volunteer commentedridiculous quick follow up at #2639796: file widget duplicate code