Separate "Selection" and "behavior" to different plugins

Wow, I think I was high when I wrote this issue ;) No problem just extending the base handler...

Ok. Not high :)

If we have class OgHandler_base extends EntityReferenceHandler_base, then with #1343918: Invoke field_[op] in the handler entityreference_field_invoke_handler() will not find OG's handler but (for example) EntityReferenceHandler_node -- thus will not call my custom function fieldLoad|Insert|Update

So I wonder if those extending classes should not be part of the EntityReferenceHandler_base itself.

Seems like a great separation.

> Define if behaviors are field level, instance level
I think it should be one or the other, and we should have this in the behviour plugin, so for example:

$plugin = array(
  'title' => t('Test behavior'),
  'class' => 'EntityReferenceBehaviorExample',
  'weight' => 10,
  // Behavior is per field. could be also 'instance'.
  'level' => 'field',
  // Access callback to determine if the behavior can be used (e.g. maybe it will appear only for a certain selection handler).
  'access callback' => 'foo_access',
);

Also maybe this patch should include #1399048: Pass $instance to handler. In that case If we add $instance = NULL to the signature, and we cache the handler, we will need to make sure we return a fresh object in case the instance was populated.

+++ b/entityreference.moduleundefined
@@ -72,20 +73,66 @@ function entityreference_field_is_empty($item, $field) {
+function entityreference_get_selection_handler($field) {
+  $object_cache = drupal_static(__FUNCTION__);

Oops, correct status

Ok, I've created a 1343854 branch, and I'll commit my work there.

Quick update on the 1343854 branch (code is there, no patch here):

- I've added the UI -- entityreference_get_behavior_elements(). I tried to use "Vertical tabs", but it sticks a form element inside the field settings, so I just show/ hide the behavior's settings if it is enabled.
- I've added a base class for behavior plugins, and added an access() method.
- On the field level we have $field['settings']['handler_settings']['behaviors'] -- the reason it is under the handler_settings, is that I want to make sure we show only the correct behaviors when the selection handler changes.

The current issue that I see is that we might have problems passing the instance info to the instance level behaviors on hook_field_load() -- as we get $instances and not a single $instance.

Next is passing the $instance to the selection handler when possible.

@Damz,

I've started working on OG's side, and it looks good, however I see there's still the problem of extending the correct selection class. I'm doing class OgSelectionHandler extends EntityReference_SelectionHandler_Generic, but how can I make sure the entity specific handlers are invoked when needed (e.g. EntityReference_SelectionHandler_Generic_node)?

We still need to deal with #11, but here's what I've got so far (attached full patch + interdiff from #40). Setting to needs review to get other people's attention :)

> but how can I make sure the entity specific handlers are invoked when needed (e.g. EntityReference_SelectionHandler_Generic_node)?

I can actually do it via OG's code - I can make sure to call those sub-classes, so #11 is not a concern.

This patch adds two more minor fixes:

ctools_plugin_load_class() seems to have some caching issues, and might return wrong values, so I've added ctools_get_plugins('entityreference', 'selection'); before it. Need to open an issue for CTools.

In EntityReference_SelectionHandler_Generic::buildEntityFieldQuery I've changed the entity access tag to be $query->addTag('entity_field_access');. The reason is that otherwise, if we pass 'node_access' we might get a query in _node_query_node_access_alter(), from this lines:

      // The node_access table has the access grants for any given node so JOIN
      // it to the table containing the nid which can be either the node
      // table or a field value table.
      if ($type == 'node') {
        $access_alias = $query->join('node_access', 'na', '%alias.nid = ' . $nalias . '.nid'); // <-- Here is the error $nalias might no have nid.
      }
      else {
        $access_alias = $query->leftJoin('node_access', 'na', '%alias.nid = ' . $nalias . '.entity_id');
        $base_alias = $nalias;
      }

> Could we look into the cache issue.

Yap, I am planning on doing that.

> I'm not quite sure about the access change.

Yes, I might be missing somehting, but we do need to change it to something else, as in Og's selection handler - OgSelectionHandler::buildEntityFieldQuery I have

$query->fieldCondition(OG_GROUP_FIELD, 'value', 1, '=');

which results with Unknown column 'field_data_group_group0.nid' (because indeed, there is no nid column). By passing entity_field_access, we still get to be processed by node-access.

> Could we look into the cache issue.

Still not sure exactly why, but I tracked it down to ctools_plugin_load_includes(). It is doing an includerequire_once DRUPAL_ROOT . '/' . $file->uri; expecting to populate the $plugin, but it doesn't puplate the base.inc's $plugin.

btw, For those that want to see the cache issue:

1. Get OG branch #1342632: Deprecate OG group entity
2. Add a ref field with OG selection handler
3. Add a ref field with ER's selection handler -- notice how there are no values in that field, due to the above issue.

I've fixed the caching issue, by following export-ui example, and separating the plugin info into an base.inc file, and the class into an base.class.php

@Damien, Apart of updating the docs, I think it's ready for a code review and figuring out if the node-access thing I did is indeed correct.

Updated PHPdocs.

> but overall I think we can go ahead as is,

Great! I'll go over your changes and commit. I'll also need re-check the access tag issue with OG.

> and release another beta with this.

Ok, I have a few other smaller issues in the issue queue I'd like to push before we tag the next beta.

Committed!