Problem/Motivation

DER doesn't support OptionWidgets.

Proposed resolution

To make DER inline with ER let's add OptionWidgets for single value target type fields(for simplicity).
This also fixes some nasty bugs in DERItemList and DERItem which I found while fixing this.

Remaining tasks

Review

User interface changes

This patch adds the following widgets:

API changes

Hmmm let me think.

CommentFileSizeAuthor
#8 add_optionwidgets_for-2413641-8.patch23.53 KBjibran
#8 interdiff.txt2.89 KBjibran
#6 2413641-6.patch22.09 KBjibran
#6 interdiff.txt8.34 KBjibran
#5 2413641-5.patch15.35 KBjibran
#5 interdiff.txt7.06 KBjibran
#4 screenshot-d8.dev 2015-01-27 08-04-34.png2.5 KBjibran
#4 screenshot-d8.dev 2015-01-27 08-03-57.png8.63 KBjibran
#4 2413641-4.patch8.57 KBjibran
#4 interdiff.txt1.97 KBjibran
#3 der-option-widget-2413641-3.patch8.82 KBjibran
Screenshot from 2015-01-25 04:26:30.png8.91 KBjibran
screenshot-d8.dev 2015-01-25 04-29-27.png6.76 KBjibran
screenshot-d8.dev 2015-01-25 04-24-26.png3.23 KBjibran
screenshot-d8.dev 2015-01-25 04-23-20.png2.89 KBjibran
der-option-widget.patch31.83 KBjibran
der-option-widget-do-not-test.patch8.82 KBjibran

Comments

jibran’s picture

jibran
he/him
Primary language Urdu
Location Toronto, Canada
commented
Title: Add OptionWidgets for single value target type DER fields » Add OptionWidgets for single value target type DER fields

Do not test patch is without #2411981: Fixes after generic 'entity_autocomplete' Form API element fix.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented 
  1. +++ b/src/Plugin/Field/FieldWidget/DynamicEntityReferenceOptionsTrait.php
@@ -0,0 +1,96 @@
+    return count(DynamicEntityReferenceItem::getTargetTypes($field_definition->getSettings())) == 1;

    Neat 

  2. +++ b/src/Plugin/Field/FieldWidget/DynamicEntityReferenceOptionsTrait.php
@@ -0,0 +1,96 @@
+      list($values[$index]['target_type'], $values[$index]['target_id']) = explode('-', $value['target_id']);

    Cool trick

Is there somewhere we're checking the user has access to the entity label? E.g. we don't want to be showing node titles if the node is unpublished and the user doesn't have access. Similar stuff to what we do with formatters. I assume it defers to something in ER in core? But best check because otherwise we might have security issue.

What happens if there are 1000s of nodes and someone (stupidly) uses this? Should we check that in the is available method too? To make sure that people don't bust their site.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Toronto, Canada
commented
Issue tags: +Needs manual testing
StatusFileSize
new der-option-widget-2413641-3.patch8.82 KB

Is there somewhere we're checking the user has access to the entity label? E.g. we don't want to be showing node titles if the node is unpublished and the user doesn't have access. Similar stuff to what we do with formatters. I assume it defers to something in ER in core? But best check because otherwise we might have security issue.

From DynamicEntityReferenceItem::getSettableOptions()

    foreach (array_keys($target_types) as $target_type) {
      $options[$target_type] = \Drupal::service('plugin.manager.dynamic_entity_reference_selection')->getSelectionHandler($field_definition, $this->getEntity(), $target_type)->getReferenceableEntities();
    }

so the answer is we are getting reference-able entities form ER selection plugins in core which are completely secure.

What happens if there are 1000s of nodes and someone (stupidly) uses this? Should we check that in the is available method too? To make sure that people don't bust their site.

Isn't ER has the same problem? We can create evil like views_field_view.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Toronto, Canada
commented
Issue summary: View changes
Issue tags: +Needs tests
StatusFileSize
new interdiff.txt1.97 KB
new 2413641-4.patch8.57 KB
new screenshot-d8.dev 2015-01-27 08-03-57.png8.63 KB
new screenshot-d8.dev 2015-01-27 08-04-34.png2.5 KB 
+++ b/src/Plugin/Field/FieldWidget/DynamicEntityReferenceOptionsTrait.php
@@ -29,16 +29,6 @@ trait DynamicEntityReferenceOptionsTrait {
-  protected function supportsGroups() {

Now select list supports option groups.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Toronto, Canada
commented
StatusFileSize
new interdiff.txt7.06 KB
new 2413641-5.patch15.35 KB

Added tests for defaultValues.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Toronto, Canada
commented
Issue tags: -Needs tests
StatusFileSize
new interdiff.txt8.34 KB
new 2413641-6.patch22.09 KB

And we have a core bug.

Status: Needs review » Needs work

The last submitted patch, 6: 2413641-6.patch, failed testing.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Toronto, Canada
commented
StatusFileSize
new interdiff.txt2.89 KB
new add_optionwidgets_for-2413641-8.patch23.53 KB

Created #2426781: Custom OptionWidget have no empty option label for fails in #6. Meanwhile the work around.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Toronto, Canada
commented
Status: Needs work » Needs review
larowlan’s picture

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

jibran+=1000
love the new test coverage

jibran’s picture

jibran
he/him
Primary language Urdu
Location Toronto, Canada
commented

Thanks for the review and kind words.

  • jibran committed 956a444 on 8.x-1.x 
    Issue #2413641 by jibran: Add OptionWidgets for single value target type...
jibran’s picture

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

Status: Fixed » Closed (fixed)

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