D7's Entity reference implements a generic and plugable reference field, that can replace the taxonomy term and provide more functionality.

What's making ER powerful, is the concept of the plugin-types: "Selection" and "Behavior".

Selection plugin

Every field can have a single selection plugin, which will determine which entities are referencable. For example OG 7.x-2.x uses this set the group-audience field based on the group membership.

Behavior plugin

This is, as DamZ said, "field API implenented as CTools plugins". Currently the field API hooks aren't "real" hooks in the sense that just a single function is called. The behavior plugins allow implementing modules to attach their own unique "behavior" -- prepopulate a field value; Update the {taxonomy_index} table; etc'.
Possibly, the "behavior" concept should be extended to all fields not just ER.
Work on the behavior plugin is done in #1803064: Horizontal extensibility of Fields: introduce the concept of behavior plugins

Steps to test

#313 (originally found in #199 and #278)

Follow-up issues

#1847608: Improve CommentSelection access control
#1847588: Implement the new entity field API for the Entity-Reference field type
#1847590: Fix UX of entity reference field type selection
#1847594: Replace File and Image field types with Entity-reference
#1847596: Remove Taxonomy term reference field in favor of Entity reference
#1847600: Use proper entity 'view' access checks in entity reference
#1847602: Allow grouping in Entity-reference View's style plugin
#1847606: Improve FileSelection access control
#1847608: Improve CommentSelection access control
#1847924: Allow Entity-Reference field to autocreate items
#1891558: Improve TermSelection access control
#201 2. Provide an example Views configuration, for example "Content by user", so more advanced users can see the ER display style in use
#1904882: Entity reference field Reference method Default settings order should be Filter by then Sort by
#1904892: Add Entity reference fields like: User reference, Content reference, File reference
#1904896: when create view from entity reference field instructions to create one, default to have entity reference display
#1904900: Default auto pick the primary field for the type in view wizard to avoid red error: 'Display "EntityReference" needs a selected
#1904902: replace "autocomplete" in modal with more accurate word and replace "Search field" during entity reference view wizard process
#1906806: Provide a relationship for each entity reference field.
#1953568: Make the bundle settings for entity reference fields more visible.

CommentFileSizeAuthor
#325 entityreference-1801304-changelog.patch447 bytesamateescu
#320 entityreference-1801304-320.patch213.64 KBamateescu
#320 interdiff.txt3.13 KBamateescu
#319 1801304-319.patch213.5 KBstar-szr
#319 interdiff.txt26.59 KBstar-szr
#312 er-interdiff-312.txt4.17 KBamitaibu
#312 entityreference-1801304-312.patch213.57 KBamitaibu
#303 er-interdiff-299.txt5.89 KBamitaibu
#303 entityreference-1801304-302.patch213.57 KBamitaibu
#289 entityreference-1801304-289.patch212.59 KBamateescu
#289 interdiff.txt1.95 KBamateescu
#286 entityreference-1801304-286.patch212.81 KBeffulgentsia
#286 interdiff.txt1.05 KBeffulgentsia
#284 entityreference-1801304-284.patch212.09 KBeffulgentsia
#284 interdiff.txt9.86 KBeffulgentsia
#270 er-interdiff-270.txt936 bytesamitaibu
#270 entityreference-1801304-270.patch212.16 KBamitaibu
#266 interdiff.txt25.52 KBamateescu
#266 entityreference-1801304-266.patch212.13 KBamateescu
#262 interdiff.txt820 bytesamateescu
#262 entityreference-1801304-262.patch212.81 KBamateescu
#260 interdiff.txt3.05 KBamateescu
#260 entityreference-1801304-260.patch212.81 KBamateescu
#252 er-interdiff-252.txt5.83 KBamitaibu
#252 entityreference-1801304-252.patch214.9 KBamitaibu
#250 er-interdiff-250.txt8.87 KBamitaibu
#250 entityreference-1801304-250.patch209.07 KBamitaibu
#248 er-interdiff-248.txt5.83 KBamitaibu
#248 entityreference-1801304-248.patch212.84 KBamitaibu
#247 er-interdiff-247.txt23.66 KBamitaibu
#247 entityreference-1801304-247.patch207.01 KBamitaibu
#243 er-interdiff-243.txt39.26 KBamitaibu
#243 entityreference-1801304-243.patch164.82 KBamitaibu
#238 entityreference-1801304-237.patch183.36 KBamateescu
#238 interdiff-230-237-do-not-test.patch2.7 KBamateescu
#233 reference-type.jpeg58.14 KBBojhan
#233 target-type.jpeg45.81 KBBojhan
#230 entityreference-1801304-230.patch180.3 KBamateescu
#230 er-230.png892.02 KBamateescu
#229 entityreference-1801304-229.patch379.36 KBamateescu
#229 interdiff-228-229-do-not-test.patch11.76 KBamateescu
#228 entityreference-1801304-228.patch181.26 KBamitaibu
#228 interdiff-228.txt23.43 KBamitaibu
#223 er-223.txt2.81 KBamitaibu
#216 interdiff-216.txt18 KBamitaibu
#216 entityreference-1801304-216.patch141.74 KBamitaibu
#212 merge-two-fields-more-clear.png61.98 KBBojhan
#211 clean-up-selection-method.png24.75 KBBojhan
#211 merge-fields.png56.52 KBBojhan
#208 entityreference-1801304-208.patch134.73 KBamateescu
#206 entityreference-1801304-206.patch134.69 KBamateescu
#206 interdff-178-206-do-not-test.patch12.22 KBamateescu
#199 views-none-found.png28 KBwebchick
#199 views-wizard.png43.5 KBwebchick
#199 view-add-er-display.png54.51 KBwebchick
#199 views-er-error.png38.08 KBwebchick
#199 views-er-settings.png30.81 KBwebchick
#199 views-er-style-options.png37.98 KBwebchick
#199 views-er-preview.png12.51 KBwebchick
#199 views-preview-alpha-editor.png10.27 KBwebchick
#196 entity-selection-node-form.png41.22 KBwebchick
#195 article-add-field.png53.39 KBwebchick
#195 select-entity-type.png47.15 KBwebchick
#195 entity-selection.png23.88 KBwebchick
#195 entity-selection-sort.png29.75 KBwebchick
#195 entity-selection-property.png33.34 KBwebchick
#195 entity-selection-field.png41.95 KBwebchick
#178 entityreference-1801304-177.patch133.52 KBamitaibu
#178 interdiff-177.txt3.8 KBamitaibu
#170 entityreference-1801304-169.patch132.96 KBamitaibu
#170 interdiff-169.txt90.08 KBamitaibu
#172 entityreference-1801304-172.patch133.47 KBamateescu
#172 interdiff-170-172-do-not-test.patch1.01 KBamateescu
#161 entityreference-1801304-161.patch133.04 KBamitaibu
#161 interdiff-161.txt6.27 KBamitaibu
#158 entityreference-1801304-157.patch131.91 KBamitaibu
#158 interdiff-157.txt5.7 KBamitaibu
#152 Screenshot 11:20:12 10:43 PM.png23.61 KBamitaibu
#150 1801304-er-150.patch131.93 KBamitaibu
#150 interdiff-150.txt10.06 KBamitaibu
#148 1801304-er-148.patch127.11 KBamitaibu
#148 interdiff-148.txt29.63 KBamitaibu
#142 1801304-er-142.patch125.79 KBamitaibu
#142 interdiff-142.txt2.51 KBamitaibu
#138 1801304-er-138.patch125.6 KBamateescu
#138 interdiff-129-138-do-not-test.patch451 bytesamateescu
#134 core-add-entity_reference-field-1801304-134.diff134.22 KBFabianx
#134 core-add-entity_reference-field-1801304-134.diff134.22 KBFabianx
#129 1801304-er-129.patch125.6 KBamitaibu
#121 1801304-er-121.patch121.39 KBamitaibu
#121 interdiff-121.txt1.65 KBamitaibu
#119 1801304-er-119.patch121.13 KBamitaibu
#119 interdiff-119.txt4.51 KBamitaibu
#111 1801304-er-111.patch120.12 KBamitaibu
#111 interdiff-111.txt5.11 KBamitaibu
#96 1801304-er-96.patch118.83 KBamateescu
#96 interdiff-95-96-do-not-test.patch27.99 KBamateescu
#95 1801304-er-95.patch118.58 KBamitaibu
#95 interdiff-95.txt22.87 KBamitaibu
#92 1801304-er-92.patch96.52 KBamitaibu
#92 interdiff-92.txt8.78 KBamitaibu
#91 1801304-er-91.patch96.33 KBamateescu
#91 interdiff-89-91-do-not-test.patch7.17 KBamateescu
#89 1801304-er-89.patch96.33 KBamateescu
#89 interdiff-88-89-do-not-test.patch6.78 KBamateescu
#88 interdiff-85-88-do-not-test.patch960 bytesamateescu
#85 1801304-er-85.patch96.14 KBamitaibu
#85 interdiff-85.txt1.13 KBamitaibu
#82 1801304-er-81.patch96 KBamitaibu
#82 interdiff-81.txt1.62 KBamitaibu
#80 1801304-er-80.patch95.12 KBamitaibu
#80 interdiff-80.txt13.21 KBamitaibu
#77 1801304-er-core-77.patch91.81 KBamateescu
#77 interdiff-70-77-do-not-test.patch12.27 KBamateescu
#75 1801304-er-75.patch92.29 KBamitaibu
#75 interdiff.txt12.99 KBamitaibu
#70 1801304-er-core-70.patch90.05 KBamitaibu
#70 interdiff-do-not-test.patch6.86 KBamitaibu
#68 1801304-er-core-68.patch90.57 KBamitaibu
#68 interdiff-do-not-test.patch4.51 KBamitaibu
#67 1801304-er-core-66.patch90.19 KBEclipseGc
#57 1801304-er-core-56.patch86.91 KBamitaibu
#57 interdiff-1801304-er-core-56-do-not-test.patch22.78 KBamitaibu
#53 Screen Shot 2012-10-21 at 22.00.36.png11.13 KBswentel
#52 1801304-er-core-52.patch86.67 KBamitaibu
#52 interdiff-1801304-er-core-52-do-not-test.patch1.1 KBamitaibu
#50 1801304-er-core-50.patch86.74 KBamitaibu
#50 interdiff-1801304-er-core-50-do-not-test.patch29.22 KBamitaibu
#45 1801304-er-core-44.patch87.84 KBamateescu
#45 interdiff-41-44-do-not-test.patch2.22 KBamateescu
#41 1801304-er-core-41.patch87.84 KBamateescu
#41 interdiff-35-41-do-not-test.patch41.77 KBamateescu
#35 1801304-er-core-35.patch88.36 KBamitaibu
#33 1801304-er-core-33.patch88.36 KBamitaibu
#29 1801304-er-core-29.patch81.95 KBamitaibu
#27 drupal-1801304-27.patch81.92 KBtim.plunkett
#22 1801304-er-core-22.patch81.91 KBamitaibu
#13 er1.jpg12.49 KBamitaibu
#13 er2.jpg16.05 KBamitaibu
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

bojanz’s picture

As discussed with DamZ, we'll want to do "Entityreference with selection plugins" in one patch, and the "Behavior plugins" in another patch, to minimize bike shedding.

Seeing how the selection plugins look will help people to realize how broken and incomplete entity access is, as well.

I guess the first step would be to create a 8.x-1.x branch of Entityreference and port the code, then divide it and post patches?

sun’s picture

Priority: Normal » Major
Issue tags: +Killer End-User Features

Yes, ideally perform the entire port in a new 8.x-x.x branch in the project's existing contrib repository. This means you may grant commit access to further core contributors, you have an existing bug tracker, related issues, etc.pp. There's even a chance for backporting fixes to the D7 branch. It also means that there would be ready code to release in case this issue won't make it.

Once complete, you can use git-subtree to merge the entire repo+branch into the Drupal core tree and create a patch out of that. git-subtree also allows you to commit further adjustments to the subtree and push them, but also to pull in changes from upstream; i.e., it works both ways. We had great success with this approach for #501434: Move Link/URL field type into core already.

amitaibu’s picture

we'll want to do "Entityreference with selection plugins"

Yes, I agree we should start with that, and behaviors can com later - ER specific, or for all fields.

yched’s picture

