This is postpone on #1959806: Provide a generic 'entity_autocomplete' Form API element. We have to rethink and fix DER widget after the brand new 'entity_autocomplete' Form API element.

CommentFileSizeAuthor
#24 Screenshot 2015-02-18 19.32.24.png24.05 KBlarowlan
#24 Screenshot 2015-02-18 19.30.07.png44.89 KBlarowlan
#20 fixes_after_generic-2411981-20.patch22.2 KBjibran
#20 interdiff.txt594 bytesjibran
#18 fixes_after_generic-2411981-18.patch22.24 KBjibran
#18 interdiff.txt3.81 KBjibran
#11 fixes_after_generic-2411981-10.patch22.42 KBjibran
#11 interdiff.txt6.41 KBjibran
#8 fixes_after_generic-2411981-8.patch17.52 KBjibran
#2 2107243-2.patch24.4 KBjibran
#2 interdiff.txt2.81 KBjibran
decouple-er-selection-plugin.patch17.64 KBjibran
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran commented

Well this is frustrating the parent issue got re-scoped and I have to change the logic again. But I think this is what you have to deal with if you are following a moving target. :/

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran commented
Status: Postponed » Needs review
FileSize
interdiff.txt2.81 KB
2107243-2.patch24.4 KB

Can I get review/RTBC here before #2374019: Cleanup the use of custom item properties in EntityReferenceFormatterBase got fixed? Everything is green locally.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran commented

Minor docs adjustment needed.

