Field setting form is broken after 2412569 [#2712545]

Problem/Motivation

After #2412569: Allow setting the auto-create bundle on entity reference fields with multiple target bundles selecting the bundle(s) of any entity type the ajax request is triggered and calls \Drupal\Core\Entity\Plugin\EntityReferenceSelection\DefaultSelection::validateConfigurationForm which expects form states values to be set as ER Field setting and it broke the DER field setting form.

Proposed resolution

Fix the DER field setting form.

Remaining tasks

Fix the test fails and bug.

User interface changes

None

API changes

None

Data model changes

None

Comment	File	Size	Author
#16	field_setting_form_is-2712545-16.patch	11.65 KB	jibran
#16	8.x-1.x: PHP 5.5 & MySQL 5.5, D8.2 39 pass PHP 5.6 & MySQL 5.5, D8.2 39 pass PHP 7 & MySQL 5.5, D8.2 39 pass
#15	settings-entity-test-base-field-display-handler-settings-target-bundles-entity-test-base-field-display-7.jpg	607.58 KB	jibran
#12	field_setting_form_is-2712545-12.patch	9.69 KB	jibran
#12	8.x-1.x: PHP 5.5 & MySQL 5.5, D8.2 38 pass, 3 fail PHP 5.6 & MySQL 5.5, D8.2 38 pass, 3 fail
#2	field_setting_form_is-2712545-2-test-only.patch	6.35 KB	jibran
#2	8.x-1.x: PHP 5.5 & MySQL 5.5, D8.2 38 pass, 3 fail PHP 5.6 & MySQL 5.5, D8.2 38 pass, 3 fail
#2	field_setting_form_is-2712545-2.patch	9.81 KB	jibran
#2	8.x-1.x: PHP 5.5 & MySQL 5.5, D8.2 38 pass, 3 fail PHP 5.6 & MySQL 5.5, D8.2 38 pass, 3 fail

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

24 April 2016 at 17:17

jibran created an issue. See original summary.

Comment #2

jibran

he/him

Urdu

Sydney, Australia

CreditAttribution: jibran as a volunteer commented 24 April 2016 at 17:24

Assigned:	Unassigned	» larowlan
Status:	Active	» Needs review

File	Size
field_setting_form_is-2712545-2.patch	9.81 KB
8.x-1.x: PHP 5.5 & MySQL 5.5, D8.2 38 pass, 3 fail PHP 5.6 & MySQL 5.5, D8.2 38 pass, 3 fail
field_setting_form_is-2712545-2-test-only.patch	6.35 KB
8.x-1.x: PHP 5.5 & MySQL 5.5, D8.2 38 pass, 3 fail PHP 5.6 & MySQL 5.5, D8.2 38 pass, 3 fail

While working on #2469609: Convert all WebTestBase to BrowserTestBase I found this bug. @larowlan can you please have look at it?

Comment #3

24 April 2016 at 17:31

The last submitted patch, 2: field_setting_form_is-2712545-2.patch, failed testing.

Comment #4

24 April 2016 at 17:32

The last submitted patch, 2: field_setting_form_is-2712545-2.patch, failed testing.

Comment #5

24 April 2016 at 17:32

Status:

Needs review

» Needs work

The last submitted patch, 2: field_setting_form_is-2712545-2-test-only.patch, failed testing.

Comment #6

24 April 2016 at 17:32

The last submitted patch, 2: field_setting_form_is-2712545-2-test-only.patch, failed testing.

Comment #7

larowlan

🇦🇺🏝.au GMT+10

CreditAttribution: larowlan commented 24 April 2016 at 20:58

Comment #8

OnkelTem CreditAttribution: OnkelTem commented 25 April 2016 at 19:26

I have a related issue.

$triggering_element = $form_state->getTriggeringElement();
$parents = $triggering_element['#parents'];
if (array_shift($parents) === 'settings') {
  // ...
}

the if-part never triggers here on Drupal 8.0.5.
Particularly, $parents = $triggering_element['#parents']; results to ['submit'] array i.e. w/o 'settings'.

Comment #9

OnkelTem CreditAttribution: OnkelTem commented 25 April 2016 at 19:52

I've checked the patch with the latest Drupal 8.1.x (thinking that the patch logic rely on some latest changes) and it doesn't seem to work properly - the if-content is still unreachable.

For example, if you create a DER field referencing a non-bundled entity like "User", then settings>user>handler_settings>target_bundles will get value of "[]", which prevents autocomplete to work as it expects either not empty array of bundles or NULL (which obviously would happen in the 320th line if it was called).

Comment #10

OnkelTem CreditAttribution: OnkelTem commented 26 April 2016 at 07:21

What do you think:

diff --git a/src/Plugin/Field/FieldType/DynamicEntityReferenceItem.php b/src/Plugin/Field/FieldType/DynamicEntityReferenceItem.php
index 0860a99..c85f26c 100644
--- a/src/Plugin/Field/FieldType/DynamicEntityReferenceItem.php
+++ b/src/Plugin/Field/FieldType/DynamicEntityReferenceItem.php
@@ -341,6 +341,16 @@ class DynamicEntityReferenceItem extends EntityReferenceItem {
         'handler_submit',
       ]);
     }