Note that "field types as plugins" is definitely on the roadmap (#1785256: Widgets as Plugins is in, #1785748: Field formatters as plugins is near-RTBC, field types are next - not before november, though). I'm not sure how the "behavior" plugin would be different.

Also, when field types are plugins (which they aren't for now, and a D8 port for core should most probably not stay postponed on that), "X reference" field types very much smell like plugin derivatives to me.

andypost’s picture

Entity refs are mentioned in summary #1346214: [meta] Unified Entity Field API

DamienMcKenna’s picture

I know we're not supposed to do this, but it needs to be said: +1!!!

cosmicdreams’s picture

Exciting! So is the best way to help this effort forward to do the following:

1. Goto http://drupal.org/project/issues/entityreference?categories=All
2. get commit access to new branch
3. start converting entityreference to use CMI and other D8 features?

bojanz’s picture

Assigned: Unassigned » bojanz

I have commit access and I'm planning to do just that.
Ping me on IRC if you want to help (and I'll try to create a set of issues in the next few days so that we can segment the work).

amitaibu’s picture

@bojanz, happy you start working on it. I've committed a completely messy, not working untested code to 1801304-amitaibu in ER I've done yesterday. Maybe you'll be able to use it.
Did I mention it's messy and not working? :)

bojanz’s picture

@Amitaibu
Wouldn't want it any other way :D

amitaibu’s picture

I'll play around with it a bit more -- first time PSRing! So you can have a look at it tomorrow.

swentel’s picture

@bojanz - just pushed #1043198: Convert view modes to ConfigEntity forward, something that entity reference field can benefit from as well.

amitaibu’s picture

FileSize
16.05 KB
12.49 KB

@bojanz, want to see something cool? Lots of @todos, but still :)

andypost’s picture

Please, take care of proper separation of instance and field level settings as discussed in #1340098: Move target bundle to the field instance settings

Damien Tournoud’s picture

@andypost: the target entity type will have to stay a field setting, the rest can move to an instance setting.

Damien Tournoud’s picture

@yched: the Behavior system of Entity Reference is about horizontal extensibility of fields. It allow you to extend a Field using one or more additional behaviors, in a very clean way. It's complementary, but actually very independent of the Field API as plugins.

Damien Tournoud’s picture

andypost’s picture

We can have a great usage of ER for shortcut module to reference a menu-link entities
Related issues
#916388: Convert menu links into entities
#1811640: Convert shortcuts into configuration
#1802750: [Meta] Convert configurable data to ConfigEntity system

amitaibu’s picture

@Bojanz,
I've pushed my recent work to 1801304-amitaibu in ER. Basically it's ER7 ported to 8, we still need to do some cleanup, and tests, but it's already working.

amitaibu’s picture

  • I have ported the admin UI test to D8. The Selection access test doesn't work on my local, hopefully testbot can help get a clue
  • Following Link and Views project, I've created a 8.x-1.x branch in ER, to make the code more visible, and allow opening issues in ER for 8.x
amitaibu’s picture

Sweet, all the tests are now fixed and green :)

amitaibu’s picture

Status: Active » Needs work
FileSize
81.91 KB

Still work in progress, but worth uploading a first patch to get some initial reviews.

falcon03’s picture

Hi Amitaibu,

one of the things that makes me prefer ER instead of term reference is the ability to add field values "on the fly", even using the "select list" widget (provided by entity connect).

Will this feature be moved to core too?
I think it is really a good one and it will be great to have it in core!

amitaibu’s picture

amitaibu’s picture

Status: Needs work » Needs review

Setting to needs review for testbot.

Status: Needs review » Needs work

The last submitted patch, 1801304-er-core-22.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
81.92 KB

Rerolling, adding use Drupal\Core\Database\Database;

amitaibu’s picture

FileSize
81.95 KB

Sorry, I think I rolled a wrong version in #22, retrying.

nils.destoop’s picture

I see that you are still using hook_field_widget_info. Widgets are beïng converted to plugins. The changelog can be found at http://drupal.org/node/1796000

nils.destoop’s picture

Status: Needs review » Needs work
amitaibu’s picture

Assigned: bojanz » amitaibu

I'll be working on converting to widget and formatter to plugins.

amitaibu’s picture

Status: Needs work » Needs review
FileSize
88.36 KB

Widgets and formatters converted to plugins.

yched’s picture

Hard for me to do a real, in-depth review, but here are a couple notes from visual inspection.

+function entityreference_get_selection_handler($field, $instance = NULL, EntityInterface $entity = NULL) {
+  $plugin = entityreference_get_plugin_manager('selection')->getDefinition($field['settings']['handler']);
+  $class = $plugin['class'];
+
+  if (class_exists($class)) {
+    return call_user_func(array($class, 'getInstance'), $field, $instance, $entity);
+  }
+
+  return Drupal\entityreference\Plugin\Type\Selection\SelectionBroken::getInstance($field, $instance, $entity);
+}

This and the factory presented as a static getInstance() method in the SelectionInterface itself is completely out of line with the usual instantiation flow promoted by the Plugin API (which is to call PluginManager::getInstance()). Could we get back to the regular Plugin API flow, or at least comment why a different flow has to be used ?

+/**
+ * Implements hook_field_widget_info_alter().
+ *
+ * @todo: Move this to plugin alteration after:
+ * http://drupal.org/node/1751234
+ * http://drupal.org/node/1705702
+ */
+function entityreference_field_widget_info_alter(&$info) {
+  if (module_exists('options')) {
+    $info['options_select']['field types'][] = 'entityreference';
+    $info['options_buttons']['field types'][] = 'entityreference';
+  }
+}

I think this this the correct way to do it, and it should work right now, the @todo can be removed.

+/**
+ * Interface definition for field widget plugins.
+ *
+ * This interface details the methods that most plugin implementations will want
+ * to override. See Drupal\field\Plugin\Type\Selection\SelectionBaseInterface for base
+ * wrapping methods that should most likely be inherited directly from
+ * Drupal\entityreference\Plugin\Type\Selection\SelectionBase..
+ */
+interface SelectionInterface  {

That phpdoc looks like an unedited copy/paste :-)

+  /**
+   * Overrides Drupal\Component\Plugin\PluginManagerBase::getDefinition().
+   *
+   * @todo Remove when http://drupal.org/node/1778942 is fixed.
+   */
+  public function getDefinition($plugin_id) {
+    $definition = $this->discovery->getDefinition($plugin_id);
+    if (!empty($definition)) {
+      $this->processDefinition($definition, $plugin_id);
+      return $definition;
+    }
+  }

Obsolete, can be removed.

Also, the patch still contains mentions of EntityReferenceHandler in phpdocs, while it seems this is now SelectionInterface ?

+class SelectionBase extends PluginSettingsBase implements SelectionInterface {

You don't seem to use PluginSettingsBase methods - besides, SelectionBase::construct() receives no settings, so I assume a Selection object has no settings of its own (in the sense that a widget or a formatter have settings), and you have no use for that superclass - wrong copy/paste from widgets code ? :-)

+  public function settingsSummary() {
+    $summary = array();
+    $settings = $this->settings;
+
+    $entity_info = entity_get_info($this->field['settings']['target_type']);
+    $summary[] = t('Rendered as @mode', array('@mode' => isset($entity_info['view modes'][$settings['view_mode']]['label']) ? $entity_info['view modes'][$settings['view_mode']]['label'] : $settings['view_mode']));
+    $summary[] = !empty($settings['links']) ? t('Display links') : t('Do not display links');
+
+    return implode('<br />', $summary);
+  }

Please don't use $this->settings directly, but $this->getSetting('foobar') (like your current settingsForm() above does). I didn't specifically check the rest of the code, but this applies to all widgets and formatters code. When field types get converted to plugins, that's also how field and instance settings will be accessed, FWIW.

+abstract class DefaultAutocompleteWidget extends WidgetBase {

According to our current standards, this should be AutocompleteWidgetBase.

+  public function formElement(array $items, $delta, array $element, $langcode, array &$form, array &$form_state) {
+    // We let the Field API handles multiple values for us, only take
+    // care of the one matching our delta.
+    if (isset($items[$delta])) {
+      $items = array($items[$delta]);
+    }
+    else {
+      $items = array();
+    }

I'm not in front of the upstream code, but this looks like uneeded precaution, or it would be needed on each and every existing widget implementation.

amitaibu’s picture

FileSize
88.36 KB

Thanks yched! I've addressed issues from #34

I have moved the getInstance() logic into entityreference_get_selection_handler() but it is still very hardcoded. I Don't know exactly what's the best way to solve this. Also all the current sub-selection handlers (e.g. SelectionEntityTypeFile) have @todo code.
I think bojanz has some plans about it -- that involves entity_access()

+/**
+ * Implements hook_field_widget_info_alter().
+ *
+ * @todo: Move this to plugin alteration after:
+ * http://drupal.org/node/1751234
+ * http://drupal.org/node/1705702
+ */

This doesn't work. As far as I can see, there is no longer hook_field_widget_info_alter() in core.

yched’s picture

Status: Needs review » Needs work

As far as I can see, there is no longer hook_field_widget_info_alter() in core

Yeah, sorry, my bad. That's because #1705702: Provide a way to allow modules alter plugin definitions got in without WidgetPluginManager being updated to include the new AlterDiscoveryDecorator. #1817180: Tests: hook_widget_info_alter() is not called anymore is open for this.

But your code as it stands now should work once this one gets fixed.

amitaibu’s picture

The test fails in Drupal\file\Tests\FileManagedFileElementTest which doesn't seem to be related to this patch.

This test also passes on my local.

amateescu’s picture

Status: Needs work » Needs review
FileSize
41.77 KB
87.84 KB

Gave this patch some core love:
- moved the selection plugin manger instantiation to the DIC, where it belongs
- removed the ugly hack for getting the right selection class for core entity types
- a full rundown on the docs
- and a lot of other small fixes all around, it's all in the interdiff :)

tim.plunkett’s picture

+++ b/core/modules/entityreference/entityreference.moduleundefined
@@ -0,0 +1,394 @@
+  $plugin = drupal_container()->get('plugin.manager.entityreference.selection')->getDefinition($field['settings']['handler']);
+  $class = $plugin['class'];
+
+  if (class_exists($class)) {
+    return call_user_func(array($class, 'getInstance'), $field, $instance, $entity);
+  }
+
+  return SelectionBroken::getInstance($field, $instance, $entity);

When we do something similar to this in Views, we use a try/catch instead, with no need for getDefinition(). Not sure which is better? Also, why the call_user_func?

+++ b/core/modules/entityreference/lib/Drupal/entityreference/Plugin/entityreference/selection/SelectionBase.phpundefined
@@ -0,0 +1,323 @@
+    // Remove 'Drupal' from the entity class.
+    $class_name = '\Drupal\entityreference\Plugin\Type\Selection' . substr($entity_info['entity class'], 6);
+
+    if (class_exists($class_name)) {
+      return new $class_name($field, $instance, $entity);
+    }
+    else {
+      return new SelectionBase($field, $instance, $entity);

This just looks weird.

yched’s picture

Status: Needs review » Needs work

It's still unclear why the object creation flow is so different from the usual Plugin API one (getInstace() method on the manager class plus __construct() methods on the plugin classes, rather than getInstance() static methods on th plugin classes here).
We should switch to the regular flow, or document why it can't apply here

amateescu’s picture

Status: Needs work » Needs review
FileSize
2.22 KB
87.84 KB

Fixed the tests and the first comment from #42. The second one needs more thought.

amateescu’s picture

It's still unclear why the object creation flow is so different from the usual Plugin API one (getInstace() method on the manager class plus __construct() methods on the plugin classes, rather than getInstance() static methods on th plugin classes here).

Because no one bothered to update this part of D7 code to new Plugin API from D8 :) But I agree, we should switch to that.

amitaibu’s picture

Status: Needs review » Needs work

Oh, the patch in #34 wasn't my most updated one -- don't know what I did wrong -- where I removed the getInstance() :(

@amateescu,
Any chance we can work on a sandbox for easier re-rolls?

amitaibu’s picture

I'll re-roll - removing getInstance() and some widget/ formatter fixes I've already done.

amitaibu’s picture

Status: Needs work » Needs review
FileSize
29.22 KB
86.74 KB

Ok, from now on, no uploading of wrong patches from my side! ;)

  • Patch moves getInstance login into entityreference_get_selection_handler()
  • Rename widget/ formatter base classes
amitaibu’s picture

Assigned: amitaibu » Unassigned

Also un-assigning myself, as others are, thankfully, working on this as-well.

amitaibu’s picture

Patch removes PluginSettingsBase as previously commented by yched in #34

swentel’s picture

Gave this a quick spin and this already looks really really great. I tested referencing basic pages from the article content type and that seems to work out nicely.
One minor thing I came across is the selection of the 'Rendered as entity' formatter, other than that, code in general is looking nice, so wow, yes, let's try to get this in! :)

+++ b/core/modules/entityreference/lib/Drupal/entityreference/Plugin/field/formatter/EntityReferenceEntityFormatter.phpundefined
@@ -0,0 +1,117 @@
+ *   settings = {
+ *     "view_mode" = "",

Because this is empty by default, the formatter summary returns nothing when you switch over to this formatter, see attached screenshot.

+++ b/core/modules/entityreference/lib/Drupal/entityreference/Plugin/field/formatter/EntityReferenceEntityFormatter.phpundefined
@@ -0,0 +1,117 @@
+    $view_mode = $this->getSetting('view_mode');
+    $summary[] = t('Rendered as @mode', array('@mode' => isset($entity_info['view modes'][$view_mode]['label']) ? $entity_info['view modes'][$view_mode]['label'] : $view_mode));

Maybe, in case the view mode is empty (because you switch to this formatter from another one), we should print out another message to inform people they should select a view mode ? Or simply go for a default. To be honest, haven't really looked what the behavior is in the D7 version.

Screen Shot 2012-10-21 at 22.00.36.png

amitaibu’s picture

Patch moves getInstance login into entityreference_get_selection_handler()

The code in #50 for entityreference_get_selection_handler() is still wrong, not sure what's the best approach.

* If the handler == base we need to check if there's a entity-type specifc class, and call it; otherwise call the base class; otherwise BrokenHanler.
* If the handler == og we need to follow similar logic.

So we probably need some naming convention, and we'll have to use class_exists() for checking the entity-type specific handler.

Damien Tournoud’s picture

Assigned: Unassigned » amitaibu

It's still unclear why the object creation flow is so different from the usual Plugin API one (getInstace() method on the manager class plus __construct() methods on the plugin classes, rather than getInstance() static methods on th plugin classes here).
We should switch to the regular flow, or document why it can't apply here

We have a single selection plugin (the base one):

+/**
+ * Plugin implementation of the 'selection' entityreference.
+ *
+ * @Plugin(
+ *   id = "base",
+ *   module = "entityreference",
+ *   label = @Translation("Simple selection")
+ * )
+ */
+class SelectionBase extends PluginSettingsBase implements SelectionInterface {

... that needs to workaround various quirks about core entities and thus needs different implementations depending on the entity type.

So we have a factory method on the plugin class (public static getInstance($entity_type, ...)) that returns the proper implementation. Is there a better way to allow multiple implementations per plugins depending of the parameters?

amitaibu’s picture

Assigned: amitaibu » Unassigned
FileSize
22.78 KB
86.91 KB

Patch moves entity-type specific selection under the handler name (e.g. Drupal\entityreference\Plugin\Type\Selection\base\user\User)

amitaibu’s picture

Cross post with #56.

@Damz, what do you think about my solution from #57?

Berdir’s picture

Noticed two things in the patch...

+++ b/core/modules/entityreference/lib/Drupal/entityreference/EntityreferenceBundle.phpundefined
@@ -0,0 +1,25 @@
+  public function build(ContainerBuilder $container) {
+    // register the BlockManager class with the dependency injection container.
+    $container->register('plugin.manager.entityreference.selection', 'Drupal\entityreference\Plugin\Type\SelectionPluginManager');

Comment looks wrong here, probably copied from somewhere else?

+++ b/core/modules/entityreference/lib/Drupal/entityreference/Plugin/field/formatter/EntityReferenceEntityFormatter.phpundefined
@@ -0,0 +1,117 @@
+      unset($entity->content);
+      $elements[$delta] = entity_view($entity, $view_mode, $langcode);

Are you sure the unset() is necessary? I think content is initialized an empty array by default, there's even a test in core for this.

cweagans’s picture

Status: Needs review » Needs work

I proposed adding entityreference to core in IRC just now and was directed here. I don't really have anything to add, other than +1 :)

yched’s picture

re plugin instanciation : #57 looks a bit better :-)
Now what's missing is;
- the logic within entityreference_get_selection_handler() should go into into SelectionPluginManager::getInstance().
- when its logic is done, SelectionPluginManager::getInstance() should then call $this->factory->createInstance() to create the actual object. SelectionPluginManager::__construct() currently misses declaring a factory for the plugin type, ReflectionFactory could do the job.
- entityreference_get_selection_handler() should then simply do:
$plugin = drupal_container()->get('plugin.manager.entityreference.selection')->getInstance($options);
(not sure then whether entityreference_get_selection_handler() is worth keeping as a separate function, your call)

yched’s picture

+/**
+ * Interface definition for field widget plugins.
+ *
+ * This interface details the methods that most plugin implementations will want
+ * to override. See Drupal\field\Plugin\Type\Selection\SelectionBaseInterface for base
+ * wrapping methods that should most likely be inherited directly from
+ * Drupal\entityreference\Plugin\Type\Selection\SelectionBase.
+ */

That comment is still wrongly copy/pasted from widgets :-). "Interface definition for reference field selection plugins." should be enough.

yched’s picture

I finally made sense of those lines :

+ // Remove 'Drupal' from the entity class.
+  $class_name = '\Drupal\entityreference\Plugin\Type\Selection\\' . $handler . substr($entity_info['entity class'], 6);
+
+  if (class_exists($class_name)) {
+    // Entity-type specific class.
+    return new $class_name($field, $instance, $entity);
+  }

So this means the actual Selection class that is used is strictly derived from the entity class - one selection class per entity class, or SelectionBase as default.

Then I'd say this is actually not a case for plugins. The classes are not interchangeable, but directly and imperatively derived from the entity class - and actually, as @Damz pointed, the patch only has one single plugin class, extended by classes that are not plugin themselves.

What we're talking about is more a 'reference_controller', whose class name should be set as part of entity_info(), like storage controller, form controller or view controller. No need to leverage the plugin API here, it only adds needless overhead.
It also means that the actual classes can be provided by each entity module rather than entity_reference.

tim.plunkett’s picture

What we're talking about is more a 'reference_controller', whose class name should be set as part of entity_info(), like storage controller, form controller or view controller. No need to leverage the plugin API here, it only adds needless overhead.
It also means that the actual classes can be provided by each entity module rather than entity_reference.

Yes! This makes much more sense to me.

amitaibu’s picture

But if OG wants to provide its own selection handler? Same entity type -- different selection logic.

EclipseGc’s picture

FileSize
90.19 KB

So, I don't promise that this code works, but organizationally it should be better.

I've moved all the classes around to make each implementation a first class plugin. Added a derivative handler to the base plugin that removes supported entity_types from it's list (it might should also check if there's a base table, I leave that as an exercise to other). I added an entityreference_selection alter callback (via the AlterDecorator).

In addition to all of the the SelectionBroken logic really seemed out of place, so I overrode the createInstance method of the plugin manager (oh added reflection factory there) so now a simple call to createInstance should result in an instantiated plugin, or the SelectionBroken fallback if no class was available. DamZ and I discussed a custom mapper but I didn't attempt to do that as I think Amateescu likely has a better handle on what should be there than I do.

I've not updated any of the code that was actually using plugins, just the plugin code itself. Hopefully this is helpful here.

Eclipse

amitaibu’s picture

Status: Needs work » Needs review
FileSize
4.51 KB
90.57 KB

Some fixes to make EclipseGc's code work (EDIT: work as in not WSOD -- didn't change selection_handler() yet)

amitaibu’s picture

Status: Needs review » Needs work
FileSize
6.86 KB
90.05 KB

Moved logic out of get_selection_handler() -- however, I'm only getting the Base class, and I never see the code in Drupal\entityreference\Plugin\Derivative\SelectionBase being called. @EclipseGC, am I passing something wrong to the plugin manager ?

Interdiff against #67

joshmiller’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 1801304-er-core-70.patch, failed testing.

bojanz’s picture

I'm assuming that at one point ER should stop storing ids and start storing uuids.

Not sure if that should be a part of the initial patch or not.

A definite followup would be to make it support referencing a specific revision of an entity as well.

amitaibu’s picture

@amateescu,
Are you going to re-roll with a fix to the Derivative code?

amitaibu’s picture

Status: Needs work » Needs review
FileSize
12.99 KB
92.29 KB

Re-rolled as per #1821060: Return values from getReferencableEntities() keyed by bundle
Taxonomy terms now appear with hierarchy, and if there are multiple bundles they appear as optgroup

Status: Needs review » Needs work

The last submitted patch, 1801304-er-75.patch, failed testing.

amateescu’s picture

Status: Needs work » Needs review
FileSize
12.27 KB
91.81 KB

Here's the update that I promised :) It doesn't include the changes from #75 because I think we should have that patch commited to the 7.x version before rolling it here.

I started a new branch in yched's Field API sandbox so it's easier to track progress: http://drupalcode.org/sandbox/yched/1736366.git/shortlog/refs/heads/enti...

amitaibu’s picture

Status: Needs review » Needs work
+++ b/core/modules/entityreference/lib/Drupal/entityreference/Plugin/entityreference/selection/User.phpundefined
@@ -20,6 +20,8 @@
+ *   entity_types = {"user"},
+ *   weight = 1,

Why add a 'weight' property? Wouldn't a 'base' property (TRUE/ FALSE) be better to indicate which one is a base and which is a derivative?

amitaibu’s picture

Status: Needs work » Needs review
FileSize
13.21 KB
95.12 KB

I've committed #1821060: Return values from getReferencableEntities() keyed by bundle to 7.x, so added it to 8.x as-well.

amitaibu’s picture

FileSize
1.62 KB
96 KB

Following 7.x -- Split autocomplete callback to allow other modules (e.g. OG, OG-vocab ) to re-use ER's code.

amitaibu’s picture

I don't know why simpltest are failing, there are running locally ok.

amitaibu’s picture

FileSize
1.13 KB
96.14 KB

Followup on 7.x.
Now that Views is in core, we should also add a ViewsBase selection handler.

amitaibu’s picture

I've started porting the Views selection handler in yched's sandbox under entityreference-1801304-views. Don't hesitate to land a hand ;)

amateescu’s picture

Status: Needs review » Needs work
FileSize
960 bytes

Pushed a small update for entityreference_options_list() from #1821060-7: Return values from getReferencableEntities() keyed by bundle. Now let's look at the tests :)

amateescu’s picture

Status: Needs work » Needs review
FileSize
6.78 KB
96.33 KB

Let's see if this one fixes the tests.

amateescu’s picture

Getting there :)

amitaibu’s picture

FileSize
8.78 KB
96.52 KB

follow up on 7.x -- getReferencableEntities() should return Bundle's machine-name, and those should become a label in hook_options_list()

falcon03’s picture

tagging to let our accessibility team to review this patch before committing

falcon03’s picture

tagging; something went wrong with the previous comment and tags haven't been added.

amitaibu’s picture

FileSize
22.87 KB
118.58 KB

I think we are getting close, probably can start getting some reviews.. :)

  • Add View selection handler. Thanks @amateescu!
  • Add ER to options_widget_info() so it works in select list.

@amateescu,
What do you think about #79?

amateescu’s picture

Let's save reviewers some time; I did another run-down on documentation and some other minor code cleanup.

About #79: we provide a weight setting in order to allow other plugins to take over our core entity handling if they need to :)

tstoeckler’s picture

+++ b/core/modules/field/modules/entityreference/css/entityreference.admin.css
--- /dev/null
+++ b/core/modules/field/modules/entityreference/entityreference.info

I know we recently introduced keyvalue into core, instead of key_value, but for modules so far we have been consistent in splitting words with an underscore. I think entity_reference.module would be a better choice.

+++ b/core/modules/field/modules/entityreference/entityreference.module
@@ -0,0 +1,463 @@
+      // Default to the core target entity type node.
+      'target_type' => 'node',

Don't know if we should keep the default or not, but if we do, this needs to be wrapped in a module_exists('node'), I think. I didn't actually try what happens when you disable node.module and add an entity reference field, so maybe this is bogus.

+++ b/core/modules/field/modules/entityreference/entityreference.module
@@ -0,0 +1,463 @@
+    'default_widget' => 'options_list',

I stumbled on this, as options.module is not required by entityreference.module, but apparently options.module does the same thing with list.module. Hmm...

Leaving at needs review.

ParisLiakos’s picture

+1 for entity_reference.
entityreference always bugged me:/

amateescu’s picture

Fixed all the points from #97 in http://drupalcode.org/sandbox/yched/1736366.git/commitdiff/b01d222d0f9bc.... Not posting a new patch because everything needs to be updated for EFQ v2.

amitaibu’s picture

> everything needs to be updated for EFQ v2.

I'll give it a shot.

amitaibu’s picture

Started a entityreference-1801304-efq2 branch.
I wonder if efq2 will allow us to remove some of the entityFieldQueryAlter() implementations (maybe in User?)

yched’s picture

Thanks for the awesome work, folks ! Will try to do another review when I get back to a coding environment in a couple days.

One design topic I'd like to see discussed however is :
"One single field type with target entity type being a property" (what entityreference D7 and the current patch do)
vs
"One different field type per target entity type" (what userref / noderef / references did in D6/D7)

Could we get a summary of the reasoning that led to this design in entityreference D7 ? Was it a deliberate choice or did it derive from technical limitations ?

I personally see arguments in favor of separate field types:

- UI wise, as a site admin, I'd rather do :
1) in 'add new field', see 'User reference', 'Node reference', ... 'X reference' in the list and pick the one i want,
rather than
1) in 'add new field', pick an abstract, catch-all 'Reference' type, then
2) in the following screen, select the target entity type as a nested setting that I won't be able to change later on anyway.

