Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tedbow created an issue. See original summary.

tedbow’s picture

tedbow’s picture

Issue tags: +Needs tests
FileSize
1.74 KB

Here is an initial patch that pulled changes from #2936293: At least inform content authors where they can list and add media

tedbow’s picture

Status: Active » Needs review
FileSize
1.19 KB

Looking 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

 * @FieldWidget(
 *   id = "test_field_widget_multiple",
 *   label = @Translation("Test widget - multiple"),
 *   multiple_values = TRUE,
 *   weight = 10
 * )

Because multiple_values = TRUE then new hook would never have actually affected this field.

tedbow’s picture

Ok. 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.

tedbow’s picture

+++ b/core/modules/field/src/Tests/FormTest.php
@@ -91,7 +91,7 @@ protected function setUp() {
-  public function testFieldFormSingle() {
+  public function xtestFieldFormSingle() {

Whoops! fixing these

The last submitted patch, 5: 2943035-5.patch, failed testing. View results

tedbow’s picture

#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.

tedbow’s picture

Here is TEST_ONLY patch that will fail.

The last submitted patch, 6: 2943035-6.patch, failed testing. View results

Status: Needs review » Needs work

The last submitted patch, 9: 2943035-8-TEST_ONLY.patch, failed testing. View results

xjm credited tim.plunkett.

xjm’s picture

Status: Needs work » Needs review

Adding credit for Tim as well who originally found the problem with the multiple code paths in WidgetBase and proposed the fix.

xjm’s picture

Priority: Normal » Major
Issue summary: View changes
Issue tags: -Needs tests
FileSize
110.74 KB
61.05 KB

I 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:

widgetAlterTest
fail: [Other] Line 693 of core/modules/field/src/Tests/FormTest.php:
"From hook_field_widget_multivalue_form_alter(): prefix on field_multiple parent element." found only once

fail: [Other] Line 693 of core/modules/field/src/Tests/FormTest.php:
"From hook_field_widget_multivalue_WIDGET_TYPE_form_alter(): prefix on field_multiple parent element." found only once

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.

xjm’s picture

Status: Needs review » Reviewed & tested by the community

OK, RTBC. :)

xjm’s picture

Just hiding the test-only patch from the summary list for clarity; the one to commit is #8.

amateescu’s picture

+++ b/core/modules/field/tests/modules/field_test/src/Plugin/Field/FieldWidget/TestFieldWidgetMultipleSingleValues.php
@@ -0,0 +1,22 @@
+class TestFieldWidgetMultipleSingleValues extends TestFieldWidgetMultiple {

What is the difference between this new widget and the existing \Drupal\field_test\Plugin\Field\FieldWidget\TestFieldWidget?

alexpott’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: +Needs followup

I think we should have a followup to update hook_field_widget_multivalue_form_alter() and hook_field_widget_multivalue_WIDGET_TYPE_form_alter() docs. Also there's an error there where hook_field_widget_multivalue_WIDGET_TYPE_form_alter() refers to hook_field_widget_form_alter() when I think it means hook_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!

  • alexpott committed 60b8de9 on 8.6.x
    Issue #2943035 by tedbow, xjm, tim.plunkett:...

  • alexpott committed b5c4aa5 on 8.5.x
    Issue #2943035 by tedbow, xjm, tim.plunkett:...
xjm’s picture

Re: #17, see #4.

amateescu’s picture

I 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.

alexpott’s picture

Discussed #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.

amateescu’s picture

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.