+    // Addition
+    $field = $form_state->getFormObject()->getEntity();
+    // @todo inject this
+    $entity_types = \Drupal::entityManager()->getDefinitions();
+    $settings = $field->getSettings();
+    foreach (array_keys(array_intersect_key($settings, $entity_types)) as $entity_type_id) {
+      if ($form_state->getValue(['settings', $entity_type_id, 'handler_settings', 'target_bundles']) === []) {
+        $form_state->setValue(['settings', $entity_type_id, 'handler_settings', 'target_bundles'], NULL);
+      }
+    }
   }
 
   /**

Comment #11

jibran

he/him

Urdu

Sydney, Australia

CreditAttribution: jibran as a volunteer commented 28 April 2016 at 02:33

Thanks @OnkelTem. I think #10 makes a lot of sense. If you can do a patch it'd be great but if not I'll have a look at it on weekend if Lee won't beat me to it.

Comment #12

jibran

he/him

Urdu

Sydney, Australia

CreditAttribution: jibran as a volunteer commented 28 April 2016 at 19:48

Status:

Needs work

» Needs review

File	Size
field_setting_form_is-2712545-12.patch	9.69 KB
8.x-1.x: PHP 5.5 & MySQL 5.5, D8.2 38 pass, 3 fail PHP 5.6 & MySQL 5.5, D8.2 38 pass, 3 fail

2 files were hidden/shown/deleted

File	Size
field_setting_form_is-2712545-2.patch	9.81 KB
8.x-1.x: PHP 5.5 & MySQL 5.5, D8.2 38 pass, 3 fail PHP 5.6 & MySQL 5.5, D8.2 38 pass, 3 fail
field_setting_form_is-2712545-2-test-only.patch	6.35 KB
8.x-1.x: PHP 5.5 & MySQL 5.5, D8.2 38 pass, 3 fail PHP 5.6 & MySQL 5.5, D8.2 38 pass, 3 fail

@OnkelTem can you try this?

Comment #13

28 April 2016 at 19:52

Status:

Needs review

» Needs work

The last submitted patch, 12: field_setting_form_is-2712545-12.patch, failed testing.

Comment #14

28 April 2016 at 19:52

The last submitted patch, 12: field_setting_form_is-2712545-12.patch, failed testing.

Comment #15

jibran

he/him

Urdu

Sydney, Australia

CreditAttribution: jibran as a volunteer commented 28 April 2016 at 20:14

File	Size
settings-entity-test-base-field-display-handler-settings-target-bundles-entity-test-base-field-display-7.jpg	607.58 KB

Comment #16

jibran

he/him

Urdu

Sydney, Australia

CreditAttribution: jibran as a volunteer commented 7 May 2016 at 19:53

Status:

Needs work

» Needs review

File	Size
field_setting_form_is-2712545-16.patch	11.65 KB
8.x-1.x: PHP 5.5 & MySQL 5.5, D8.2 39 pass PHP 5.6 & MySQL 5.5, D8.2 39 pass PHP 7 & MySQL 5.5, D8.2 39 pass

2 files were hidden/shown/deleted

File	Size
field_setting_form_is-2712545-12.patch	9.69 KB
8.x-1.x: PHP 5.5 & MySQL 5.5, D8.2 38 pass, 3 fail PHP 5.6 & MySQL 5.5, D8.2 38 pass, 3 fail
settings-entity-test-base-field-display-handler-settings-target-bundles-entity-test-base-field-display-7.jpg	607.58 KB

Here is a green patch.

Comment #17

larowlan

🇦🇺🏝.au GMT+10

CreditAttribution: larowlan commented 5 May 2016 at 02:41

Status:

Needs review

» Reviewed & tested by the community

+++ b/tests/src/FunctionalJavascript/DynamicEntityReferenceTest.php
@@ -0,0 +1,118 @@
+   * Tests if the toolbar can be toggled with JavaScript.

c/p issue

other than that, rtbc

great work

Comment #18

7 May 2016 at 19:55

jibran committed 03a37a7 on 8.x-1.x

Issue #2712545 by jibran, OnkelTem, larowlan: Field setting form is...

Comment #19

jibran

he/him

Urdu

Sydney, Australia

CreditAttribution: jibran as a volunteer commented 17 May 2016 at 00:59

Status:

Reviewed & tested by the community

» Fixed

Thanks @larowlan for the review.
Fixed on commit

diff --git a/tests/src/FunctionalJavascript/DynamicEntityReferenceTest.php b/tests/src/FunctionalJavascript/DynamicEntityReferenceTest.php
index 5262e92..52eb284 100644
--- a/tests/src/FunctionalJavascript/DynamicEntityReferenceTest.php
+++ b/tests/src/FunctionalJavascript/DynamicEntityReferenceTest.php
@@ -66,7 +66,7 @@ class DynamicEntityReferenceTest extends JavascriptTestBase {
   }
 
   /**
-   * Tests if the toolbar can be toggled with JavaScript.
+   * Tests field settings of dynamic entity reference field.
    */
   public function testFieldSettings() {
     $assert_session = $this->assertSession();