- Also, on the list of existing fields presented on the 'Manage fields' overview, the current approach means all you see is that there are 'reference' fields, with no notion the target entity type, you need to open the field settings to know what the field actually references.
This is not ideal to make sense of the content architecture, esp. if we want at some point to have taxo fields, file/image fields replaced by reference fields.

- Similarly, code wise, when building logic on the reference tree (e.g do something for all file fields), it means you can't know the target entity type without loading the full field definition, which is costly CPU and memory wise.
Since #1040790: _field_info_collate_fields() memory usage, we're trying very much to avoid "load all field definitions", "load all instance definitions" calls, because those just cannot be made to scale. I might even even file an issue to remove field_info_fields() / field_info_instances(NULL, NULL) in D8.
The new field_info_field_map() gets you a lightweight map of fields, containing only the field type. But for reference fields in the current approach, that would not be enough, the field type tells you nothing.

- Widgets and formatters are 'per field type'. The current approach means you can't have a widget that's valid for 'file references' only.
Granted, the "separate field types" approach would mean a widget needs a way to declare it applies for all '*_reference' field types, but that one seems more solvable than the other way around.

cosmicdreams’s picture

+++ b/core/modules/field/modules/entityreference/lib/Drupal/entityreference/Plugin/views/row/Entityreference.phpundefined
@@ -0,0 +1,61 @@
+    return parent::pre_render($row);

+++ b/core/modules/views/lib/Drupal/views/Plugin/entityreference/selection/Views.phpundefined
@@ -0,0 +1,218 @@
+      $entities = $this->view->executeDisplay($display_name, $args);

pre_render from the Fields is this

/**
   * Allow the style to do stuff before each row is rendered.
   *
   * @param $result
   *   The full array of results from the query.
   */
  function pre_render($result) { }

which returns nothing, so this overridden pre_render shouldn't return anything either.

+++ b/core/modules/views/lib/Drupal/views/Plugin/entityreference/selection/Views.phpundefined
@@ -0,0 +1,218 @@
+      $entities = $this->view->executeDisplay($display_name, $args);

are we sure this function returns an array?

amitaibu’s picture

amitaibu’s picture

  • Merged EFQ into main branch, although it's not complete until #1828408: Re-add addTag() and AddMetaData() to EFQ
  • Started entityreference-1801304-102 branch, to split ER into different field types. EDIT: Currently hook_field_widget_info_alter() is missing, so I've hardcoded some entities into the widgets and formatters
lpalgarvio’s picture

#102

Having a single field with property has some perks.
- you can change the field property until it has data: this means more flexibility
- it doesn't "polute" the field type with as many entity types you have in the site (node, user, taxonomy, product, etc, can be as many as 10 in a standard instalation): this means more organization

please keep the original design of a single field type + property, like in entityreference D7.

amateescu’s picture

There are advantages and disadvantages for both approaches. We discussed this during the Gent sprint with yched and Damien but unfortunately we weren't able to reach an agreement.

On one hand, yched is really against the current approach because of UX, performance and widget/formatter concerns, and on the other hand Damien is really against chaging the current approach without having a sub-field-type concept in field api. I'm at a loss on how we can solve this in order to move forward here..

The latest work for field type per entity type is in the entityreference-1801304-102 branch from yched's field api sandbox, but I'm not pursuing it anymore because the workarounds that we have to do there are getting quite ugly.

Until we have some consensus for the issue above, I'm pursuing other things that everyone seems to agree on:
- UUID support (a separate 'target_uuid' column in the field table because in #1726734: Replace serial entity IDs with UUIDs in URLs, or even globally? it was decided to keep referencing by id internally)
- move selection handler settings from field level to instance level settings (except target_type, of course)

yakoub’s picture

yched, i have little experience in field implementation but i think all the points you talked about in #102 should actually be fixed in field api itself and not require entityreference to accommodate this seemingly field api "weakness"

Fabianx’s picture

#107: So now that we know the positions, what are the reasons and especially goals Damien and yched have for their respective approaches?

I mean the general goal is clear: entity_reference in core

* Why does yched want to have a widget per field?
* Why does Damien want to change field api first?

to reach this goal?

sun’s picture

How much different are the implementation approaches in real terms?

Can't we go with a single field type first, and rather easily turn that into a derivative-whatever later on?

(assuming field types will turn into plugins very soonish)

amitaibu’s picture

FileSize
5.11 KB
120.12 KB

While we figure out #107, I've Added revision ID column. There's no UI for it, but if you set the value programmatically, and use the "Render entity" formatter you can see the difference. No tests yet.

Test will fail here until #1828408: Re-add addTag() and AddMetaData() to EFQ is in (it passes locally with that patch applied, tough)

eaton’s picture

Status: Needs review » Needs work

I'm definitely a little late to the game, but wanted to chime in with some thoughts on the "one field type versus many" question that's come up.

Many of our projects have somewhat complex content models with many relationships, and many require custom (or customized) widgets to smooth things for content editors. We've made equal use of Node/User Reference and EntityReference, and I think the difference is a bit of a wash. Most of the times we've needed to parse the field info to find the entity-type it points to have been restricted to low-traffic administrative pages where the slight hit wasn't noticable. And from a UX perspective, having special-use widgets that say, 'Sorry, I can only be used with Users' is no worse than the proliferation of near-duplicate field types.

In some ways it's like the file field problem: If someone creates a widget that only makes sense for audio files, or PDF files, does that imply the need for mimetype-specific file fields, or is it just a configuration gotcha to keep in mind? I'd prefer a universal field, with ongoing future work to give widgets more control of where they're made available, to a system that enforces entity-specific reference fields.

amitaibu’s picture

Issue summary: View changes

Updated issue summary.

amitaibu’s picture

fyi, I've started to work on the "Behaviors plugins" in #1803064: Horizontal extensibility of Fields: introduce the concept of behavior plugins, (until we figure out #107, and get ER in)

amitaibu’s picture

Status: Needs work » Needs review
FileSize
4.51 KB
121.13 KB

Better handle getting base-table in entityFieldQueryAlter()

amitaibu’s picture

FileSize
1.65 KB
121.39 KB

Re-key items array if entity was deleted.

Regarding #119 -- tests are passing locally for me..

yched’s picture

Status: Needs review » Needs work

re @eaton #113

Most of the times we've needed to parse the field info to find the entity-type it points to have been restricted to low-traffic administrative pages where the slight hit wasn't noticable

What I'm worried about is house-keeping stuff like our current file_get_file_references (runs every time a file field value gets removed - dunno how much this qualifies as low-traffic)

And from a UX perspective, having special-use widgets that say, 'Sorry, I can only be used with Users' is no worse than the proliferation of near-duplicate field types.

This probably applies for custom widgets developed for your site's specific use case, I've done this myself, widgets or formatters coded for a specific field on a specific node type.
But do we want to have this UX for the 98% case of adding taxonomy or images to your articles ? I do remember the negative feedback when adding taxonomy on a node moved from "create a vocab and hit the "node types" checkboxes on the create form" to "create a vocab and then go add a 'taxo ref' field on each of your node types"...

Also, I have trouble convincing myself that having your "Manage fields" list of fields on a node type only say "there are those three 'reference' fields", with no clue to what they reference other than the convention you might have applied on the field name, does not impact grasping the content model. But well, I lack personal battlefield experience with D7 entityreference, TBH.

yched’s picture

More generally on this question of "one single field type" / "one field type per target entity type" :

In the end, I don't really care how entity_ref does its job internally, as long as I don't have to maintain it :-).
My feeling is that :
- IMO this can only get in core if there's a plan to make it *replace* file fields, image fields & taxo fields, because duplicate field types would be confusing
- In doing so, we cannot degrade "too much" the UX of tasks that are likely to be the 1st things a newcomer tries on his freshly installed site (add an image or file upload fields, add taxonomy)
But in the end I won't be the keeper of *those* gates.

What I do know as a Field API maintainer is : while we can wish for a more granular way to say which widgets / formatters are eligible for which field, that's much more easily said than cleanly done :-).
For instance:
a) The exact granularity needed is unclear. "Field subtype", ugh, what is that exactly ?
It sounds like what is needed is a tag system - each existing field has tags derived of its configuration, and widgets say which tags they work with.
b) Potential for confusion and unpredictability - "what exactly to I have to configure where so that Widget 'foo' becomes available ?"
b) The tags on a field cannot change after field creation, or you find the widget you configured for the field does not apply anymore because you made this or that config change.
c) This does not play well with the current Field UI way of working (the list of widgets is presented upfront on "Manage Fields / add new field", and the field is created before you actually configure its properties). This can be changed, of course, and possibly should, but that's not a small preliminary task.
d) We still need to know a "default" widget applicable for all the fields.
That's a very non-minor change, it's very late in the D8 cycle to explore this, and I myself have more than enough on my plate already, so I'm not exactly looking forward to this...

Having separate field types doesn't require that much legwork on the underlying API. The problem of "sea of field types available in the 'add new field' dropdown" is strictly a UI issue, can be worked around.
- Config entities will account for at least 80% of the number of entity types. For starters, do we really want to have config entities in there ? "breakpoint reference", "input format reference", really ? It seems config entities have very different needs / capabilities in terms of widgets and formatters...
- Organizing the list of field type in optgroups ('Text', 'Number', 'List', 'Reference') would come a long way to make this dropdown more usable. That's a really low-hanging fruit, just one additional entry in widget plugin annotations...

eaton’s picture

Hmmmm.

I'm begrudgingly coming around to your perspective, yched -- there ARE an awful lot of special cases to be handled there, and I can see the file reference in particular being thorny if it's handled generically. I think I'll back slowly away from this and concede that, as long as there's a way for widgets and formatters to announce that they apply to any reference field, the problems with individual field types aren't as serious.

If there were a way for us to create a "generic entityreference", in which the id and entity type were both stored, it might make more sense to have a single field type. Having a field that links to a comment OR a user OR a node etc is a use case that would require something generic, but for the moment even the generic reference field doesn't support that.

amitaibu’s picture

@yched,

IMO this can only get in core if there's a plan to make it *replace* file fields, image fields & taxo fields, because duplicate field types would be confusing

Taxonomy term -- absolutely, that's also stated as one of my goals in the issue summary. I've actually haven't thought about file/ image, for those we should first find the UI solution that will allow assigning the file-widget only to "File reference" entities.

It seems the UX questions might take awhile to be discussed and solved. I think it should be as follow-up issues (as well as deprecating taxonomy-term field), once ER gets in.

@eaton,

If there were a way for us to create a "generic entityreference", in which the id and entity type were both stored, it might make more sense to have a single field type. Having a field that links to a comment OR a user OR a node etc is a use case that would require something generic, but for the moment even the generic reference field doesn't support that.

This is, if I understand you correctly, the current implementation. The field is generic, catering all entity-types. Regarding "which the id and entity type were both stored" In D7 we had the "target_type" inside the field, so theoretically you could reference via a single field multilve entity-types, but this was deprecated, as most implementing modules (e.g. Views) expect a single entity type to be referenced - and I think it's the right behavior.

sun’s picture

"target_type" inside the field, so theoretically you could reference via a single field multilve entity-types, but this was deprecated

Funnily, I assumed that this most basic and most generic case would be covered by ER in core. Adding constraints is more functionality, not less, no? ;)

Any chance we could retain the target_type field schema column and just pollute it, but don't necessarily use it in core? I understand there might be other components that won't support multiple different types, but I do see quite some potential for contrib and custom sites/modules to build on top of the core ER field instead of building an entirely new, if we'd store the target type.

eaton’s picture

I think the main lesson to learn from this thread is that I shouldn't post in issues while I"m high on cold medicine. Carry on, everyone. CARRY ON! ;-)

amitaibu’s picture

Status: Needs work » Needs review
FileSize
125.6 KB

> Carry on, everyone. CARRY ON! ;-)

Yap :)

@amateescu,
Can you figure out why tests are not working on testbot?
Anyway, since I was in the area, I've added a test for sorting.

amitaibu’s picture

Status: Needs review » Needs work

The test bot reports:

Symfony\Component\DependencyInjection\Exception\InvalidArgumentException: The service definition "plugin.manager.entity_reference.selection" does not exist. in Symfony\Component\DependencyInjection\ContainerBuilder->getDefinition() (line 690 of /var/lib/drupaltestbot/sites/default/files/checkout/core/vendor/symfony/dependency-injection/Symfony/Component/DependencyInjection/ContainerBuilder.php).

This doesn't happen locally, though. Seems related to the DIC?

chx’s picture

I do not see the new property API being implemented. See #1839070: Implement the new entity field API for the taxonomy reference field type and the exisiting EntityReferenceItem. Also, regarding multiple entity types referenced, EFQ is getting relationship support #1446600: EntityFieldQuery (pseudo-)join support with a syntax like ->condition('field_name.entity.foo') now if field_name can refer multiple entity types then what am I going to join in for foo?? If it's strictly one entity type then of course 'entity' can be resolved based on field_name settings (and if you implement the property API with an entity api constraint and an id source then EFQ will already do that) What you can do, sure you can, and relation will need to, anyways, is that you can implement what I call a relationship specifier for every entity type being referenced so that you can do ->condition('field_name.node.foo') and ->condition('field_name.users.foo') and so on.

Anyways, for now I strongly recommend not doing that and just keeping with a single entity type so that the uniform field_name.entity.foo syntax can be used.

Fabianx’s picture

Fabianx’s picture

Status: Needs work » Needs review
FileSize
134.22 KB
134.22 KB

Lets try that with the patch from #133 as multi patch.

EDIT: Oops, somehow I triggered a race condition that created the attachment twice.

amitaibu’s picture

I'll be working on #132, although it can also be a follow up patch.
The current hold-back is the failing tests, which are for an unknown reason. Help is appreciated :)

amateescu’s picture

Responding to the bat signal :)

amitaibu’s picture

fyi, some discussion with fago on the property metadata implementation

amitaibu’s picture

Test are passing, thanks amateescu!

nevergone’s picture

I'm fast tested:

- Download and install latest Drupal 8
- Apply the patch, cache clear
- Generate nodes and users
- Create two entity reference, target: node and user

The reference is works well, if:
Entity selection:
Mode: Simple selection
Sort by: Don't sort

But not works (empty result):
Sort by: A property of the base table of the entity
and Sort property is anything

amitaibu’s picture

FileSize
2.51 KB
125.79 KB

@nevergone, thanks. Indeed there was an error in the code and in the test :/

Attached the fixed code.

nevergone’s picture

Test:

- Download and install latest Drupal 8
- Apply the patch, cache clear
- Generate nodes, comments and users
- Create three entity reference:
source: user -> target: node, source: comment -> target: user

source: user -> target: node
widget: Select list
mode: simple selection
target bundles: Article
Sort by: Don't sort
default value: - None -
Display mode: no link

Create and edit existing user: Works well, correct operation

create another field (source: user -> target: node)
target bundles: Article
default value: some kind of Page type

Result: Works well: the field default value: - None -

source: comment -> target: user
widget: Check boxes/Radio buttons
mode: simple selection
sort by: A property of the base table of the entity
sort property: name
sort direction: Descending
default value: N/A
display mode: link

Create and edit existing user: Works well, but if select Anonymous, then fatal error, error message:

PHP Fatal error: Call to a member function label() on a non-object in /var/www/core/modules/field/modules/entity_reference/lib/Drupal/entity_reference/Plugin/field/formatter/EntityReferenceLabelFormatter.php on line 67
PHP Stack trace:
PHP 1. {main}() /var/www/index.php:0
PHP 2. Symfony\Component\HttpKernel\Kernel->handle() /var/www/index.php:35
PHP 3. Drupal\Core\HttpKernel->handle() /var/www/core/vendor/symfony/http-kernel/Symfony/Component/HttpKernel/Kernel.php:193
PHP 4. Symfony\Component\HttpKernel\HttpKernel->handle() /var/www/core/lib/Drupal/Core/HttpKernel.php:51
PHP 5. Symfony\Component\HttpKernel\HttpKernel->handleRaw() /var/www/core/vendor/symfony/http-kernel/Symfony/Component/HttpKernel/HttpKernel.php:73
PHP 6. call_user_func_array() /var/www/core/vendor/symfony/http-kernel/Symfony/Component/HttpKernel/HttpKernel.php:129
PHP 7. Drupal\Core\EventSubscriber\{closure}() /var/www/core/vendor/symfony/http-kernel/Symfony/Component/HttpKernel/HttpKernel.php:0
PHP 8. call_user_func_array() /var/www/core/lib/Drupal/Core/EventSubscriber/LegacyControllerSubscriber.php:52
PHP 9. node_page_view() /var/www/core/lib/Drupal/Core/EventSubscriber/LegacyControllerSubscriber.php:0
PHP 10. node_show() /var/www/core/modules/node/node.module:2451
PHP 11. node_view_multiple() /var/www/core/modules/node/node.module:1114
PHP 12. entity_view_multiple() /var/www/core/modules/node/node.module:2371
PHP 13. Drupal\Core\Entity\EntityRenderController->viewMultiple() /var/www/core/includes/entity.inc:542
PHP 14. module_invoke_all() /var/www/core/lib/Drupal/Core/Entity/EntityRenderController.php:128
PHP 15. call_user_func_array() /var/www/core/includes/module.inc:988
PHP 16. comment_node_view() /var/www/core/includes/module.inc:0
PHP 17. comment_node_page_additions() /var/www/core/modules/comment/comment.module:714
PHP 18. comment_view_multiple() /var/www/core/modules/comment/comment.module:741
PHP 19. entity_view_multiple() /var/www/core/modules/comment/comment.module:1042
PHP 20. Drupal\Core\Entity\EntityRenderController->viewMultiple() /var/www/core/includes/entity.inc:542
PHP 21. Drupal\comment\CommentRenderController->buildContent() /var/www/core/lib/Drupal/Core/Entity/EntityRenderController.php:121
PHP 22. Drupal\Core\Entity\EntityRenderController->buildContent() /var/www/core/modules/comment/lib/Drupal/comment/CommentRenderController.php:30
PHP 23. field_attach_view() /var/www/core/lib/Drupal/Core/Entity/EntityRenderController.php:63
PHP 24. field_invoke_method() /var/www/core/modules/field/field.attach.inc:1458
PHP 25. Drupal\field\Plugin\Type\Formatter\FormatterBase->view() /var/www/core/modules/field/field.attach.inc:181
PHP 26. Drupal\entity_reference\Plugin\field\formatter\EntityReferenceLabelFormatter->viewElements() /var/www/core/modules/field/lib/Drupal/field/Plugin/Type/Formatter/FormatterBase.php:101

create another field (source: comment -> target: user)
widget: select
mode: simple selection
sort by: A field attached to this entity
select field and sort direction

Create and edit existing user: Works well.

dawehner’s picture

What a great effort to get this into core. It would be amazing to have this super-useful feature into core.
Here are some points, nothing big but it's too late for a longer review ;)
Feel free to contact me, so we can work out the hook_views_data views integration (probably a follow up). In general you could try to save some time by getting just the base in, until december.