diff --git a/src/Plugin/Field/FieldType/DynamicEntityReferenceItem.php b/src/Plugin/Field/FieldType/DynamicEntityReferenceItem.php
index db8c39e..2ffd600 100644
--- a/src/Plugin/Field/FieldType/DynamicEntityReferenceItem.php
+++ b/src/Plugin/Field/FieldType/DynamicEntityReferenceItem.php
@@ -208,7 +208,7 @@ class DynamicEntityReferenceItem extends ConfigurableEntityReferenceItem {
    *
    * This is same as
    * \Drupal\entity_reference\ConfigurableEntityReferenceItem::fieldSettingsForm()
-   * but it use dynamic_entity_reference_selection plugin manager instead of
+   * but it uses dynamic_entity_reference_selection plugin manager instead of
    * entity_reference_selection plugin manager.
    *
    * @param array $form
larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan commented
Status: Needs review » Reviewed & tested by the community

Thanks jibran

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran commented
Status: Reviewed & tested by the community » Fixed

Thank you for the review. Committed and pushed 2fe2783 to 8.x-1.x.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran commented
Title: Fixes after decoupling of entity reference selection plugins » Fixes after generic 'entity_autocomplete' Form API element
Issue summary: View changes
Priority: Normal » Critical
Status: Fixed » Active
Related issues: +#1959806: Provide a generic 'entity_autocomplete' Form API element

I have changed the title and scope of the issue so that we can fix #1959806: Provide a generic 'entity_autocomplete' Form API element here.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran commented
Priority: Critical » Major

I pushed the fix for widget aa241d1 but we still need this issue to find a proper solution.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran commented
Status: Active » Postponed
FileSize
fixes_after_generic-2411981-8.patch17.52 KB

This is postpone on #2419923: Port SA-CONTRIB-2013-096 to D8

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran commented
Status: Postponed » Needs review

Status: Needs review » Needs work

The last submitted patch, 8: fixes_after_generic-2411981-8.patch, failed testing.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran commented
Status: Needs work » Needs review
FileSize
interdiff.txt6.41 KB
fixes_after_generic-2411981-10.patch22.42 KB

I was expecting more test drama.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran commented 
+++ b/src/Plugin/Field/FieldWidget/DynamicEntityReferenceWidget.php
@@ -32,46 +35,50 @@ class DynamicEntityReferenceWidget extends EntityReferenceAutocompleteWidget {
+      '#element_validate' => array(
+        array($this, 'elementValidate'),
+        array('\Drupal\Core\Entity\Element\EntityAutocomplete','validateEntityAutocomplete'),
+      ),

I think I should create a bug against core for this.

Dave Reid’s picture

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid commented

I think the proper way to add this is:

#element_validate => array_merge(array(array($this, 'elementValidate')), element_info_property('entity_autocomplete', '#element_validate', array()));

There's no way for FAPI to know if you want the defaults merged in, or if you want to override them, so it's not a core bug.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan commented 
  1. +++ b/src/Plugin/Field/FieldWidget/DynamicEntityReferenceWidget.php
@@ -32,46 +35,50 @@ class DynamicEntityReferenceWidget extends EntityReferenceAutocompleteWidget {
+        'data-autocomplete-paths' => Json::encode($this->createAutoCompletePaths(array_keys($available))),

    Is this repeated on every target-type field? could we just store it in settings instead of repeating it? 

  2. +++ b/src/Plugin/Field/FieldWidget/DynamicEntityReferenceWidget.php
@@ -135,37 +123,23 @@ class DynamicEntityReferenceWidget extends EntityReferenceAutocompleteWidget {
+          'uid' => ($entity instanceof EntityOwnerInterface) ? $entity->getOwnerId() : \Drupal::currentUser()->id()

    We can inject this - widgets support injection right? 

  3. +++ b/src/Plugin/Field/FieldWidget/DynamicEntityReferenceWidget.php
@@ -187,49 +161,52 @@ class DynamicEntityReferenceWidget extends EntityReferenceAutocompleteWidget {
+        'selection_settings' => $settings[$target_type]['handler_settings'] ? base64_encode(serialize($settings[$target_type]['handler_settings'])) : '',

    Where is this used? I don't see unserialize or decoding happening - or is there something in core you're using here?

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran commented

Thank you @Dave Reid for the suggestion, I didn't know that. I'll fix it.
Thanks for the review @larowlan.

  1. I tried that let me write a proper fix for that.
  2. Can we do that in follow up please?
  3. Yeah core uses it in AutoComplete controller.
larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan commented

1. ta
2. sure
3. ta

amateescu’s picture

amateescu CreditAttribution: amateescu commented

Apart from #14.1, this patch looks good to me. Glad to see so much code removed :)

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran commented
FileSize
interdiff.txt3.81 KB
fixes_after_generic-2411981-18.patch22.24 KB

Here we go. Fixed #14.1 and #13.

amateescu’s picture

amateescu CreditAttribution: amateescu commented 
  1. +++ b/js/dynamic-entity-reference-widget.js
@@ -8,19 +8,22 @@
+      var $context = $(context);

    Doesn't seem to be used below. 

  2. +++ b/src/Plugin/Field/FieldWidget/DynamicEntityReferenceWidget.php
@@ -93,6 +91,11 @@ class DynamicEntityReferenceWidget extends EntityReferenceAutocompleteWidget {
+            "{$items->getName()}[$delta][target_type]" => $this->createAutoCompletePaths(array_keys($available)),

    Are you sure this works? I don't see $delta used in the JS...

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran commented
FileSize
interdiff.txt594 bytes
fixes_after_generic-2411981-20.patch22.2 KB

Thank you for the review.

  1. Removed.
  2. +++ b/js/dynamic-entity-reference-widget.js
@@ -8,19 +8,22 @@
+        $autocomplete.attr('data-autocomplete-path', settings.dynamic_entity_reference[$select[0].name][entityTypeId]);

    It translates to select name(in this case $select[0].name) which is field_der[0][target_id], field_der[1][target_id], field_der[3][target_id] and so one. I am binding the change event on that select.

.

amateescu’s picture

amateescu CreditAttribution: amateescu commented

Hm.. okay, I'll let @larowlan mark it as RTBC then.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran commented

Do you have some other implementation in mind?

amateescu’s picture

amateescu CreditAttribution: amateescu commented

Nope, not really.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan commented
Issue summary: View changes
FileSize
Screenshot 2015-02-18 19.30.07.png44.89 KB
Screenshot 2015-02-18 19.32.24.png24.05 KB

We have a UI regression here - the entity ID autocomplete now sits on the next line.

But the JavaScript elements etc all work fine, so other than the css issue/width issue - this is good to go.

Manual testing screenshot:

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran commented

I was unable to reproduce it. After increasing the text field size to 100 I was able to reproduce it but not with the default value 60.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran commented
Status: Needs review » Fixed

I tried the patch at simplytest.me and there was no UX regression. Committing this as per #21 and #24. Thank you everyone.

  • jibran committed 847ba03 on 8.x-1.x 
    Issue #2411981 by jibran, larowlan, amateescu, Dave Reid: Fixes after...
jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran commented

Oh it is an issue in firefox not in chorme

  • jibran committed aa89a7e on 8.x-1.x 
    Issue #2411981 Follow up: Fixes after generic 'entity_autocomplete' Form...
jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran commented

Fixed the issue in the follow up commit. Changed text field size from 60->40.

Status: Fixed » Closed (fixed)

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