Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 UTC on 18 March 2024, to get $100 off your ticket.
Follow up to #2940201: hook_field_widget_form_alter() can no longer affect the whole widget for multi-value fields
When using this new hook in #2936293: At least inform content authors where they can list and add media it was realized that for a widget like options that can handle multiple values in single widget these new hooks didn't get invoked.
Comment | File | Size | Author |
---|---|---|---|
#22 | 2943035-22.patch | 6.25 KB | amateescu |
#14 | now_works_multiple.png | 61.05 KB | xjm |
#14 | worked_multiple_element.png | 110.74 KB | xjm |
#8 | 2943035-8.patch | 10.3 KB | tedbow |
#8 | interdiff-6-8.txt | 1.58 KB | tedbow |
Comments
Comment #2
tedbowComment #3
tedbowHere is an initial patch that pulled changes from #2936293: At least inform content authors where they can list and add media
Comment #4
tedbowLooking at #2940201: hook_field_widget_form_alter() can no longer affect the whole widget for multi-value fields in more detail the tests added don't actually test the field they are adding.
This patch simply removes adding the field in
\Drupal\field\Tests\FormTest::widgetAlterTest
and the tests still pass.The test fields is using the widget
test_field_widget_multiple
,\Drupal\field_test\Plugin\Field\FieldWidget\TestFieldWidgetMultiple
which has
Because
multiple_values = TRUE
then new hook would never have actually affected this field.Comment #5
tedbowOk. So to test correctly I made new widget where
multiple_values = FALSE,
test both cases.Also the hooks in field_test.module were not checking for field_name so they were alter other fields that is why the tests still passed in #4.
Comment #6
tedbowWhoops! fixing these
Comment #8
tedbow#5 failed in
\Drupal\field_ui\Tests\ManageDisplayTest::testWidgetUI()
because it wasn't expecting the new widget.Adding a state() check to only use this widget when the test method will need it.
Comment #9
tedbowHere is TEST_ONLY patch that will fail.
Comment #13
xjmAdding credit for Tim as well who originally found the problem with the multiple code paths in
WidgetBase
and proposed the fix.Comment #14
xjmI looked this over; looks good. Reusing the other tests' fixtures was a bit of an issue. :P The test-only result at https://www.drupal.org/pift-ci-job/883613 shows the correct fail:
I also ran the test locally and inspected the verbose output. This is the multiple allowed value, multiple single field case that already works in HEAD.
This is the single element with multiple values that doesn't work in HEAD, but does now with the patch applied:
Promoting to major since it again blocks the other major.
Comment #15
xjmOK, RTBC. :)
Comment #16
xjmJust hiding the test-only patch from the summary list for clarity; the one to commit is #8.
Comment #17
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedWhat is the difference between this new widget and the existing
\Drupal\field_test\Plugin\Field\FieldWidget\TestFieldWidget
?Comment #18
alexpottI think we should have a followup to update
hook_field_widget_multivalue_form_alter()
andhook_field_widget_multivalue_WIDGET_TYPE_form_alter()
docs. Also there's an error there wherehook_field_widget_multivalue_WIDGET_TYPE_form_alter()
refers tohook_field_widget_form_alter()
when I think it meanshook_field_widget_multivalue_form_alter()
.However committing this now so that the new hook is invoked correctly in the 8.5.0 beta.
Committed and pushed 60b8de9ef0 to 8.6.x and b5c4aa5dbf to 8.5.x. Thanks!
Comment #21
xjmRe: #17, see #4.
Comment #22
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedI saw #4, yet I still don't understand why we need the new widget. There's no such thing as "multiple single value", a widget either handles multiple values on its own or not.
Comment #23
alexpottDiscussed #22 with @amateescu on Slack and we agreed a followup was best because #22 is test-only changes and the hook is now being invoked correctly which seems important for 8.5.0-beta. And the test changes can safely be backported to the beta when ready.
Comment #24
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedOpened a followup to clean this up: #2943189: Clean up the test code for hook_field_widget_multivalue_form_alter()