+++ b/core/modules/field/modules/entity_reference/entity_reference.moduleundefined
@@ -0,0 +1,472 @@
+  $items['entity_reference/autocomplete/single/%/%/%'] = array(
...
+  $items['entity_reference/autocomplete/tags/%/%/%'] = array(

Not sure, but if you have parameters at the end you currently don't need to specify them explicitly.

+++ b/core/modules/field/modules/entity_reference/entity_reference.moduleundefined
@@ -0,0 +1,472 @@
+          'message' => t('The referenced entity (@type: @id) is invalid.', array('@type' => $field['settings']['target_type'], '@id' => $id)),

What means invalid, couldn't you say that the entities don't exist?

+++ b/core/modules/field/modules/entity_reference/entity_reference.moduleundefined
@@ -0,0 +1,472 @@
+  if (isset($element['#ajax']) && $element['#ajax'] === TRUE) {

You could simplify this to !empty($element['#ajax'])

+++ b/core/modules/field/modules/entity_reference/entity_reference.moduleundefined
@@ -0,0 +1,472 @@
+function entity_reference_query_entity_reference_alter(AlterableInterface $query) {
...
+  $handler->entityFieldQueryAlter($query);

Just in general i'm wondering whether this could use efqv2 directly and deal with that so you could use other entity storage backends.

+++ b/core/modules/field/modules/entity_reference/entity_reference.moduleundefined
@@ -0,0 +1,472 @@
+  if ($entity_id !== 'NULL') {

'NULL' as strings seems to be a bit odd.

+++ b/core/modules/field/modules/entity_reference/entity_reference.moduleundefined
@@ -0,0 +1,472 @@
+    // TODO: Improve when we have entity_access().

Better use "@todo Improve when ..." here.

+++ b/core/modules/field/modules/entity_reference/entity_reference.moduleundefined
@@ -0,0 +1,472 @@
+
+  if ($type == 'tags') {
...
+  else {

What about just use drupal_explode_tags first and then count() and the items. See taxonomy_autocomplete() for a similar behavior. This also saves us one menu item.

+++ b/core/modules/field/modules/entity_reference/lib/Drupal/entity_reference/Plugin/Type/Selection/SelectionBroken.phpundefined
@@ -0,0 +1,65 @@
+    $this->field = $field;
+    $this->instance = $instance;

Nitpick: You should probably also document these fields.

+++ b/core/modules/field/modules/entity_reference/lib/Drupal/entity_reference/Plugin/Type/SelectionPluginManager.phpundefined
@@ -0,0 +1,84 @@
+      return new SelectionBroken($configuration['field'], $configuration['instance']);

What about use return parent::createInstance('broken') instead? Ah i see, the broken class is not an actual plugin.

+++ b/core/modules/field/modules/entity_reference/lib/Drupal/entity_reference/Plugin/entity_reference/selection/SelectionGeneric.phpundefined
@@ -0,0 +1,311 @@
+      elseif (count($entities) > 5) {
...
+      elseif (count($entities) > 1) {

Nice idea!

+++ b/core/modules/field/modules/entity_reference/lib/Drupal/entity_reference/Plugin/entity_reference/selection/SelectionGeneric.phpundefined
@@ -0,0 +1,311 @@
+        // More helpful error if there are only a few matching entities.

What about appending the second part of the other error message here as well?

+++ b/core/modules/field/modules/entity_reference/lib/Drupal/entity_reference/Plugin/entity_reference/selection/Term.phpundefined
@@ -0,0 +1,81 @@
+    $vocabulary_alias = $query->innerJoin('taxonomy_vocabulary', 'n', '%alias.vid = ' . $base_table . '.vid');

Just linking to #1552396: Convert vocabularies into configuration as this has to be reworked based on the other issue.

+++ b/core/modules/field/modules/entity_reference/lib/Drupal/entity_reference/Plugin/views/display/EntityReference.phpundefined
@@ -0,0 +1,166 @@
+ * "entity_reference_display" is a custom property, used with
+ * views_get_applicable_views() to retrieve all views with a
+ * 'Entity Reference' display.
...
+  *  entity_reference_display = TRUE

Just throwing out some idea: Couldn't you use 'type' => 'entity_reference' for that, similar to Feed.php does it.

+++ b/core/modules/field/modules/entity_reference/lib/Drupal/entity_reference/Plugin/views/display/EntityReference.phpundefined
@@ -0,0 +1,166 @@
+ *   use_ajax = FALSE,
+ *   use_pager = FALSE,
+ *   accept_attachments = FALSE,

This three properties should be defined on the class as properties.

+++ b/core/modules/field/modules/entity_reference/lib/Drupal/entity_reference/Plugin/views/display/EntityReference.phpundefined
@@ -0,0 +1,166 @@
+
+    // Make sure the query is not cached.
+    $options['defaults']['default']['cache'] = FALSE;
+
+    // Set the display title to an empty string (not used in this display type).
+    $options['title']['default'] = '';
+    $options['defaults']['default']['title'] = FALSE;

If it makes sense to hide the config of cache/title from the UI, you could use optionsSummary and just not return the appropriate items.

+++ b/core/modules/field/modules/entity_reference/lib/Drupal/entity_reference/Plugin/views/display/EntityReference.phpundefined
@@ -0,0 +1,166 @@
+    if (!empty($this->view->result) || !empty($this->view->style_plugin->definition['even empty'])) {

You could use the even_empty method on the style plugin.

+++ b/core/modules/field/modules/entity_reference/lib/Drupal/entity_reference/Plugin/views/display/EntityReference.phpundefined
@@ -0,0 +1,166 @@
+    // Make sure the id field is included in the results, and save its alias
+    // so that references_plugin_style can retrieve it.
+    $this->id_field_alias = $id_field = $this->view->query->add_field($this->view->storage->base_table, $this->view->storage->base_field);
+    if (strpos($id_field, '.') === FALSE) {
+      $id_field = $this->view->storage->base_table . '.' . $this->id_field_alias;
+    }

You can assume that the id_field is the $this->view->storage->get('base_field') now (see Sql::add_field()) Btw. i think the properties on storage are protected now.

+++ b/core/modules/field/modules/entity_reference/lib/Drupal/entity_reference/Plugin/views/style/EntityReference.phpundefined
@@ -0,0 +1,105 @@
+  protected $usesRowPlugin = TRUE;
...
+  protected $usesFields = TRUE;

You could also document them via "Overrides \Drupal\views\Plugin\style\StylePluginBase::$usesRowPlugin".

+++ b/core/modules/field/modules/entity_reference/lib/Drupal/entity_reference/Plugin/views/style/EntityReference.phpundefined
@@ -0,0 +1,105 @@
+    if (isset($form['grouping'])) {
+      $options = $form['grouping'][0]['field']['#options'];

It seems to be for me that you want to not allow grouping for entity reference style plugins, so $this->usesGrouping = FALSE; and use $this->displayHandler->getFieldLabels(TRUE); instead

+++ b/core/modules/field/modules/entity_reference/lib/Drupal/entity_reference/Plugin/views/style/EntityReference.phpundefined
@@ -0,0 +1,105 @@
+
+    // Play nice with Views UI 'preview': if the view is not executed through
+    // Drupal\views\Plugin\entity_reference\selection\Views::getReferencableEntities(),
+    // just display the HTML.
+    if (empty($options)) {
+      return parent::render();
+    }

What about output the array in a human readable way via print_r or var_export with a pre around it (similar to Feed.php). In general this overriding can be done in preview()

+++ b/core/modules/views/lib/Drupal/views/Plugin/entity_reference/selection/Views.phpundefined
@@ -0,0 +1,220 @@
+    $options = array();
+    foreach ($displays as $data) {
+      list($view, $display_id) = $data;
+      if ($view->storage->base_table == $entity_info['base table']) {
+        $options[$view->storage->name . ':' . $display_id] = $view->storage->name . ' - ' . $view->storage->display[$display_id]['display_title'];
+      }

It seems to be that we have to improve views_get_views_as_options to be more generic.

+++ b/core/modules/views/lib/Drupal/views/Plugin/entity_reference/selection/Views.phpundefined
@@ -0,0 +1,220 @@
+    if (!$this->view || !$this->view->storage->getDisplay($display_name) || !$this->view->access($display_name)) {

It's enough to just check the access as the view access method takes care about invalid displays.

+++ b/core/modules/views/lib/Drupal/views/Plugin/entity_reference/selection/Views.phpundefined
@@ -0,0 +1,220 @@
+      watchdog('entity_reference', 'The view %view_name is no longer eligible for the %field_name field.', array('%view_name' => $view_name, '%field_name' => $this->instance['label']), WATCHDOG_WARNING);

I'm wondering whether we want to throw exceptions instead of watchdog messages in d8 for semi bad things like that.

+++ b/core/modules/views/lib/Drupal/views/Plugin/entity_reference/selection/Views.phpundefined
@@ -0,0 +1,220 @@
+      $entities = entity_load_multiple($target_type, array_keys($result));

This piece of code does not work because the result array keys are just ascending array indexes. (0, 1, 2 ...)
What you probably want is to just get the entity from $view->result[$row_index]->_entity as it's loaded there already.

+++ b/core/modules/views/lib/Drupal/views/Plugin/entity_reference/selection/Views.phpundefined
@@ -0,0 +1,220 @@
+    return $this->view->total_items;

You could use $this->view->pager->get_total_items() which is a bit safer.

+++ b/core/modules/views/lib/Drupal/views/Plugin/entity_reference/selection/Views.phpundefined
@@ -0,0 +1,220 @@
+      $entities = $this->view->executeDisplay($display_name, $args);
+      $result = array_keys($entities);

What about let Sql::load_entities() return all the entities?

amitaibu’s picture

@nevergone,
I've pushed a fix to #143

@dawehner,
Thanks for the review. I'll go over the non-Views issues first.

Berdir’s picture

Not sure, but if you have parameters at the end you currently don't need to specify them explicitly.

Support for this might be removed and is not yet supported by the new router system. And even if not, it's better to explicitly define them if they're required.

chx’s picture

Note: EFQ expects you to have entity type in constraints and "id source" in settings for getPropertyDefinitions().

amitaibu’s picture

FileSize
29.63 KB
127.11 KB

@dawehner, I haven't addressed:

'NULL' as strings seems to be a bit odd

I don't remember now, but there was a reason for it, I'll need to try and remember :)

What about just use drupal_explode_tags first and then count() and the items. See taxonomy_autocomplete() for a similar behavior. This also saves us one menu item.

I actually think it's a bit more readable as it is. It allows us to understand the matches can be for a tags style autocomplete VS a single autocomplete per delta. Maybe better docs would be ok for you?

Just throwing out some idea: Couldn't you use 'type' => 'entity_reference' for that, similar to Feed.php does it.

This doesn't seem to be in Feed.php, and we need it for views_get_applicable_views() - did you mean something else?

What about let Sql::load_entities() return all the entities?

Sorry, didn't understand what you mean?

amitaibu’s picture

'NULL' as strings seems to be a bit odd

I don't remember now, but there was a reason for it, I'll need to try and remember :)

Damien remembers :)

DamZ: amitaibu: the NULL in just a placeholder for the URL
[10:46am] DamZ: amitaibu: most web servers don't accept empty path parts like "toto//titi"
[10:46am] amitaibu: DamZ: thanks. yeah, I wasn't sure why we didn't use an empty string, though
[10:46am] amitaibu: DamZ: and remember there was a reason
[10:46am] amitaibu: DamZ: I'll add your comment to the issue
[10:46am] DamZ: amitaibu: we initially did, but most web servers actually fold "//" into "/"

amitaibu’s picture

FileSize
10.06 KB
131.93 KB

Added test for Views' entity-reference selection handler.

So, when does this patch, like hmm, you know... gets committed? ;)

amitaibu’s picture

Assigned: Unassigned » amitaibu
FileSize
23.61 KB

Failing tests don't seem to be related to ER. Let's try again.

DamienMcKenna’s picture

Status: Needs review » Needs work

The failing tests are all for Views, so maybe wait a day for someone to fix it on the Views queue?

(in other words, argh)

tim.plunkett’s picture

@DamienMcKenna, Views is in core, it's no different than any core module in terms of fixing failures. Have you tried running those tests locally? Those failures look like they'd be pretty evident on a local test run.

amitaibu’s picture

@tim.plunkett,
Oops, indeed they fail also locally. Investigating :/

amitaibu’s picture

I've renamed the test View, but the tests will still fail.
dawehner said he will have a look at it later on.

andyceo’s picture

Status: Needs work » Needs review
amitaibu’s picture

dawehner suggested via IRC to move the exported View to entity-reference-test module.

amitaibu’s picture

Green!

nevergone’s picture

Status: Needs review » Reviewed & tested by the community

I'm tested extensively (multiple sort mode: entity propery, field, views), and works well.
Great works!

Berdir’s picture

Status: Reviewed & tested by the community » Needs review

Here's a nitpick review. Looks quite nice, noticed quite a few @todo's and comments about limitations that ER has to work around. Can we get a few follow-up issues and link them in the issue summary? We actually have a chance to fix the source and clean up the code here now :)

Same for follow-up tasks to attempt to replace something like the taxonomy term reference field with this.

I also think this needs a sign-off from the field system maintainers, so setting back to needs review.

+++ b/core/modules/field/modules/entity_reference/entity_reference.installundefined
@@ -0,0 +1,48 @@
+  // Create a foreign key to the target entity type base type.
+  // @todo It's still not safe to call entity_get_info() in here..
+//  $entity_type = $field['settings']['target_type'];
+//  $entity_info = entity_get_info($entity_type);
+//
+//  $base_table = $entity_info['base_table'];
+//  $id_column = $entity_info['entity_keys']['id'];
+//
+//  $schema['foreign keys'][$base_table] = array(
+//    'table' => $base_table,
+//    'columns' => array('target_id' => $id_column),

Should be intended correctly, even when commented out like this.

+++ b/core/modules/field/modules/entity_reference/entity_reference.moduleundefined
@@ -0,0 +1,472 @@
+    'description' => t('This field reference another entity.'),

That sounds... wrong, should be referenceS I think.

+++ b/core/modules/field/modules/entity_reference/lib/Drupal/entity_reference/EntityReferenceBundle.phpundefined
@@ -0,0 +1,26 @@
+/**
+ * @file
+ * Definition of Drupal\entity_reference\EntityReferenceBundle.

Should be Contains... now

+++ b/core/modules/field/modules/entity_reference/lib/Drupal/entity_reference/EntityReferenceRecursiveRenderingException.phpundefined
@@ -0,0 +1,14 @@
+ * @file
+ * Definition of Drupal\entity_reference\EntityReferenceRecursiveRenderingException.
+ */
+
+namespace Drupal\entity_reference;
+
+/**
+ * Exception thrown when the entity view renderer goes into a potentially
+ * infinite loop.
+ */
+class EntityReferenceRecursiveRenderingException extends \Exception {}

I think we can simplify the class name to RecursiveRenderingException. No need to repeat the module name.

+++ b/core/modules/field/modules/entity_reference/lib/Drupal/entity_reference/Plugin/Type/Selection/SelectionInterface.phpundefined
@@ -0,0 +1,79 @@
+   * @return int|null
+   *   Value of a matching entity ID, or NULL if none.

We have a mess with this already, but I think it should be integer, not int.

+++ b/core/modules/field/modules/entity_reference/lib/Drupal/entity_reference/Plugin/entity_reference/selection/SelectionGeneric.phpundefined
@@ -0,0 +1,334 @@
+   * Builds an EntityFieldQuery to get referencable entities.
+   */
+  public function buildEntityQuery($match = NULL, $match_operator = 'CONTAINS') {
+    $target_type = $this->field['settings']['target_type'];
...
+   * Helper method: Passes a query to the alteration system again.
+   *
+   * This allows Entity Reference to add a tag to an existing query so it can
+   * ask access control mechanisms to alter it again.
+   */
+  protected function reAlterQuery(AlterableInterface $query, $tag, $base_table) {

Should have some basic @param's.

+++ b/core/modules/field/modules/entity_reference/lib/Drupal/entity_reference/Plugin/entity_reference/selection/Term.phpundefined
@@ -0,0 +1,81 @@
+
+/**
+ * @file
+ * Definition of Drupal\entity_reference\Plugin\entity_reference\selection\Term.

I think someone already suggested that these should be moved to the corresponding modules, is there a reason for not doing it?

+++ b/core/modules/field/modules/entity_reference/lib/Drupal/entity_reference/Plugin/entity_reference/selection/User.phpundefined
@@ -0,0 +1,85 @@
+          // Sadly, the Database layer doesn't allow us to build a condition
+          // in the form ':placeholder = :placeholder2', because the 'field'
+          // part of a condition is always escaped.
+          // As a (cheap) workaround, we separately build a condition with no
+          // field, and concatenate the field and the condition separately.
+          $value_part = db_and();
+          $value_part->condition('anonymous_name', $condition['value'], $condition['operator']);
+          $value_part->compile(Database::getConnection(), $query);
+          $or->condition(db_and()
+            ->where(str_replace('anonymous_name', ':anonymous_name', (string) $value_part), $value_part->arguments() + array(':anonymous_name' => user_format_name(user_load(0))))
+            ->condition('users.uid', 0)

This looks weird, why not just use where directly?

+++ b/core/modules/field/modules/entity_reference/lib/Drupal/entity_reference/Tests/EntityReferenceSelectionSortTest.phpundefined
@@ -0,0 +1,154 @@
+    // Create an Article node type.
+    if ($this->profile != 'standard') {
+      $this->drupalCreateContentType(array('type' => 'article', 'name' => 'Article'));

Do we still need the standard check? No other test that I know of does something like this and we default to testing now.

+++ b/core/modules/views/lib/Drupal/views/Plugin/entity_reference/selection/Views.phpundefined
@@ -0,0 +1,221 @@
+
+  /**
+   * @var \Drupal\views\ViewExecutable;
+   */
+  protected $view;

Should have a description.

+++ b/core/modules/views/lib/Drupal/views/Plugin/entity_reference/selection/Views.phpundefined
@@ -0,0 +1,221 @@
+   * Initializes a view.
+   *
+   * @param string $match
+   * @param string $match_operator
+   * @param int $limit
+   * @param array $ids
+   *
+   * @return bool
+   *   Return TRUE if the views was initialized, FALSE otherwise.

Missing descriptions.

yched’s picture

No time for an extensive review tonight, just :

class Node extends SelectionGeneric

Could we name those classes differently than the corresponding entity classes ? Like NodeSelection ?
Having another Node class sounds really weird, despite namespaces.
[edit: actually, it's not just that it's weird, it's against code standards : "Classes and interfaces should have names that stand alone to tell what they do without having to refer to the namespace" - in other words : this is not a node, it's a "node selection", or a "node reference selection"]

Other than that, I have voiced my concerns about the "single field type" approach, and although I haven't seen proper responses to the "forbids widgets or formatters specific to an entity type" issue (other than "Field API should be made more flexible", which is still a pipe-dream at this point), I can live with being overruled :-)

In order to commit, though, I think it might be a good thing to bring core maintainers over to this issue, to get their feeling about the existing field types this would duplicate / possibly-but-maybe-not deprecate (image, file, taxo term).

yched’s picture

Also, SelectionGeneric should probably be SelectionBase.

Berdir’s picture

+++ b/core/modules/field/modules/entity_reference/entity_reference.moduleundefined
@@ -0,0 +1,472 @@
+function entity_reference_field_info() {
+  $field_info['entity_reference'] = array(
+    'label' => t('Entity Reference'),
+    'description' => t('This field reference another entity.'),

I was thinking about the name of the field and the module this morning and I was wondering if we shouldn't call this just Reference. The name Entity Reference made sense 7.x to separate it from the References module, but there's no need for that in 8.x.

Just thinking out loud...

amitaibu’s picture

Great, thanks for the review, I'll work on addressing the comments and opening follow-up issues.

amitaibu’s picture

Issue summary: View changes

Updated issue summary.

amitaibu’s picture

I've updated the issue summary with the follow-up issues.

amitaibu’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
90.08 KB
132.96 KB

Addressed the above comments except the following which I have no idea:

+++ b/core/modules/field/modules/entity_reference/lib/Drupal/entity_reference/Plugin/entity_reference/selection/User.phpundefined
@@ -0,0 +1,85 @@
+          // Sadly, the Database layer doesn't allow us to build a condition
+          // in the form ':placeholder = :placeholder2', because the 'field'
+          // part of a condition is always escaped.
+          // As a (cheap) workaround, we separately build a condition with no
+          // field, and concatenate the field and the condition separately.
+          $value_part = db_and();
+          $value_part->condition('anonymous_name', $condition['value'], $condition['operator']);
+          $value_part->compile(Database::getConnection(), $query);
+          $or->condition(db_and()
+            ->where(str_replace('anonymous_name', ':anonymous_name', (string) $value_part), $value_part->arguments() + array(':anonymous_name' => user_format_name(user_load(0))))
+            ->condition('users.uid', 0)
I was wondering if we shouldn't call this just Reference.

I think Entity-Reference is more descriptive. I know I can reference a generic entity, and not let say a config.

and although I haven't seen proper responses to the "forbids widgets or formatters specific to an entity type" issue

Well, that was not done out of no respect to your comments -- It's that 1 DEC deadline that keeps reminding me we need to stick our foot in door :) . I've opened a follow up issue for this #1847590: Fix UX of entity reference field type selection, as it probably deserves it's own issue.

EDIT: The interdiff is big due to changing the file location, and replacing the word "generic" with "base"

Damien Tournoud’s picture

Status: Needs review » Reviewed & tested by the community

Let's get this in. EVERYTHING ELSE can be dealt with in follow-ups, please.

amateescu’s picture

I don't agree with the renaming of "generic" to "base" because this is a selection plugin implementation that's not really supposed to be used as a base from which other classes/implementations extend upon. Our own views selection plugin is an example, it doesn't need to extend the generic one. But nevermind, I'll let it rest for now becuase we really need to get this in :)

Fixed a file name that was forgotten and added an entry in MAINTAINERS.txt.

Status: Reviewed & tested by the community » Needs work
Issue tags: -Killer End-User Features, -Accessibility

The last submitted patch, entityreference-1801304-172.patch, failed testing.

swentel’s picture

Status: Needs work » Needs review
Issue tags: +Killer End-User Features, +Accessibility

#172: entityreference-1801304-172.patch queued for re-testing.

swentel’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC.

yched’s picture

@amateescu: the Base suffix doesn't mean all classes implementing the interface *have* to extend that base, it's just a hint.

+ * Contains Drupal\comment\Plugin\entity_reference\selection\Comment.

Needs to be adjusted to the new class name (also applies to the other selection classes)

yched’s picture

Status: Needs review » Reviewed & tested by the community

Crosspost, didn't intend to change the status.
Although a quick reroll for the docblocks above would be sweet :-)

amitaibu’s picture

Re-rolled according to #176

catch’s picture

I haven't reviewed the patch yet, I'll try to soon.

Once we have this there are more follow-ups that come to mind, the answer to some of these might be a very reasonable 'no', but it'd be good to explore:

- what about using this for hierarchical relationships between taxonomy terms?
- and menu links? (by extension book module)
- comment threading?
All of these have significant performance challenges, but that'd give us the potential for pluggable hierarchy storage to allow the thorniest of them to be fixed in contrib (like taxonomy_get_tree() and other old friends).
- what about using it for node and/or comment authors instead of the uid property?

I'd also be very keen to see at least one conversion of an existing reference field, whether taxonomy terms or files, soon after this gets in. Otherwise I don't think we can really justify adding the feature in isolation (same as entity translation needs to get rid of node translation).

catch’s picture

Issue summary: View changes

Updated issue summary.

amitaibu’s picture

Status: Reviewed & tested by the community » Needs review

> I'd also be very keen to see at least one conversion of an existing reference field, whether taxonomy terms or files

I agree. The first task is #1847596: Remove Taxonomy term reference field in favor of Entity reference, which is blocked by #1847924: Allow Entity-Reference field to autocreate items

amitaibu’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: -Killer End-User Features

According to PIFR the test passed.

amitaibu’s picture

Re-adding tag.

ParisLiakos’s picture

Status: Needs review » Reviewed & tested by the community

testbot having hard time with all the work we give it :P

chx’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +Killer End-User Features

I am fine with a followup but can we make sure please that the new entity API is implemented some time?

amitaibu’s picture

> please that the new entity API is implemented some time?

Yes, I've already started some work in #1847588: Implement the new entity field API for the Entity-Reference field type

amitaibu’s picture

I can't assign it to Dries, but according to catch, Dries need to give his sign-off for this.

webchick’s picture

Assigned: amitaibu » Dries

I'll assign it to Dries. I don't know that he needs to sign off on it per-se, but it is a pretty big (and AWESOME) change, so doesn't hurt.

I'm trying to get us below thresholds atm, but I can't WAIT to see this committed. :D

Dries’s picture

I'm on board with adding this feature. It's very useful and powerful, and could be a building block for more. But ...

I tested the patch and it seems to work well, except for some serious usability issues. Conceptually it makes sense to drop "taxonomy term references" in favor of this, but it would require this to be pretty useable for non-developers. For example, I don't think some of the sorting options are useable enough. It wasn't immediately obvious what "Filter by an entity reference view" meant. Same for "a property of the base table of the entity" or "a field attached to this entity". This language is very developer-y and requires a deep understanding of Drupal. We can't really drop taxonomy term references in favor of this, until we fix some of the usability issues. At a minimum, I'd like to see a usability review and some screenshots before this gets committed.

As for reviewing the code and its performance; I won't be able to review a 200k patch right now though. I'd be happy to have catch sign off on the code review if he has time.

amateescu’s picture

Assigned: Dries » catch

@Dries, thanks for taking the time to actually test the patch :) While I agree with the usability concerns, this is still marked as a feature request.. and time is running out fast.

Tentatively assigning to catch for code review.

catch’s picture

Assigned: catch » Dries

I'm not clear if Dries wants usability testing before this patch goes in, or before reworking taxonomy reference to use this. Assigning back for clarity.

moshe weitzman’s picture

Dries requested that the patch authors and UX Maintainers get together and improve the copy and the UX a bit more and then this can go in. Neither formal usability testing nor taxonomy reference conversion are a prerequisite.

webchick’s picture

Assigned: Dries » Unassigned
Status: Reviewed & tested by the community » Needs work
Issue tags: +Usability
FileSize
41.95 KB
33.34 KB
29.75 KB
23.88 KB
47.15 KB
53.39 KB

I can translate. The usability of this patch right now is pretty bad, unfortunately. Adding $entity reference fields is a site builder task, but the patch in its current state it involves a deep, *deep* understanding of both developer-facing terminology as well as Level 93 Views knowledge in order to use it for all but the trivial cases. These concerns severely compound if we're planning to get rid of targeted reference fields such as Taxonomy and File in favour of this field. Adding this kind of stuff is "table steaks" site builder stuff, which means people who are not programmers and have never seen a database before need to be able to use it.

So what Dries is asking for is basically a general usability review + some fixes to make it less painful.

Here's a walkthrough of the patch as it currently stands to help that along:

USE CASE: As a site builder, I want to add an "Author credit" field to my Article type, which can reference a user on my site.

  1. Apply the patch and enable the Entity Reference module.
  2. Go to Structure >> Content Types >> Article >> Managed Fields (admin/structure/types/manage/article/fields).
  3. Add new field "Author credit" , choose "Select list" (or whatever) as the widget. Click Save.
    Adding an entity reference field to the field UI.

    NOTE: for whatever reason, tabledrag is broken on this page for me but I've been informed this is something jacked with my local install somehow, so disregard (unless someone else can reproduce this).

  4. Next, you're directed to select the type of entity you'd like to reference on the Field Settings form (field label says "Target type", description is "The entity type that can be referenced through this field."):

    Field settings direct you to determine which entity to reference. You can select from many options such as Content, Taxonomy Term, and View.

    FEEDBACK:
    a) I find it weird that both "Category" and "Taxonomy term" are listed here. What's the difference?
    b) I also find it weird that things I'd consider "config entities" like Breakpoints and Views are showing up here, along with "content entities" such as Content and Users. This seems potentially very powerful, but Is that intended? Seems like eventually (hopefully) Block, Menu, and various other things are also showing here as well, leading to an overwhelming list pretty quickly.

    What if we used an <optgroup> to separate Content from Configuration entities? You have to be pretty far down the "advanced use case" road to have a reason to attach a Breakpoint reference, IMO, so that just becomes visual clutter standing in the way of doing what you actually want to do.

  5. Choose "User" and click Save.
  6. On the next screen, you choose your "Entity selection" method. Here's where things get whacky.

    Specify selection mode and sort by.

    Under "Mode" there are two options: "Simple Select" and "Views: Filter by an entity reference view." Let's cover Simple Select first, since the use case doesn't prescribe any particular fanciness that would require a view.

    "Sort by" defaults to "Don't sort" which is pretty much what you never want. So you click that to expand it and you get... a whole pile of developer-ese gobbeldygook:

    'A property of the base table of the entity' and 'A field attached to this entity'

    FEEDBACK:
    Um. WAT? :P Sorry, but this is totally whack. :\ It's not reasonable for a site builder to understand what any of these words mean and know what choice to make here. :\

    Can we instead just expose all of the possible things you can sort by, including "none"?

  7. "A property of the base table of this entity" will give you options like "uid", "pass", "created", "langcode", etc.

    Column names from the user table.

    "A field attached to this entity" will only give you "PIcture" since that's the only field on users.

    Column names from fields attached to users.

    FEEDBACK:
    Hm. Well I guess not, since that'd mean a list with both "uuid" and "Picture (column fid)" in the same list, which would be pretty awkward. Is there any general API that we could hook into around Views to get a standard "human readable" version of this info instead? Or is this only a temporary state, which will eventually resolve itself as as we convert things to the Entity Field/Property API..?

  8. Pick "property of the base table of the entity" and the "name" property and click Save.
  9. Yay, your field is made. Now go to Create content >> Article (node/add/article) and you'll see your lovely select list.

    The author credit field shows, and displays a list of users on the site, along with 'Anonymous'

    FEEDBACK:
    It's weird that "Anonymous" is showing up here as well. While there's probably a use case for that, seems like it'd be better to give me a checkbox to enable/disable that in the field settings.

Ok. Got all that? Now let's try an only slightly more advanced use case:

USE CASE: As a site builder, I want to add an "Author credit" field to my Article type, which can reference a user in the 'editor' role on my site.

Stay tuned... spoiler alert: it's not pretty. :\

webchick’s picture

Oops, I missed a screenshot.

yoroy’s picture

First very rough review based only on webchicks screens above in #195, the simple use case

2. A common problem with this kind of functionality in general is that you don't necessarily think of these things as fields so don't necessarily end up here to even find it. Does the module description in 'Extend' provide a pointer about this?

3. First hurdle is the 'Entity reference' label. Especially 'entity'. Good thing we got rid of 'node' :) Anyway, thingies are called entities and we're trying to reference them so lets go with this for now.

4. What webchick said. Alpha-sorting is not helping us here. Can we make this list shorter and/or group and sort it more intelligently? The label 'Target type' uses new words where it's probably better to keep using 'reference' and 'entity'. "Entity to reference"? "Reference this/these entity(ies)?

6a. Selection mode: Incomprehensible (again, only looking at the screenshots). I *think* defaulting to 'simple' and providing a checkbox or something to opt for 'Views: filter by…' would be better. I don't know what I'm choosing here, the system doesn't explain what the choice entails so lets find a way to either clarify that or UI-wise, postpone on having to choose between these at all.

6b. Sort by. What webchick said. How many options can we expect here? Can we pick a better default than the current one?

Simpler wording suggestions:

- Sort by a property of the base table of the entity > Sort by property of the referenced entity?
- A field attached to this entity > "This" confuses me. This entity I'm adding the reference to or the ones that are being referenced? I guess from the scope of things here it is the later. Suggestion: A field of the referenced entity
- Drop the redundant 'column' from the 'sort field' select list, the info will stay relevant to those who understand it, abstract to those who don't.

Overall, the workflow from 4 to 8 needs a closer look. I think we can sequence these steps in a better way that provides more guidance. Which goes beyond this first initial review. Massaging the actual wording and labelling will help a lot too.

Bojhan’s picture

Perhaps it makes sense to step by #drupal-usability and schedule in a moment, to go over it in depth and to figure out some solutions. We are often around, and we have office hours - next time please assign "Usability" tag earlier in the process as starting usability review upon RTBC is often hard.

@yoroy clearly laid out valuable improvements, it seems like we will need to structure these suggestions and explore more untill we can get to a solid plan as how to tackle the problematic UX parts.

webchick’s picture

Aaaand we're back. To refresh your memory:

USE CASE: As a site builder, I want to add an "Author credit" field to my Article type, which can reference a user in the 'editor' role on my site.

  1. Repeat steps 1-5 above to add your field (or edit the one from above). You should be looking at this:

    Entity selection drop-downs on the field settings form.

  2. Because there's no way to filter the list of users by role in a "Simple select" type, we now need to jump face-first into Über Advanced Land, which means selecting "Views: Filter by an entityreference view." The first thing you'll be told is that you have no entityreference views, and you need to create one:

    views-none-found.png

    FEEDBACK:
    One really positive thing I will say is that throughout, already knowing Views, I was actually to complete this task thanks to the very precise wording in these and other various field descriptions. So bravo for attention to detail and clear use of language here.

    Unfortunately, the vast majority of people wanting to build a website out there have not used Views, and if they have used Views they most certainly probably have not messed a lot with Displays, since the wizard takes care of that. :( My first question would be, "Can we just expose the most common things people are likely going to want to filter by (e.g. Users by role, Nodes by type, etc.) on this form right here, without requiring people to jump all the way to Views to do them? I can totally see going to Views for "I want my author credit field to reference users with a certain number of user points and who have created between 1 and 5 articles with the word 'Snoopy' in them" but not for something so basic.

  3. Click "Create a view" here. But before doing that, I recommend writing down "Entity Reference display" on a napkin, or else having a really good memory, because the next screen doesn't say anything about it:

    Standard Views wizard screen.

    FEEDBACK:
    Would it be possible to customize this wizard for the task at hand? For example, adding an auto-checked fieldset for Entity Reference display so that part is already done at the time I'm specifying filters and stuff like that?

    FOLLOW-UP: Those fieldsets should not say "Details" :P Not the fault of this patch.
    FOLLOW-UP: Not sure if "Save and exit" makes sense as the primary button here. Not the fault of this patch.
    FOLLOW-UP: It was surprising that "Role" isn't one of the exposed filters on the wizard for "User." I would've thought that was pretty common.

  4. View name: editors, Show: Users, click "Continue & Edit."
  5. Now on the "Advanced" view screen, you get to do the following elaborate dance...
  6. Click "+ Add" at the top (FOLLOW-UP: The button should likely be "Add display"? Not part of this patch.) and add an "EntityReference" display:

    Add an EntityReference display to the view.

  7. All appears fine, but if you scroll down to the preview section, you will see:

    EntityReference needs a selected search field to work properly.

    FEEDBACK:
    In a similar way that the Views wizard defaults to having User: Name field already populated, can the EntityReference display default to whatever the "primary field" is of the given view type?

    FOLLOW-UP: Had the error been at the top of the page, I could've reacted faster. Not the fault of this patch. (I don't think.)

  8. Despite being scary, the message is descriptive, so click on "Settings" next to Entity Reference list

    Settings link outlined on the views admin page.

    FEEDBACK:
    Maybe just link right there from the error message directly?

  9. Click it and get a modal, with a required field with one option and nothing checked. ;) Tee hee. Check it.

    Prompt to check for 'Search fields'

    FEEDBACK:
    (minor) This is not an autocomplete widget; it's a select list. So the description is incorrect.

    I don't really understand what "Search field" means. It seems to think I know more about how this is implemented under the hood than I currently do. Can you explain this, and we can try and come up with a better word?

  10. Yay, now the preview shows stuff:
    Showing full list of users, minus Anonymous.

    FEEDBACK
    Ironically, this is showing it to me without anonymous, which is what I would expect. :D Go, Views!

  11. Add a filter criteria for user role (I won't screenshot this, you've all done it before. But worth bearing in mind that it's like 3 more steps).
  12. Also add a sort criteria since you most likely don't want the list sorted by newest first. Another 3 steps. Then you want to remove the one you don't want. Another 2 steps.

    FEEDBACK
    Related to the "wizard" question above, can we default this to "Name" instead of create date (desc)?

  13. Cool, preview updated to show only editors, in alphabetical order:
  14. Preview now showing the proper thing.

  15. Save the view. Hooray!!!!
  16. Now, simply go to node/add/article and... hey wait. It's still looking like this. What gives? :(

    Node form showing whole list of users.

    ...because you got distracted with 500 steps and forgot that you were actually configuring field settings. :P Oops. :P

  17. Go back to admin/structure/types/manage/article/fields/field_author_credit, select the view, save. NOW you're done.

    FEEDBACK
    Is it possible for Entity Reference to just ship with some sensible default content entity views that would show up in step #2 that basically replicate what Simple select does? I still wouldn't have exactly what I wanted most likely, but it would probably eliminate a TON of these steps.

Whew! Ok. So now we need to figure out what of this is commit-blocker and what of this is follow-up. But we definitely can't commit this as-is, sadly. :(

effulgentsia’s picture

#199 means that ER could be a great initial use case for making ViewAddFormController::form() / Views Wizard plugins properly horizontally extensible. Currently, you can have a wizard per base table, but being able to say, "I'm launching this form from ER field settings configuration, so make it appropriate for that, by for example, automatically creating an ER display" would help address some of the issues webchick identified. And that would let us both make ER usable, and help make sure D8 contrib modules could also integrate fast Views creation workflows tailored to their needs. I'm totally ok with that being all done post-commit of initial patch, just pointing out an opportunity, and making sure our UX thinking isn't too constrained by what the current "Add View" form allows / doesn't allow.

amitaibu’s picture

Had a conversation on IRC with webchick, in short:

  1. Give a checkbox for the user, to filter by role -- that's a typical use case, and we don't need Views for this
  2. Provide an example Views configuration, for example "Content by user", so more advanced users can see the ER display style in use
  3. Hide the config entity from the UI. We can still add them programmatically
  4. In the sort, have properties + fields together, like View does. We can use field API to get the description
nevergone’s picture

"Hide the config entity from the UI. We can still add them programmatically."

This could be made ​​conditional on some variables (or field property)? Given situation it may be necessary to make all entities visible.

Berdir’s picture

I assume that config entities do not work right now anyway? their id is a machine name, that's going to be hard to save in a target_id integer column :) Unless I'm missing something...?

And trying it out, you don't even get that far, it dies with a "LogicException: Querying configuration entities is not supported" after selecting e.g. Views.

So I would even say that hiding them is actually our only choice right now + an issue to figure out if and how we can support them. Maybe that needs #1803064: Horizontal extensibility of Fields: introduce the concept of behavior plugins so that we can build the correct field schema for each entity type? And @chx's query with grep issue for the qerying part ;)

catch’s picture

On listing config entities, I opened #1853856: Document that ConfigEntityBase and ConfigStorageController are tightly coupled. the fact that these two things are lumped into the same discovery/entity info with no real differentiation is a bug in itself, and the UX issue in this patch is just a symptom of that.

I haven't dived into this patch yet, but on using views for the selection, while it's very flexible I can see people wanting to provide custom listings without views (i.e. via raw EntityFieldQuery), how simple is it to extend it that way?

xjm’s picture

FooClass instanceof ContentEntityInterface was supposed to be the way to determine the distinction, except for users being weird and extending Entity directly right now.

amateescu’s picture

Status: Needs work » Needs review
FileSize
12.22 KB
134.69 KB

Here's a small update which fixes 1), 3) and 4) from #201 as I think 2) deserves more discussion in a followup issue.

I also contacted Bojhan on IRC to talk a bit about how can we improve the initial patch, let's see how it goes :)

Re: @catch in #204

I haven't dived into this patch yet, but on using views for the selection, while it's very flexible I can see people wanting to provide custom listings without views (i.e. via raw EntityFieldQuery), how simple is it to extend it that way?

It's very easy actually, that's the purpose of the Selection plugin type. You can create a new plugin that extends SelectionBase (which is EFQ based) and go crazy with it :)

Edit: the +// '#multiple' => TRUE, leftover has already been removed in the latest sandbox commit. Not rolling a new patch just for that.

Status: Needs review » Needs work

The last submitted patch, entityreference-1801304-206.patch, failed testing.

amateescu’s picture

Status: Needs work » Needs review
FileSize
134.73 KB

Just unbreaking some stuff from the previous patch..

amitaibu’s picture

@amateescu,
Why not allow multiple selection on the user-roles?

amateescu’s picture

We do allow that :) It was #type => 'select' with '#multiple' => TRUE but then I converted the type to checkboxes and forgot to remove #multiple.

Bojhan’s picture

Alright, my review + suggestions! I focused primarily on Field UI as @webchick covered Views UI quite well (maybe we should split fixing that off? seems quite large). I took about 45 minutes with amateescu earlier today to go over this, we walked through the difficult parts and had some discussion about possible solutions. This was with the latest patch(es) applied which resolves a number of the issues listed earlier.

Fundamentals

  • It is not discover able that you will need "Entity reference" to be able to select Content, Users, Files, Terms etc. I'd much rather have "User reference, Content reference, File reference" as options and simply keep the config options under a Config reference with a dropdown. I think the discover ability you need such an abstract term to get a field with users, is a big issue. I'm sure there are some name space conflicts, but core should use the most sensible label.
  • Why are we introducing the word "Entity" here, although I am sure it has useful qualities, as @yoroy already mentions its another abstraction label people won't get and in this case its not really necessary. We can just use the word "Reference" which makes it much less scarier to select

User/Content/File
There are a couple of issues with this UI, primary some of the functionality grouping is really necessary.

  • Simple selection is not a meaningful label, given that this is simply the default method. Why don't we just call it "default" ?
  • Lets remove all notions of "Don't sort, Don't filter" and replace them with - None -

I was quite puzzled to find Sort by and Sort field seperated, although I am sure this might scale better with hunderds of different categories. It is essentially just one list, where - None - by default, is the only nonfield. Given that there is no feature loss, and it reduces complexity significantly (e.g. I have no idea what "a field of the referenced entity" means, but do know what sort by "name" means). We should move this to just one select list, with -None - on top - this also brings it closer in line with the Views interaction which is one long list of sort by options with human readable names.

Furthermore onto labeling

  • Let's call this section "Selection" or "Reference"
  • Mode, does not give a lot of clue what this is about. Perhaps "Source" or "Method" with a little description
  • Target bundles, sounds like a really abstract label when we know what the contents are. For Content, its 99% going to be "Content types" and for terms its 99% going to be "Vocabularies" lets default core with sensible labels where we are aware of the contents. This is also in line with Views.
  • We should not use the pattern of "select none, to get all". That pattern has proven time and time again, to be hard to understand by users. Instead we should do something similar to Views, where selecting a "All *" toggles all, and when you remove a toggle from a specific vocab/content type the "All *" removes its toggle.
  • We found that, the labels should be human readable names - amateescu seemed to already be working on that.

Just an idea how some of those suggestions might end up looking:

clean-up-selection-method.png

I moved it below the "Article settings" - I am totally handwaving Field UI here, but it is really sucky to have fieldsets inside fieldsets like this. I am also not very fond of all the indenting, if we merge the two fields at the very least there is less indenting going on.

There goes my initial review :) Hope this is enough to go from.

Bojhan’s picture

Bojhan’s picture

Status: Needs review » Needs work

:D It seems just before feature freeze rush is over, hopefully this will see work before feb :) There is quite a lot.

amitaibu’s picture

catch’s picture

Assigned: Unassigned » catch

Assigning to me so I can do a pass over the API-level stuff while the usability fixes are being worked on (this shouldn't stop anyone else doing the same).

amitaibu’s picture

Status: Needs work » Needs review
FileSize
141.74 KB
18 KB

First step in deprecating taxonomy-term is adding "auto-create" feature.
Patch adds "auto-create" option in the UI - with tests.

Note that currently referencing taxonomy terms is broken, from the code:

// @todo: Taxonomy term's bundle key is vocabulary_machine_name, but
// entity_query() fails with it, so for now hardcode the vid, until
// http://drupal.org/node/1552396 is fixed.
$bundle_key = $target_type != 'taxonomy_term' ? $entity_info['entity_keys']['bundle'] : 'vid';
amitaibu’s picture

Status: Needs review » Needs work
Issue tags: -Needs usability review, -Needs screenshots

I've merged #1801304: Add Entity reference field into ER's main branch (you can see the diffs in that issue).

Next is go back to #1847596: Remove Taxonomy term reference field in favor of Entity reference. Once that issue is in, I really hope we can get ER to core, as this patch is becoming huge.

webchick’s picture

Wait. How can #1847596 get in without this issue? I'm confused.

Are you ready for a new UX review?

amitaibu’s picture

> Wait. How can #1847596 get in without this issue? I'm confused.

No, what I meant is the code from #1847596: Remove Taxonomy term reference field in favor of Entity reference was merged into the main ER code that lives in yched/entityreference-1801304

> Are you ready for a new UX review?

I'd prefer to first finish #1847596: Remove Taxonomy term reference field in favor of Entity reference

amateescu’s picture

There is still some work needed here before another UX review, #201 has been addressed but we still need to do Bojhan's feedback from #211. I'm on vacation and mostly away for another week but I'll try to see if I can steal some time for this :)

amitaibu’s picture

FileSize
2.81 KB

Vocabularies are now configuration entities, so we need to update the entityQueryAlter() method -- but not sure how?

webchick’s picture

Priority: Major » Critical

I still think this would be a killer feature to have in D8. Escalating to a critical feature request.

amitaibu’s picture

> I still think this would be a killer feature to have in D8

I agree :)
Work is now done in #1847596: Remove Taxonomy term reference field in favor of Entity reference. Help there is appreciated, cause it's going over and fixing lots of tests...

amitaibu’s picture

I could use some help in #1847596: Remove Taxonomy term reference field in favor of Entity reference, deprecating field_tags, touches so many tests, and some of them are not just a simple find & replace.

amitaibu’s picture

@catch,
Talked with Berdir, and I'd like to confirm. Is what holding this issue the deprecation of taxonomy term or just fixing the UI?
#1847596: Remove Taxonomy term reference field in favor of Entity reference is getting closer, but it's a big patch, so maybe treat it as a follow-up?

amitaibu’s picture

Status: Needs work » Needs review
FileSize
23.43 KB
181.26 KB

I've merged some of the fixes into the main branch, let's see if testbot is still happy.

amateescu’s picture

Here's an updated patch and screenshot that addresses Bojhan's usability feedback from #211.

About the Fundamentals part:

1) this was discussed at length from #102 to #124 and from what I remember the agreement was to continue with the current implementation for now. If you feel strongly that having *a lot* of 'references' vs. only one option in the Field UI is better for new users, maybe we should open a follow-up to discuss this?

2) I think I agree that renaming to Reference makes sense, but this is quite a big change if we want to want to go the full way (renaming everything in code, not just a few UI labels) so maybe we could gather more feedback about this

amateescu’s picture

FileSize
892.02 KB
180.3 KB

Correct patch and screenshot this time.. :/

er-230.png

joshmiller’s picture

The follow up amateescu proposed above exists and was part of the issue summary ... #1847590: Fix UX of entity reference field type selection

amateescu’s picture

Assigned: catch » Bojhan

Re-assigning to Bojhan at his request.

Bojhan’s picture

Assigned: Bojhan » Unassigned
FileSize
45.81 KB
58.14 KB

A whole lot less to review :) This is going in the right direction, a small review;

Perhaps we should relabel this and move the type to the top. I still think just calling this Reference will make it a lot more easier to grasp, and allows for all kinds of things to implement the idea of being a Referencable type.

We should seek ways to make these labels more sensible, on initial sight I always miss this fieldset because it doesn't stand out being about the reference option.

moshe weitzman’s picture

IMO, it would be fine to change just the UI labels to 'Reference' and leave the code as is, with the more explicit 'entity reference'.

eaton’s picture

Seconding "Reference Type" and "Reference Method".

I'd lean towards "Create referenced entities if they don't already exist" as opposed to "Auto-create." More verbose, but.

pounard’s picture

Target type sounds good to me, we are not talking about the "reference" type but the "referenced object" type instead.

amateescu’s picture

Assigned: Unassigned » catch
Issue tags: -Needs usability review

Implemented the feedback from #233.2 and #235.

#233.1 is a limitation of Field UI, we cannot (easily) move the target type select box above the values :( Also left the 'Target type' text intact because @pounard is right in #236.

Moving back on catch's radar :)

amateescu’s picture

Status: Needs review » Needs work

The last submitted patch, entityreference-1801304-237.patch, failed testing.

amitaibu’s picture

Test are failing as #1778178: Convert comments to the new Entity Field API got in. It's becoming really hard to chase head :/

amitaibu’s picture

Assigned: catch » amitaibu

@amateescu, I'll be working on fixing the tests.

amitaibu’s picture

I'm thinking we should change the key from target_id to value. Apart of the whole find replace we need to do to adapt the tests, and different code to deal with finding the right key, I think it's be a better UX.
There's no reason a developer should do $comment->nid->target_id. With $comment->nid->value the developer doesn't need to know about the internal implementation.

amitaibu’s picture

Status: Needs work » Needs review
FileSize
164.82 KB
39.26 KB

Let's see if at least testbot likes my idea from #242 :)

amitaibu’s picture

btw, the target_id key was introduced in this patch as legacy from D7. The current implementation in D8 is with value

Status: Needs review » Needs work

The last submitted patch, entityreference-1801304-243.patch, failed testing.

amitaibu’s picture

amateescu is against the target_id => value change, saying

amateescu: amitaibu: so yeah, $comment->nid->value needs to be smart enough to return 'target_id' instead of 'value'

Anyway, it's back to fixing the tests...

amitaibu’s picture

Status: Needs work » Needs review
FileSize
207.01 KB
23.66 KB

Incremental progress, out of interest lets see how many more fails left.

amitaibu’s picture

Missed a few more ->value to ->target_id replaces.

Status: Needs review » Needs work

The last submitted patch, entityreference-1801304-248.patch, failed testing.

amitaibu’s picture

Status: Needs work » Needs review
FileSize
209.07 KB
8.87 KB

We should make sure to follow this pattern instead of hardcoding ->value in foreach() when dealing with EntityNG. For example

foreach ($this->entityInfo['schema_fields_sql']['data_table'] as $name) {
  $key_name = key($entity->$name->offsetGet(0)->getProperties());
  $record->$name = $entity->$name->{$key_name};
 }

Not sure if my fixes covered all the failing tests, but got to run, so here's another patch :)

EDIT: I've improved the above code to have more context

Status: Needs review » Needs work

The last submitted patch, entityreference-1801304-250.patch, failed testing.

amitaibu’s picture

Status: Needs work » Needs review
FileSize
214.9 KB
5.83 KB

Oops, changes #248 were somehow ignored. Re-rolled.

amitaibu’s picture

OMG, it's green again! :)

plach’s picture

Issue summary: View changes

Updated issue summary.

aspilicious’s picture

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Plugin/entity_reference/selection/TermSelection.phpundefined
@@ -0,0 +1,61 @@
+    // @todo: How to set access, as vocabulary is now config?

Shouldn't we answer this question or open a followup for it?
Anyway I would love this to get in :)

amitaibu’s picture

> Shouldn't we answer this question or open a followup for it?

Yes, it is in the follow-up issues list #1891558: Improve TermSelection access control :)

stevector’s picture

Assigned: amitaibu » Unassigned

Looks like this should go back to catch then :)

amateescu’s picture

Assigned: Unassigned » amateescu
Status: Needs review » Needs work

We should make sure to follow this pattern instead of hardcoding ->value in foreach() when dealing with EntityNG. For example

foreach ($this->entityInfo['schema_fields_sql']['data_table'] as $name) {
  $key_name = key($entity->$name->offsetGet(0)->getProperties());
  $record->$name = $entity->$name->{$key_name};
}

I don't agree with this, it's a true performance killer.. think of how many function calls it adds for each field of each queried entity. I'll try to come up with something else tonight :)

amateescu’s picture

Assigned: amateescu » catch
Status: Needs work » Needs review
FileSize
212.81 KB
3.05 KB

I think this is as far as we can go without solving the problems from DatabaseStorageControllerNG in this patch.

  /**
   * Overrides \Drupal\Core\Entity\Field\FieldItemBase::get().
   */
  public function get($property_name) {
    $property_name = ($property_name == 'value') ? 'target_id' : $property_name;
    return parent::get($property_name);
  }
amateescu’s picture

Bah, failures were due to #611294: Refine permissions for Field UI. Fixed in the attached patch.

fago’s picture

I don't agree with this, it's a true performance killer.. think of how many function calls it adds for each field of each queried entity. I'll try to come up with something else tonight :)

This will run on saving only, thus I don't see a big problem with it. But maybe that would be nicer?

foreach ($this->entityInfo['schema_fields_sql']['data_table'] as $name) {
  // Just assign the first field item value.
  foreach ($entity->{$name}[0] as $property) {
    $record->$name = $property->getValue();
    break;
  }
}
sun’s picture

Awesome work so far! Looks like we're really close to get this done! :)

I only found a few smaller things. I don't think anything of this should hold up this patch, but if we can fix them before commit, even better. :)

+++ b/core/modules/entity_reference/entity_reference.module
@@ -0,0 +1,543 @@
+function entity_reference_field_is_empty($item, $field) {
+  if (!empty($item['target_id']) && $item['target_id'] == 'auto_create') {
+    // Allow auto-create entities.
+    return FALSE;
+  }
+  return !isset($item['target_id']) || !is_numeric($item['target_id']);
+}
...
+function entity_reference_field_validate(EntityInterface $entity = NULL, $field, $instance, $langcode, $items, &$errors) {
...
+    if (!entity_reference_field_is_empty($item, $field) && $item['target_id'] !== 'auto_create') {

The auto_create condition in combination with the field_is_empty() condition is quite mind-boggling here ;)

Would it make sense to move the 'auto_create' condition first?

Not sure whether it will actually clarify the combined condition, but at minimum, it'll be faster ;)

+++ b/core/modules/entity_reference/entity_reference.module
@@ -0,0 +1,543 @@
+/**
+ * Implements hook_field_presave().
+ *
+ * Create an entity on the fly.
+ */
+function entity_reference_field_presave(EntityInterface $entity, $field, $instance, $langcode, &$items) {
+  global $user;
...
+  foreach ($items as $delta => $item) {
+    if ($item['target_id'] == 'auto_create') {
...
+      $values = array(
...
+        // @todo: Use wrapper to get the user if exists or needed.
+        'uid' => !empty($entity->uid) ? $entity->uid : $user->uid,
+      );
+      $target_entity = entity_create($target_type, $values);
+      $target_entity->save();

Is there any particular reason for why we're excluding anonymous users for 'uid' here?

Or is it possible that the intention was to use isset() instead of !empty()?

+++ b/core/modules/entity_reference/entity_reference.module
@@ -0,0 +1,543 @@
+/**
+ * Implements hook_field_instance_settings_form().
+ *
+ * The field settings infrastructure is not AJAX enabled by default,
+ * because it doesn't pass over the $form_state.
+ * Build the whole form into a #process in which we actually have access
+ * to the form state.
+ */
+function entity_reference_field_instance_settings_form($field, $instance) {

That's some massive, ugly #process callback there...

The @todo the phpDoc is talking about looks like a one-line change in field_ui.admin.inc to me? So why don't we simply adjust it? :)

-  $additions = module_invoke($field['module'], 'field_instance_settings_form', $field, $instance);
+  $additions = module_invoke($field['module'], 'field_instance_settings_form', $field, $instance, $form_state);

(Note: I've left out $form, since fields/instances/plugins shouldn't depend nor be aware of the full $form structure.)

+++ b/core/modules/entity_reference/lib/Drupal/entity_reference/Plugin/Type/Selection/SelectionBroken.php
@@ -0,0 +1,65 @@
+/**
+ * A null implementation of SelectionInterface.
+ */
+class SelectionBroken implements SelectionInterface {
...
+  public static function settingsForm(&$field, &$instance) {
+    $form['selection_handler'] = array(
+      '#markup' => t('The selected selection handler is broken.'),
+    );
+    return $form;
+  }

1) Would UnknownSelection be a better name?

2) The form outputs a user-facing message that doesn't actually seem to be targeted at users? I wonder whether we should rather throw an exception somewhere?

+++ b/core/modules/entity_reference/lib/Drupal/entity_reference/Plugin/Type/Selection/SelectionInterface.php
@@ -0,0 +1,81 @@
+  /**
+   * Counts entities that are referencable by a given field.
+   */
+  public function countReferencableEntities($match = NULL, $match_operator = 'CONTAINS');
+
+  /**
+   * Validates that entities can be referenced by this field.
+   *
+   * @return array
+   *   An array of valid entity IDs.
+   */
+  public function validateReferencableEntities(array $ids);

It's unclear what implementations of these methods are supposed to do...

I guess the first probably expects an integer count as @return?

And the second is supposed to throw exceptions on validation errors? Or just return valid IDs and silently ignore invalid ones?

+++ b/core/modules/entity_reference/lib/Drupal/entity_reference/Plugin/Type/SelectionPluginManager.php
@@ -0,0 +1,84 @@
+    $this->discovery = new CacheDecorator($this->baseDiscovery, 'entity_reference_selection');

Unless I'm mistaken, CacheDecorator is now supposed to get a list of cache tags as last argument now. (I could be mistaken though.)

+++ b/core/modules/entity_reference/lib/Drupal/entity_reference/Plugin/Type/SelectionPluginManager.php
@@ -0,0 +1,84 @@
+  public function createInstance($plugin_id, array $configuration = array()) {
+    // We want to provide a broken handler class whenever a class is not found.
+    try {
+      return parent::createInstance($plugin_id, $configuration);
+    }
+    catch (PluginException $e) {
+      return new SelectionBroken($configuration['field'], $configuration['instance']);
+    }
+  }

It's not really clear to me under which circumstances the selection handler plugin could vanish?

+++ b/core/modules/entity_reference/lib/Drupal/entity_reference/Plugin/entity_reference/selection/SelectionBase.php
@@ -0,0 +1,332 @@
+  /**
+   * Implements SelectionInterface::validateReferencableEntities().
+   */
+  public function validateReferencableEntities(array $ids) {
+    $result = array();
+    if ($ids) {
+      $target_type = $this->field['settings']['target_type'];
+      $entity_info = entity_get_info($target_type);
+      $query = $this->buildEntityQuery();
+      $result = $query
+        ->condition($entity_info['entity_keys']['id'], $ids, 'IN')
+        ->execute();
+    }
+
+    return $result;
+  }

Now that I see an implementation, I wonder whether "validate" is the appropriate terminology for this method...? Looks more like a "filter"?

Also, any chance to move the !empty($ids) condition into the calling API code? Looks like needless duplication in all implementations?

+++ b/core/modules/entity_reference/lib/Drupal/entity_reference/Plugin/entity_reference/selection/SelectionBase.php
@@ -0,0 +1,332 @@
+  public function validateAutocompleteInput($input, &$element, &$form_state, $form, $strict = TRUE) {
+    $entities = $this->getReferencableEntities($input, '=', 6);
...
+    if (empty($entities)) {
+      if ($strict) {
+        // Error if there are no entities available for a required field.
+        form_error($element, t('There are no entities matching "%value".', $params));
+      }
+    }

Renaming the $strict parameter to $required might help to clarify its purpose?

Alternatively, isn't #required available on the passed-in $element already?

+++ b/core/modules/entity_reference/lib/Drupal/entity_reference/Plugin/entity_reference/selection/SelectionBase.php
@@ -0,0 +1,332 @@
+      // @todo: Taxonomy term's bundle key is vocabulary_machine_name, but
+      // entity_query() fails with it, so for now hardcode the vid, until
+      // http://drupal.org/node/1552396 is fixed.
+      $bundle_key = $target_type != 'taxonomy_term' ? $entity_info['entity_keys']['bundle'] : 'vid';

That's fixed now. :)

+++ b/core/modules/entity_reference/lib/Drupal/entity_reference/Plugin/field/widget/AutocompleteWidgetBase.php
@@ -0,0 +1,132 @@
+  public function settingsForm(array $form, array &$form_state) {
+    $element['match_operator'] = array(
+      '#type' => 'select',
+      '#title' => t('Autocomplete matching'),
+      '#default_value' => $this->getSetting('match_operator'),
+      '#options' => array(
+        'STARTS_WITH' => t('Starts with'),
+        'CONTAINS' => t('Contains'),
+      ),

Is it proper to use a select widget here, if there are just two options? Did we consider to use radios?

+++ b/core/modules/entity_reference/lib/Drupal/entity_reference/Plugin/field/widget/AutocompleteWidgetBase.php
@@ -0,0 +1,132 @@
+      '#description' => t('Select the method used to collect autocomplete suggestions. Note that <em>Contains</em> can cause performance issues on sites with thousands of nodes.'),

Stale "nodes" ?

+++ b/core/modules/entity_reference/lib/Drupal/entity_reference/Plugin/field/widget/AutocompleteWidgetBase.php
@@ -0,0 +1,132 @@
+    $element['size'] = array(
+      '#type' => 'textfield',
+      '#title' => t('Size of textfield'),
...
+      '#element_validate' => array('form_validate_number'),

I guess this should be a #type 'number' now?

+++ b/core/modules/entity_reference/lib/Drupal/entity_reference/Plugin/field/widget/AutocompleteWidgetBase.php
@@ -0,0 +1,132 @@
+  public function formElement(array $items, $delta, array $element, $langcode, array &$form, array &$form_state) {
+    $element = $this->prepareElement($items, $delta, $element, $langcode, $form, $form_state, 'entity_reference/autocomplete/single');
...
+  protected function prepareElement(array $items, $delta, array $element, $langcode, array &$form, array &$form_state) {

It looks like this code originally passed an additional argument forward to prepareElement(), but that argument is no longer taken, so we can merge the code back into formElement()?

+++ b/core/modules/entity_reference/lib/Drupal/entity_reference/Plugin/views/style/EntityReference.php
@@ -0,0 +1,120 @@
+  /**
+   * Overrides Drupal\views\Plugin\views\display\PathPluginBase::preview().
+   */
+  public function preview() {
+    if (!empty($this->view->live_preview)) {
+      return '<pre>' . check_plain($this->view->render()) . '</pre>';
+    }
+
+    return $this->view->render();
+  }

I'm not too familiar with Views... but this PRE output looks like stale debugging code?

+++ b/core/modules/entity_reference/lib/Drupal/entity_reference/Tests/EntityReferenceAutoCreateTest.php
@@ -0,0 +1,111 @@
+class EntityReferenceAutoCreateTest extends WebTestBase {

+++ b/core/modules/entity_reference/lib/Drupal/entity_reference/Tests/EntityReferenceAutocompleteTest.php
@@ -0,0 +1,114 @@
+class EntityReferenceAutocompleteTest extends TaxonomyTestBase {

+++ b/core/modules/entity_reference/lib/Drupal/entity_reference/Tests/EntityReferenceItemTest.php
@@ -0,0 +1,103 @@
+class EntityReferenceItemTest extends WebTestBase {

+++ b/core/modules/entity_reference/lib/Drupal/entity_reference/Tests/EntityReferenceSelectionAccessTest.php
@@ -0,0 +1,495 @@
+class EntityReferenceSelectionAccessTest extends WebTestBase {

+++ b/core/modules/entity_reference/lib/Drupal/entity_reference/Tests/EntityReferenceSelectionSortTest.php
@@ -0,0 +1,150 @@
+class EntityReferenceSelectionSortTest extends WebTestBase {

Would be great to leverage DrupalUnitTestBase for almost all of the new tests (they don't seem to use or need the internal browser)... but we can do that in a follow-up issue.

+++ b/core/modules/file/lib/Drupal/file/Plugin/entity_reference/selection/FileSelection.php
@@ -0,0 +1,36 @@
+ * Provides specific access control for the file entity type.
+ *
+ * @Plugin(
+ *   id = "base_file",
+ *   module = "entity_reference",
+ *   label = @Translation("File selection"),
+ *   entity_types = {"file"},
+ *   group = "base",
...
+class FileSelection extends SelectionBase {

+++ b/core/modules/node/lib/Drupal/node/Plugin/entity_reference/selection/NodeSelection.php
@@ -0,0 +1,44 @@
+ * Provides specific access control for the node entity type.
+ *
+ * @Plugin(
+ *   id = "base_node",
+ *   module = "entity_reference",
+ *   label = @Translation("Node selection"),
+ *   entity_types = {"node"},
+ *   group = "base",
...
+class NodeSelection extends SelectionBase {

Hm. Multiple issues here:

1) Why are these plugin IDs prefixed with "base_"? Did you mean to use a '_default' suffix; e.g., "file_default"?

2) Why is the module = entity_reference, if the plugin is provided by file/node/etc module?

3) What's that "group" property and where does it get exposed?

+++ b/core/modules/views/lib/Drupal/views/ViewsException.php
@@ -0,0 +1,15 @@
+/**
+ * Defines an exception thrown when Views operations fail.
+ */
+class ViewsException extends Exception { }

Hm... That looks (a bit too) generic to me.

Something like that should probably be the base exception class for all exceptions being thrown in Views...

In any case, I'd think this should extend \RuntimeException?

amateescu’s picture

Assigned: catch » amateescu

Thanks sun for a great review! I think all these should be handled before commit `cause I'm tired of seeing followups everywhere :)

amateescu’s picture

Assigned: amateescu » catch
FileSize
212.13 KB
25.52 KB

I ran out of time for responding properly to every point from #246, so I'll just post the new patch and interdiff and leave the answers for the weekend :)

Status: Needs review » Needs work
Issue tags: -Usability, -Killer End-User Features, -Accessibility

The last submitted patch, entityreference-1801304-266.patch, failed testing.

amateescu’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work
Issue tags: +Usability, +Killer End-User Features, +Accessibility

The last submitted patch, entityreference-1801304-266.patch, failed testing.

amitaibu’s picture

Status: Needs work » Needs review
FileSize
212.16 KB
936 bytes

Re-exported Views, as Views now requires ID instead of Name

amitaibu’s picture

btw, is it just on my local, or did auto-complete stop working in fields (e.g. taxonomy-term field)?

swentel’s picture

@Amitaibu, I've seen that as well. Author selection on node/add doesn't seem to work either, you will probably also get 'Theme hook hidden not found' messages in watchdog. I've posted a screenshot over at http://drupal.org/node/1886186 which is follow up on #1812724: Consolidate all form element templates and add theme_hook_suggestons which *probably* triggered this, although I'm not sure.

- edit - Core issue here #1893400: Autocomplete is busted with a patch

amitaibu’s picture

I think the failing test in #270 (Drupal\menu\Tests\MenuTest) isn't related to this issue, as it fails on my local also on the main branch 8.x

sun’s picture

Status: Needs review » Reviewed & tested by the community

Thanks! Let's go ahead here :)

webchick’s picture

Overall, LOTS of UX improvements this time. Thanks for that. However, did anyone give this a final run-through? It seems like not, because I'm hitting quite a few problems trying. Might just need a re-roll, not sure, but...

1) I'm getting this on the configure field form after adding the field:

Notice: Undefined index: bundles in entity_reference_options_list() (line 374 of core/modules/entity_reference/entity_reference.module).

2) When I attempt to add a user role filter, I get:

An AJAX HTTP error occurred.
HTTP Result Code: 200
Debugging information follows.
Path: /system/ajax
StatusText: OK
ResponseText: Recoverable fatal error: Object of class Drupal\user\Plugin\Core\Entity\Role could not be converted to string in filter_xss() (line 1329 of /Users/webchick/Sites/8.x/core/includes/common.inc).

3) I added an entity reference view/display to my field, and when attempting to load the front page I get:

Fatal error: Wrong parameters for Exception([string $exception [, long $code [, Exception $previous = NULL]]]) in /Users/webchick/Sites/8.x/core/modules/views/lib/Drupal/views/Plugin/entity_reference/selection/ViewsSelection.php on line 128

If this isn't just from HEAD changes in the past couple of days, we're going to need more test coverage here. I'm going to send it for a re-test to see.

Then just as a side-note to fatal errors and whatnot. When you create a view with an entityreference display, you get this error:

"Display "EntityReference" needs a selected search fields to work properly. See the settings for the Entity Reference list format."

This error message is confusing, because the view *already* has fields on it, and "search field" is some special thing that EntityReference introduces. We need some guidance here like "Configure the search field by clicking Settings next to blah blah", cos I got stuck here.

However, when I go to that form, "User: Name" is the only option available. So why can't it just auto-select it by default? That would've made this super simple.

swentel’s picture

Status: Reviewed & tested by the community » Needs work

#1822458: Move dynamic parts (view modes, bundles) out of entity_info() got in, that's most likely causing the first failure that webchick gets.

catch’s picture

Assigned: catch » Unassigned

Just a note I started reviewing this, got around 1/3rd in, didn't have much feedback at that point, then got distracted.

If Dries or webchick gets to this before me and is happy committing I don't want to hold it up, so un-assigning myself.

amitaibu’s picture

I'm out for the next week, so I'd be happy if someone else can bring this patch home.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
9.86 KB
212.09 KB

Rerolled for #281.

effulgentsia’s picture

Updated the test introduced in #1854874: Add serializer support for XML for the value -> target_id change.

effulgentsia’s picture

Status: Needs review » Needs work

#278.2 is still happening, so "needs work" on that. I'm not clear on what the steps are for #278.3.

amitaibu’s picture

@effulgentsia, thanks for working on this - it would be great if you could use our development git in yched/entityreference-1801304

amateescu’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
1.95 KB
212.59 KB

So, let's get this back on track :)

@webchick, yes, 90% of the fails you saw were from HEAD changes and that Exception error was a missing t() wrapper on the exception message :/

Merged @effulgentsia interdiffs as commits in the proper branch and fixed #278.2 and #278.3. I'll followup with a proper response to #263 and #264.

Back to RTBC if the bot agrees.

webchick’s picture

Awesome. This seems to be working great now, and can't find much else to complain about that can't be done in a follow-up (e.g. that auto selecting of a search field thing). The display mode options are super handy, too!!

I think Dries might want another pass at this, since the intent is to use it in place of other *Reference fields and in any case we're blocked by thresholds atm anyway. :(

sun’s picture

The only way to make sense of this and to make this work and truly happen for D8 is to commit it now. (or rather: "yesterday")

effulgentsia’s picture

Assigned: Unassigned » Dries

I agree with #291. While I appreciate the general usefulness of us having thresholds that block feature requests, I also think it's fine to make sensible exceptions to that when it's for a critical feature that other work is waiting on. Assigning to Dries in case that's the kind of decision only he can make, though I have no objection to webchick or catch committing if they're feeling bold.

moshe weitzman’s picture

I also think this is a major building block for other patches and thus we can sensibly exempt it.

cweagans’s picture

+1 for an exemption. This is pretty important.

Dries’s picture

Status: Reviewed & tested by the community » Needs work

Good progress, but more work to do:

1) Testing this with a view, I got:

"RuntimeException: The view <em class="placeholder"></em> is no longer eligible for the <em class="placeholder">Content</em> field. in Drupal\views\Plugin\entity_reference\selection\ViewsSelection->initializeView() (line 128 of /Users/dr-evil/Sites/8.x/core/modules/views/lib/Drupal/views/Plugin/entity_reference/selection/ViewsSelection.php).
The website has encountered an error. Please try again later."

2) The 'Automatically create new entity' feature seems to have bugs when tried with users and default settings on content. It also raises a lot of questions as how it can work when entities have various required fields etc etc. Let's take this feature out for now, and create a follow-up issue to bring it back.

3) When initially selecting "User" content and "name" as sort field, then changing to a Content entity instead of User, no "name" field exists so you get this error on node/add/article:

An AJAX HTTP error occurred.
HTTP Result Code: 500
Debugging information follows.
Path: http://8x.localhost:8082/entity_reference/autocomplete/single/field_author/node/article/NULL
StatusText: Internal Server Error
ResponseText: A fatal error occurred: name not found
webchick’s picture

One other minor thing, that hopefully is easy to do, can we default "Link to entity" on the entity reference field display settings? Without that, it's hard to tell if it actually worked or not.

amitaibu’s picture

2) The 'Automatically create new entity' feature seems to have bugs when tried with users and default settings on content. It also raises a lot of questions as how it can work when entities have various required fields etc etc. Let's take this feature out for now, and create a follow-up issue to bring it back.

Maybe instead of taking it all out, we can - for now - limit it to taxonomy terms? like this after ER gets committed, we can continue with #1847596: Remove Taxonomy term reference field in favor of Entity reference.

btw, The required field did come up when we planned that feature, and is actually present also in the current D7 term-field -- you can create a new term, even if that bundle has a required field. Not sure what's the best solution is, though.

amitaibu’s picture

I'll be working on #296.2 (limiting to taxonomy terms) #296.3 and #297

webchick’s picture

I think that'll work. Just note that one of the problems we ran into testing this is that the newly created entities were unpublished by default. This was in the case of nodes, not sure about terms.

webchick’s picture

Btw, this is now 300 comments, so I went through and unpublished deleted a bunch of system message + one-line comments in an effort to keep the dreaded pager from appearing.

moshe weitzman’s picture

I want to echo a concern that Berdir raised in #203. Looks like we declare target_id as an integer. In general, the entity system does not require integer ids and we should ideally not start doing that here. Using integers mean we cannot reference to config entities, which have machine names as ids. Config entities are now a major part of Drupal, and even have EFQ support.

If we keep the design of a single entity type per field, we could make the schema dependent on the target type (details TBD). Until then, Perhaps we should explicitly exclude config entities

amitaibu’s picture

Status: Needs work » Needs review
FileSize
213.57 KB
5.89 KB

Patch fixes issues from #296 (all three issues) and #297

* "auto create" option is no visible in the UI only for taxonomy terms.
* When the field's target-type is changed, the instance's hander settings are reset

amitaibu’s picture

@moshe weitzman,

Until then, Perhaps we should explicitly exclude config entities

Yes, that's currently what happens in entity_reference_field_settings_form() we filter out ConfigEntity

Status: Needs review » Needs work
Issue tags: -Usability, -Killer End-User Features, -Accessibility

The last submitted patch, entityreference-1801304-302.patch, failed testing.

amitaibu’s picture

Status: Needs work » Needs review
Issue tags: +Usability, +Killer End-User Features, +Accessibility

#303: entityreference-1801304-302.patch queued for re-testing.

amitaibu’s picture

"Token scan" test passes on my local. Re-testing patch.

amitaibu’s picture

Green :)

moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community

OK, Lets go with this.

Is there a followup to change the storage from integer to varchar when referencing a config entity or a content entity that has non-integer keys (core doesn't do this but still).

sun’s picture

Not sure whether we need a separate/dedicated issue just for entity_reference? We have this one already:
#1823494: Field API assumes serial/integer entity IDs, but the entity system does not

sun’s picture

Issue summary: View changes

Updated issue summary.

star-szr’s picture

Here are some minor items for docs cleanup.

+++ b/core/modules/entity_reference/lib/Drupal/entity_reference/Plugin/Type/SelectionPluginManager.phpundefined
@@ -0,0 +1,84 @@
+   * Overrides Drupal\Component\Plugin\PluginManagerBase::createInstace().

Typo: createInstance().

+++ b/core/modules/entity_reference/lib/Drupal/entity_reference/Plugin/field/formatter/EntityReferenceFormatterBase.phpundefined
@@ -0,0 +1,118 @@
+          // This is an "auto_create" item, so allow access to it, as the entity doesn't
+          // exists yet, and we are probably in a preview.

+++ b/core/modules/entity_reference/lib/Drupal/entity_reference/Plugin/field/widget/AutocompleteTagsWidget.phpundefined
@@ -0,0 +1,76 @@
+          // Try to get a match from the input string when the user didn't use the
+          // autocomplete but filled in a value manually.

+++ b/core/modules/views/lib/Drupal/views/Plugin/entity_reference/selection/ViewsSelection.phpundefined
@@ -0,0 +1,226 @@
+    // Explode the 'arguments' string into an actual array. Beware, explode() turns an
+    // empty string into an array with one empty string. We'll need an empty array
+    // instead.

Can we wrap these at 80 characters please? There are a few other comments that are at 81 chars but I don't want to dump them all here.

amitaibu’s picture

@Cottser, thanks. If there are more places it would be very helpful if you could re-roll the patch.

YesCT’s picture

Status: Needs work » Reviewed & tested by the community

task A. add reference to user

following #195

  1. Apply the patch and enable the Entity Reference module.
  2. Go to Structure >> Content Types >> Article >> Managed Fields (admin/structure/types/manage/article/fields).
  3. Add new field "Author credit" , field type: entity reference, choose "Select list" (or whatever) as the widget. Click Save.
  4. Next, you're directed to select (the maximum number of values and) the type of entity you'd like to reference on the Field Settings form (field label says "Target type", description is "The entity type that can be referenced through this field."). (In the drop down, I see: Comment, Contact message, File, Content (the default), Taxonomy term, User.)
  5. Choose "User" and click Save field settings.
  6. On the next screen (the field "Edit" tab that is the field instance settings), you choose your "Reference method" in the Reference Type field set. Under "Reference method" there are two choices: "Default" (the default), and "Views: Filter by an entity reference view".
  7. With "Default", there are choices for: "Sort by" with values in the drop down: "- None -" and .. whoa a lot (uid, uuid, name, langcode, pass, mail, theme, signature, signature_format, created, access, login, status, timezone, preferred_langcode, preferred_admin_langcode, init, Picture (fid), Picture (alt), Picture (title), Picture (width), Picture (height). the choices under "Filter by" are values in the drop down: "- None -" and User role.
  8. Pick "Default" for the "Reference method", and "name" for "Sort by". A "Sort direction" drop down appears, with choices for the values "Ascending" (the default) and "Descending". Leave as ascending. Leave Filter by - None -. Click "Save settings"
  9. Yay, your field is made. Now go to Create content >> Article (node/add/article) and you'll see your lovely select list. Author Credit drop down has values: "- None -", Anonymous, (and in my case:) admin, Sally, tom

task B. add reference to user, filter choices by role

following #199

  1. picking up where left off, edit the author credit field (admin/structure/types/manage/article/fields/field_author_credit)
  2. change "Filter by" to "User role". check the box next the role. Save settings.
  3. Create an article and see just the users with that role in the drop down (much easier than what was needed in #199)
  4. save the article. (shows author credit fine. link to that user works.)
  5. go to front page (no errors. but dont see author credit since it's not part of the "teaser" display.)
  6. configure the article content type to show author credit field in the teaser. (the default rendered entity doesn't show much. change reload front page. (change format to label, and click the gear to change from "no link" to "link to the referenced entity". click save. reload front page again. all is well, it shows and no errors)

task C. add reference to user, filter by custom view

Since that was so easy, going through similar to #199 view wizard

  1. picking up where left off, edit the author credit field (admin/structure/types/manage/article/fields/field_author_credit)
  2. change the "Reference Type" "Reference method" to "Views: Filter by an entity reference view". which changes the form removing the Filter and Sort by and showing the message "No eligible views were found. Create a view with an Entity Reference display, or add such a display to an existing view."
  3. Click "Create a view" here.
  4. View name: people start with a. (I did not see a field set for entity reference display.) Show: Users, click "Save and edit".
  5. Now on the "Advanced" (advanced is a section on the next view screen. dont need that) next view screen, you get to do the following elaborate dance...
  6. Next screen shows "Displays". Click "+ Add" next to "Master". (hover just right) and click EntityReference. (follow-up #1904896: when create view from entity reference field instructions to create one, default to have entity reference display)
  7. All appears fine, but if you scroll down to the preview section, you will see: red error: 'Display "EntityReference" needs a selected search fields to work properly. See the settings for the Entity Reference list format.' (check if there is a follow-up to auto pick the primary field for the type follow-up #1904900: Default auto pick the primary field for the type in view wizard to avoid red error: 'Display "EntityReference" needs a selected)
  8. Despite being scary, the message is descriptive, so click on "Settings" next to Entity Reference list (check if there is a follow-up for Maybe just link right there from the error message directly? add as possible solution to #1904900: Default auto pick the primary field for the type in view wizard to avoid red error: 'Display "EntityReference" needs a selected)
  9. Click it and get a modal, with a required field with one option and nothing checked. ;) Tee hee. Check it. Click Apply. (still autocomplete widget message problem. (still says search fields. check for a follow-up for I don't really understand what "Search field" means. It seems to think I know more about how this is implemented under the hood than I currently do. Can you explain this, and we can try and come up with a better word?) #1904902: replace "autocomplete" in modal with more accurate word and replace "Search field" during entity reference view wizard process
  10. Yay, now the preview shows stuff:
  11. Add a filter criteria for User: name
  12. (cant actually do the example of users that start with a. so use operator Is not one of, and then fill in the autocomplete for 'admin')
  13. Also add a sort criteria since you most likely don't want the list sorted by newest first. Another 3 steps. Then you want to remove the one you don't want. Another 2 steps. [OH! when click remove for newest first, or whatever it was, jumped out of the overlay and go error: Page not found The requested page "/javascript%3Avoid%28%29" could not be found. Start all the way over at 1. (oh, pick username raw this time, operator starts with, enter a). OH! reordering filters also errors. start over again, again... dont reorder or remove anything.]
  14. Cool, preview updated to show only users that start with 'a'
  15. Save the view. (save is in the upper right corner)
  16. Now, simply go to node/add/article and... hey wait. It's still looking like this. What gives? :( (yep, this still happens)
  17. Go back to admin/structure/types/manage/article/fields/field_author_credit. Pick "Views: Filter by an entity reference view" again for Reference menthod. Drop down looks different this time. Not just message to create one, now there is a setting labeled "View used to select the entities" with a drop down with just one value (the machine name of the view I just made appended with "- EntityReference". There is another field labeled: View arguments. Leave that blank. Click "Save settings".
  18. edit or create an article and see under Author Credit only those user's whose username starts with 'a'.

Whew!

[edit: OK. I'm saving what I have so far. My goal was to try and go through some steps to test, just regular first to get a feel for it, and then to try it with some multilingual something. But I didn't make it that far yet. I'll try and come back to this later today. I also need to check and see if follow-ups I confirmed are still problems are already listed in the issue summary in the follow-up list. So I'll edit this comment later with that info about the follow-ups: if they exist already or not. [edit: added follow-ups to issue summary]] [edit: resuming above with step 14.]

YesCT’s picture

Issue summary: View changes

Updated issue summary, added links to comments that sound like they have steps to test.

webchick’s picture

Status: Reviewed & tested by the community » Needs work

Sorry to keep pushing back on this, but if there's anything we don't need right now it's more critical bugs. :(

That JS error YesCT ran into seems like it needs to be addressed. Not sure if it's strictly related to this patch, though.

We hit one big issue in the most recent round of testing, which is that when we deleted the Entity Reference display in our view and then went to add a different one, every node-related page fails hard with:

Error message
RuntimeException: The view <em class="placeholder">ters</em> is no longer eligible for the <em class="placeholder">A view of terms2</em> field. inDrupal\views\Plugin\entity_reference\selection\ViewsSelection->initializeView() (line126 of/Users/webchick/Sites/8.x/core/modules/views/lib/Drupal/views/Plugin/entity_reference/selection/ViewsSelection.php).
The website has encountered an error. Please try again later.

This seems to be a variation on #1898804: Create a view block, place the block, delete the block, BOOM your site is dead.

Additionally, filtering users by the "Authenticated user" role didn't work; the list was empty. So either we should fix that (if it can be fixed; Moshe pointed out that that might not be queryable) or we should remove it from the list of available filter options. I also found it weird that on every other form the filters are displayed inline, but with users it's in a dropdown with only one option.

Additionally, Dries had some feedback on some wording things:

- The fieldset that you use to configure how the reference field gets populated is labeled "Reference Type", however that doesn't actually describe what options lie below. Suggestion was "Reference Settings" although the whole page is a settings form, so not sure.

- There are other labels that are extremely technical sounding like "View used to select the entities" and "Create referenced entities if they don't already exist" At the point of this form, you already know what kind of entity you're dealing with, so it should just be "content" or "terms" or whatever.

I imagine this isn't solvable, but it's really odd to see "EntityReference" strung together in the Views display listings. No way of making that "Entity Reference" is there?

We'll take another pass at this tomorrow, if it's ready, and Dries thinks it may be able to be committed then.

YesCT’s picture

Status: Reviewed & tested by the community » Needs work

I added followups (and edit, again, #313). I promise not to edit that comment again.

If the followups are duplicates, sorry. Just close them duplicates or let me know and I'll do it.

YesCT’s picture

Issue summary: View changes

Updated issue summary with followups from #313 and other places

YesCT’s picture

Status: Needs review » Needs work

Steps to test with multilingual

  1. extend by enabling content and interface translation modules
  2. add 3 or so languages at admin/config/regional/language
  3. add a user, pick a language in their user settings
  4. enable translation on article (either in language content type settings page, or edit content type and in the language settings show language select and enable translation)
  5. add a entity reference field to article pick type: user. pick may translate this field.
  6. use reference method default, sort by langcode. save settings.
  7. create article
  8. selet some things. (nothing breaks)
  9. edit the field, allow 2 values.
  10. edit article. pick two users. (nothing breaks)
  11. translate article... no translatable fields... hm. we make the ER translatable. check admin/structure/types/manage/article/fields again in the global field settings tab. enable translation (must not have done that when created it.)
  12. reload the translations tab of the article node.
  13. add a translation. pick different users for the translation.
  14. check the node in one language has the right users. check the node in the other language has the right users for that language. great.

do it again more careful... with a use case.
task B. select a user reference, and only list user's who language setting in their account matches the language of the translation being edited.

make an entity reference view that takes an argument and lists users with that language in their account.

uh, I dont know how to do that... [edit: #1904934: use views context and translation langcode to filter user listing ]

YesCT’s picture

I can confirm that the deleting filter criteria from the rearrange is in a clean install of 8.x. Tim says a separate issue for that is coming.

I think that leaves mostly the other things in #314

tim.plunkett’s picture

star-szr’s picture

FileSize
26.59 KB
213.5 KB

@Amitaibu - Thanks! I would have done a reroll in the first place but honestly I wasn't sure what was happening with sandboxes etc. - see #288.

Here's a reroll with some more docs updates. I tried to make the capitalization of "Entity Reference" more consistent as well when referring to the module and not the field. See #1430452: Use Proper Name Case for core modules.

The only code-related change I made was to remove one commented-out line of code from \Drupal\entity_reference\Plugin\views\display\EntityReference.

amateescu’s picture

Status: Needs work » Needs review
FileSize
3.13 KB
213.64 KB

I took care of most the issues from #314, namely:

- removed the authenticated role from the list
- changed that pesky EntityReference to Entity Reference
- switched the RuntimeException to a warning message, and also tweaked the message a bit with Bojhan on IRC

The strings that Dries thought were too technical were actually the result of Bojhan's UX review..

Also committed Cottser's patch to the sandbox, thanks for it btw! And thanks to @YesCT, we now have followups for everything else.

Bojhan’s picture

Status: Needs work » Needs review

@amateescu I cannot remember reviewing those texts? I reviewed the primary labels, not each select option.

amateescu’s picture

That's true, some others came from @eaton in this issue :) Anyway, any suggestion for improving them are welcome, as usual.

effulgentsia’s picture

Status: Needs review » Reviewed & tested by the community

#319 and #320 look good. I think polishing strings and other things can happen more productively in follow ups, so RTBC.

Dries’s picture

Assigned: Dries » Unassigned
Priority: Critical » Normal
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs change record

This looks good now. Committed to 8.x. :-)

Setting to 'needs work' as there are a couple of follow-up tasks that need to be created and linked to from this issue.

Also could benefit from a CHANGELOG.txt and/or a Change Notice.

Thanks everyone!

amateescu’s picture

Status: Needs work » Needs review
FileSize
447 bytes

Wow, creating this small patch felt so good :)

I also updated http://drupal.org/node/1796546 to include the ER field.

cweagans’s picture

Status: Needs review » Reviewed & tested by the community
webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. Thanks!

tim.plunkett’s picture

Title: Add Entity reference field » Change notice: Add Entity reference field
Category: feature » task
Priority: Normal » Critical
Status: Fixed » Active
Issue tags: -Needs change record
tim.plunkett’s picture

Title: Change notice: Add Entity reference field » Add Entity reference field
Category: task » feature
Priority: Critical » Normal
Status: Active » Fixed

http://drupal.org/node/1796546 already covers this. Sorry for the noise!

effulgentsia’s picture

Priority: Normal » Critical

Back to original priority for posterity/statistics.

effulgentsia’s picture

Issue summary: View changes

Updated issue summary with location of most recent steps to test.

Status: Fixed » Closed (fixed)

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

Anonymous’s picture

Issue summary: View changes

add another follow up

xjm’s picture

xjm’s picture

Issue summary: View changes

Updated issue summary.

jhedstrom’s picture

cilefen’s picture